Add ResultsDirectory arg to cli #322
Conversation
* Add Acceptance test for ResultsDirectory argument * Update Unittest of runsettings related Argument process
|
|
||
| if (EqtTrace.IsInfoEnabled) | ||
| { | ||
| EqtTrace.Info("Using .Net Framework version:{0}", commandLineOptions.TargetFrameworkVersion); | ||
| } | ||
|
|
||
| if (this.commandLineOptions.TargetFrameworkVersion != Framework.DefaultFramework |
There was a problem hiding this comment.
Can we get more context on why this warning is interesting?
| @@ -190,6 +192,9 @@ private int GetArgumentProcessors(string[] args, out List<IArgumentProcessor> pr | |||
| var processorsToAlwaysExecute = processorFactory.GetArgumentProcessorsToAlwaysExecute(); | |||
| processors.AddRange(processorsToAlwaysExecute); | |||
|
|
|||
| // Initialize Runsettings with defaults | |||
| RunSettingsUtilities.AddDefaultRunSettings(RunSettingsManager.Instance); | |||
There was a problem hiding this comment.
Can the defaults be initialized on the first query to ActiveRunSettings?
There was a problem hiding this comment.
Currenlty we can't assign defaults on first query, because it is public type in different assembly.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Xml; |
| argument = Path.GetFullPath(argument); | ||
| } | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Should we catch only expected exceptions?
There was a problem hiding this comment.
👍 Added expecting exceptions.
| @@ -173,19 +173,12 @@ private IXPathNavigable GetRunSettingsDocument(string runSettingsFile) | |||
| { | |||
| runSettingsDocument = XmlRunSettingsUtilities.CreateDefaultRunSettings(); | |||
|
|
|||
| #if NET46 | |||
There was a problem hiding this comment.
This is not required. Legacy never supports FrameworkCore10.
| @@ -69,6 +69,11 @@ internal enum HelpContentPriority | |||
| ParallelArgumentProcessorHelpPriority, | |||
|
|
|||
| /// <summary> | |||
| /// ResultsDirectoryArgumentProcessor Help | |||
| /// </summary> | |||
| ResultsDirectoryArgumentProcessorHelpPriority, | |||
There was a problem hiding this comment.
Please get the help display approved from @pvlakshm
|
|
||
| runSettings = GetEffectiveRunSettings(runSettings, commandlineOptions); | ||
| public static void UpdateRunSettingsXmlDocument(XmlDocument xmlDocument, string key, string value) |
There was a problem hiding this comment.
This API should take IRunSettingsProvider similar to other APIs in the class.
There was a problem hiding this comment.
This method exists to make efficient updates from CLIRunSettingsArgumentProcessor.cs. Making it private method for clean design.
| runSettings = GetEffectiveRunSettings(runSettings, commandlineOptions); | ||
| public static void UpdateRunSettingsXmlDocument(XmlDocument xmlDocument, string key, string value) | ||
| { | ||
| var node = GetXmlNode(xmlDocument, key) ?? RunSettingsUtilities.CreateNode(xmlDocument, key.Split('.')); |
There was a problem hiding this comment.
If possible, we should keep the logic of splitting by . restricted to one place.
There was a problem hiding this comment.
Moved to CreateNode().
| /// <param name="commandlineOptions"> The command line options specified. </param> | ||
| /// <returns></returns> | ||
| internal static string GetRunSettings(IRunSettingsProvider runSettingsProvider, CommandLineOptions commandlineOptions) | ||
| public static void UpdateRunSettings(IRunSettingsProvider runSettingsManager, string runsettingsXml) |
There was a problem hiding this comment.
This class can possibly become RunSettingsProviderExtensions and all APIs can be extension methods.
| { | ||
| var settingsXml = runSettingsProvider.ActiveRunSettings.SettingsXml; | ||
|
|
||
| #if net46 |
| { | ||
| runSettings = EmptyRunSettings; | ||
| #if net46 | ||
| doc = (XmlDocument)XmlRunSettingsUtilities.CreateDefaultRunSettings(); |
| #if net46 | ||
| doc = (XmlDocument)XmlRunSettingsUtilities.CreateDefaultRunSettings(); | ||
| #else | ||
| using ( |
There was a problem hiding this comment.
Removed unnecessary #if.
|
|
||
| runSettings = GetEffectiveRunSettings(runSettings, commandlineOptions); | ||
| public static void UpdateRunSettingsXmlDocument(XmlDocument xmlDocument, string key, string value) |
There was a problem hiding this comment.
value is a keyword. We can use some other name.
| @@ -623,4 +623,16 @@ | |||
| <data name="MalformedRunSettingsKey" xml:space="preserve"> | |||
| <value>One or more runsettings provided contain invalid token</value> | |||
| </data> | |||
| <data name="ResultsDirectoryArgumentHelp" xml:space="preserve"> | |||
| <value>--ResultsDirectory|/ResultsDirectory | |||
There was a problem hiding this comment.
Tagging @pvlakshm for review.
|
|
||
| this.InvokeVsTest(arguments); | ||
|
|
||
| Assert.IsTrue(File.Exists(trxFilePath), $"Expexted Trx file: {trxFilePath} not created in results directory"); |
| Assert.IsTrue(File.Exists(trxFilePath), $"Expexted Trx file: {trxFilePath} not created in results directory"); | ||
| } | ||
|
|
||
| //Getting Current directory C:\Windows\system32 https://github.com/Microsoft/vstest/issues/311 |
There was a problem hiding this comment.
I think @singhsarab has a fix for #311 in pr. We can remove this after merge from master.
There was a problem hiding this comment.
PR has been reverted, keeping it as Ignored.
| @@ -74,39 +77,42 @@ public void InitializeShouldThrowIfArgumentIsEmpty() | |||
| [TestMethod] | |||
| public void InitializeShouldThrowIfArgumentIsInvalid() | |||
| { | |||
| var executor = new FrameworkArgumentExecutor(CommandLineOptions.Instance); | |||
| var executor = new FrameworkArgumentExecutor(CommandLineOptions.Instance, RunSettingsManager.Instance); | |||
There was a problem hiding this comment.
Use mocks for static singletons.
There was a problem hiding this comment.
Due to Moq limitation of unable to mock extensions methods, using TestableRunSettingsProvider.
| @@ -54,7 +57,7 @@ public void CapabilitiesShouldReturnAppropriateProperties() | |||
| [TestMethod] | |||
| public void InitializeShouldThrowIfArgumentIsNonNull() | |||
| { | |||
| var executor = new ParallelArgumentExecutor(CommandLineOptions.Instance); | |||
| var executor = new ParallelArgumentExecutor(CommandLineOptions.Instance, RunSettingsManager.Instance); | |||
There was a problem hiding this comment.
Can we make executor an instance variable?
| @@ -36,7 +40,7 @@ public void CapabilitiesShouldReturnAppropriateProperties() | |||
| { | |||
| var capabilities = new PlatformArgumentProcessorCapabilities(); | |||
| Assert.AreEqual("/Platform", capabilities.CommandName); | |||
| Assert.AreEqual("--Platform|/Platform:<Platform type>\n Target platform architecture to be used for test execution. \n Valid values are x86, x64 and ARM.", capabilities.HelpContentResourceName); | |||
| Assert.AreEqual("--Platform|/Platform:<Platform type>" + Environment.NewLine + " Target platform architecture to be used for test execution. " + Environment.NewLine + " Valid values are x86, x64 and ARM.", capabilities.HelpContentResourceName); | |||
| [TestMethod] | ||
| public void InitializeShouldThrowIfArgumentIsNull() | ||
| { | ||
| var executor = new ResultsDirectoryArgumentExecutor(CommandLineOptions.Instance, RunSettingsManager.Instance); |
There was a problem hiding this comment.
DRY violation. Please move to ctor.
There was a problem hiding this comment.
Moved to instance variable.
| @@ -1,6 +1,7 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
|
|
|||
|
|
|||
| namespace Microsoft.VisualStudio.TestPlatform.CommandLine.UnitTests.Processors | |||
There was a problem hiding this comment.
Change RunSettingsArgumentProcessortTests.cs to RunSettingsArgumentProcessorTests.cs
| @@ -180,6 +240,7 @@ public void InitializeShouldSetActiveRunSettingsForTestSettingsFiles() | |||
| StringAssert.Contains(settingsProvider.ActiveRunSettings.SettingsXml, "<RunSettings>\r\n <RunConfiguration>\r\n <TargetPlatform>X86</TargetPlatform>\r\n <TargetFrameworkVersion>Framework45</TargetFrameworkVersion>\r\n </RunConfiguration>\r\n <MSTest>\r\n <SettingsFile>C:\\temp\\r.testsettings</SettingsFile>\r\n <ForcedLegacyMode>true</ForcedLegacyMode>\r\n </MSTest>\r\n <DataCollectionRunSettings>\r\n <DataCollectors />\r\n </DataCollectionRunSettings>\r\n</RunSettings>"); | |||
| } | |||
|
|
|||
| [Ignore] // Code coverage yet to add | |||
There was a problem hiding this comment.
We need not ignore this test.
There was a problem hiding this comment.
Updated the test and removed [Ignore].
| @@ -63,9 +77,10 @@ public void GetExecuterShouldReturnRunTestsArgumentProcessorCapabilities() | |||
| [TestMethod] | |||
| public void CapabilitiesShouldReturnAppropriateProperties() | |||
| { | |||
| System.Diagnostics.Debugger.Launch(); | |||
| } | ||
|
|
||
| [TestMethod] | ||
| public void GetRunSettingsShouldReturnDefaultRunSettingsIfProviderIsNull() | ||
| public void UpdateRunSettingsShouldUpdateGivenSettingsXml() |
There was a problem hiding this comment.
We need to add unit tests for other APIs.
…or RunSettingsProviderExtensions *Nit fixes
| @@ -91,7 +91,7 @@ public string PublishDirectory | |||
| get | |||
| { | |||
| string value = string.Empty; | |||
| if (this.runningInCli) | |||
| if (this.runningInCli || true) | |||
There was a problem hiding this comment.
Why is this change required?
There was a problem hiding this comment.
This is not suppose to commit, this is work around to run Integration tests from IDE. will be removed .
* Update results directory help content
Related Issue: #299