-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C++ UWP Reflection based discovery #1336
C++ UWP Reflection based discovery #1336
Conversation
return sources; | ||
List<string> actualSources = new List<string>(); | ||
|
||
// We are doing this specifically for UWP, should we extract it out to some other utility? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. This code takes us back couple of years, albeit the if
used to be #if
in those days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of moving this to UWP provider, & invoke it via reflection so it always owns the ownership of giving us the correct exe, but that would still mean Desktop Provider would need to understand what all packages types it can support, however it need not understand the logic to extract sources from that package. Thoughts?
public static bool Exists(string filePath) | ||
{ | ||
// Appxrecipe gets renamed vs.appxrecipe, which test platform is unaware of, so if queried for appxrecipe, return true | ||
if (filePath.EndsWith(".appxrecipe", System.StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is business logic getting in platform layer :) Would the consumer of platform APIs expect such filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most unfortunate hack I'm introducing since we have to go back to doing UWP discovery in fullCLR process.
@@ -221,7 +223,7 @@ private Guid GetTestId() | |||
// For example in the database adapter case this is not a file path. | |||
string source = this.Source; | |||
|
|||
if (File.Exists(source)) | |||
if (PlatformFile.Exists(source)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but I do not want to create a new Instance of FileHelper to check for file Exists. Correct thing to do would be to move it out of coreutilities, & make it a static class in platform abstraction, but that would require a lot of tests changes.
I'm still thinking on how to do it better.
I'm not taking the fix in C++ adapters, as it required us to introduce package concept over there as well. Also we had to add the logic of creating TestCase ID in both C++ Desktop, & UWP adapters. |
Please see comment in TestCase .cs file
/// <summary> | ||
/// File Abstraction | ||
/// </summary> | ||
public static class PlatformPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have the code there itself. remove this.
@dotnet-bot test Windows_NT / Debug Build |
No description provided.