-
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
fixed reading test adapter paths from runsettings #455
Conversation
Hi @rahulpaw, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
{ | ||
var runConfiguration = XmlRunSettingsUtilities.GetRunConfigurationNode(runSettings); | ||
|
||
IEnumerable<string> testAdaptersPaths = null; |
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.
Please return Enumerable.Empty instead. The consumer can directly do a foreach.
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.
addressed
IEnumerable<string> testAdaptersPaths = null; | ||
if (runConfiguration != null) | ||
{ | ||
if (!runConfiguration.TestAdaptersPathsSet) |
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.
We can invert if
?
foreach (string customTestAdaptersPath in customTestAdaptersPaths) | ||
{ | ||
string adapterPath = string.Empty; | ||
adapterPath = Path.GetFullPath(Environment.ExpandEnvironmentVariables(customTestAdaptersPath)); |
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.
Merge this and above line to var adapterPath
string[] expectedResult = new string[] { @"C:\testadapterpath", @"D:\secondtestadapterpath" }; | ||
|
||
string[] result = (string[])RunSettingsUtilities.GetTestAdaptersPaths(settingXml); | ||
if (expectedResult.Length == result.Length) |
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.
Please use CollectionAssert
methods to compare results.
EqtTrace.Warning(string.Format("AdapterPath Not Found:", adapterPath)); | ||
continue; | ||
} | ||
List<string> adapterFiles = new List<string>( |
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 logic is duplicated across many files. Should we consolidate?
@rahulpaw with this change, will the final testadapterpaths continue to be a union of
|
@@ -76,6 +82,8 @@ public ITestRunRequest CreateTestRunRequest(TestRunCriteria testRunCriteria) | |||
throw new ArgumentNullException("testRunCriteria"); | |||
} | |||
|
|||
UpdateTestAdapterPaths(testRunCriteria.TestRunSettings); | |||
|
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 caused issue in Tpv1, when in VS IDE scenario data collectors were enabled, & runsettings were used to provide adapter path, it resulted in DataCollection(fakes, CodeCoverage) not working.
The workaround we used was to not update them here, but instead like we process TestAdapterPath in CLI, similarly process it, & apply it on ITestPlatform when provided via runsettings
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 would like to take a deeper look at the datacollector scenarios. but right now TPv2 does not support DataCollectors.
Fix for Issue #236.
Reads the test adapter path from runsettings.
Testing:
Added UT, Enabled Acceptance test.
validated console and IDE scenario.
Ran Test.cmd