Skip to content
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

Merged
merged 7 commits into from
Dec 21, 2017
Merged

C++ UWP Reflection based discovery #1336

merged 7 commits into from
Dec 21, 2017

Conversation

mayankbansal018
Copy link
Contributor

No description provided.

return sources;
List<string> actualSources = new List<string>();

// We are doing this specifically for UWP, should we extract it out to some other utility?
Copy link
Contributor

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.

Copy link
Contributor Author

@mayankbansal018 mayankbansal018 Dec 19, 2017

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))
Copy link
Contributor

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?

Copy link
Contributor Author

@mayankbansal018 mayankbansal018 Dec 19, 2017

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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.

@mayankbansal018
Copy link
Contributor Author

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.

/// <summary>
/// File Abstraction
/// </summary>
public static class PlatformPath
Copy link
Contributor

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.

@mayankbansal018
Copy link
Contributor Author

@dotnet-bot test Windows_NT / Debug Build

@mayankbansal018 mayankbansal018 merged commit 0a7475c into microsoft:master Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants