Add a tool to migrate testsettings to runsettings #1600

kaadhina · 2018-05-16T10:46:27Z

Description

Adding a tool to migrate testsettings to runsettings

singhsarab · 2018-05-16T12:12:48Z

src/SettingsMigrator/Migrator.cs

+        /// </summary>
+        /// <param name="oldRunsettingsPath"></param>
+        /// <param name="newRunsettingsPath"></param>
+        public void MigrateRunsettings(string oldRunsettingsPath, string newRunsettingsPath)


nit: RunSettings*

singhsarab · 2018-05-16T12:27:27Z

src/SettingsMigrator/Migrator.cs

+        /// <param name="testsettingsPath"></param>
+        /// <param name="newRunsettingsPath"></param>
+        /// <param name="oldRunSettingsContent"></param>
+        public void MigrateTestsettings(string testsettingsPath, string newRunsettingsPath, string oldRunSettingsContent = null)


Let's refactor this to create file separately, and have a common api for translating testsettings xml to required runsettings xml
MigrateLegacyNodesToRunSettings

singhsarab · 2018-05-16T12:30:06Z

src/SettingsMigrator/Migrator.cs

+                var executionNode = root.SelectSingleNode(@"/TestSettings/Execution");
+
+                string testTimeout = null;
+                if (timeoutNode != null && timeoutNode.Attributes[TestTimeoutAttributeName] != null)


Add warning for deployment and script timeout

singhsarab · 2018-05-16T12:34:18Z

src/SettingsMigrator/Migrator.cs

+                if (deploymentNode != null || scriptnode != null || assemblyresolutionNode != null ||
+                    !string.IsNullOrEmpty(parallelTestCount) || !string.IsNullOrEmpty(testTimeout) || hostsNode != null)
+                {
+                    AddLegacyNodes(deploymentNode, scriptnode, testTimeout, assemblyresolutionNode, hostsNode, parallelTestCount, newXmlDoc);


AddLegacyNodes [](start = 20, length = 14)

Create a class for all these nodes.

singhsarab · 2018-05-16T12:36:15Z

src/SettingsMigrator/Migrator.cs

+        private void AddLegacyNodes(XmlNode deploymentNode, XmlNode scriptnode, string testTimeout, XmlNode assemblyresolutionNode, XmlNode hostsNode, string parallelTestCount, XmlDocument newXmlDoc)
+        {
+            //Remove if the legacy node already exists.
+            var legacyNode = newXmlDoc.DocumentElement.SelectSingleNode(@"/RunSettings/LegacySettings");


Let's put a warning for exist

singhsarab · 2018-05-16T12:36:55Z

src/SettingsMigrator/Migrator.cs

+            }
+
+            // Execution node.
+            if (assemblyresolutionNode != null || !string.IsNullOrEmpty(parallelTestCount) || !string.IsNullOrEmpty(testTimeout) || hostsNode != null)


assemblyresolutionNode [](start = 16, length = 22)

Nit: Resolution*

singhsarab · 2018-05-16T12:38:53Z

src/SettingsMigrator/Migrator.cs

+                if (!string.IsNullOrEmpty(testTimeout))
+                {
+                    var newTimeoutsNode = newXmlDoc.CreateNode(XmlNodeType.Element, TimeoutsNodeName, null);
+                    var testtimeoutattribute = newXmlDoc.CreateAttribute(TestTimeoutAttributeName);


testtimeoutattribute [](start = 24, length = 20)

Nit: camelCase

singhsarab · 2018-05-16T12:41:14Z

src/SettingsMigrator/Migrator.cs

+                var websettingsNode = root.SelectSingleNode(@"/TestSettings/Execution/TestTypeSpecific/WebTestRunConfiguration");
+                var oldDatacollectorNodes = root.SelectNodes(@"/TestSettings/AgentRule/DataCollectors/DataCollector");
+                var timeoutNode = root.SelectSingleNode(@"/TestSettings/Execution/Timeouts");
+                var assemblyresolutionNode = root.SelectSingleNode(@"/TestSettings/Execution/TestTypeSpecific/UnitTestRunConfig");


assemblyresolutionNode [](start = 20, length = 22)

unitTestConfigNode

singhsarab · 2018-05-16T12:47:56Z

src/SettingsMigrator/Resources/Resources.resx

@@ -0,0 +1,123 @@
+<?xml version="1.0" encoding="utf-8"?>
+<root>


Designer needs to be fixed.

singhsarab · 2018-05-17T08:30:20Z

using System;

Follow the standards for commenting and using statements

Refers to: src/SettingsMigrator/TestSettingsNodes.cs:1 in 6922920. [](commit_id = 6922920, deletion_comment = False)

singhsarab · 2018-05-17T08:45:25Z

src/SettingsMigrator/Migrator.cs

+        /// </summary>
+        /// <param name="testSettingsPath"></param>
+        /// <param name="newRunSettingsPath"></param>
+        /// <param name="oldRunSettingsContent"></param>


This can be private.

singhsarab · 2018-05-17T08:55:46Z

Overall looks good. @smadala @mayankbansal018 Can you take a look?

singhsarab · 2018-05-17T08:58:54Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+    public class MigratorUnitTests
+    {
+        [TestMethod]
+        public void MigratorGeneratesCorrectRunsettingsForEmbeddedTestSettings()


Add tests for negative cases as well.
Incorrect testsettings, incorrect path etc.

singhsarab · 2018-05-17T08:59:37Z

src/SettingsMigrator/Migrator.cs

+        }
+
+
+        public void MigrateTestSettings(string oldTestSettingsPath, string newRunSettingsPath)


smadala · 2018-05-17T10:44:46Z

src/SettingsMigrator/Migrator.cs

+// 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


Should we rename Microsoft.VisualStudio.TestPlatform.SettingsMigrator?

smadala · 2018-05-17T11:51:54Z

src/SettingsMigrator/Migrator.cs

+                    // Expand path relative to runSettings location.
+                    if (!Path.IsPathRooted(testSettingsPath))
+                    {
+                        testSettingsPath = Path.Combine(oldRunSettingsPath, testSettingsPath);


Should we combine with directory of oldRunSettingsPath?

Add test for same.

smadala · 2018-05-17T11:56:03Z

src/SettingsMigrator/Migrator.cs

+                }
+                else
+                {
+                    Console.WriteLine("RunSettings does not contain an embedded testSettings, not migrating.");


No need of localization?

smadala · 2018-05-17T12:09:00Z

src/SettingsMigrator/Resources/Resources.resx

+  </data>
+  <data name="ValidUsage" xml:space="preserve">
+    <value>Valid usage: SettingsMigrator.exe &lt;Full path to testsettings file or runsettings file to be migrated&gt; &lt;Full path to new runsettings file&gt;
+Example: SettingsMigrator.exe  E:\MyTest\MyTestSettings.testsettings  E:\MyTest\MyRunSettings.runsettings</value>


add runsettings example too.

smadala · 2018-05-17T12:21:52Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+                                    "<TestTypeSpecific>" +
+                                    "<UnitTestRunConfig testTypeId=\"13cdc9d9-ddb5-4fa4-a97d-d965ccfc6d4b\">" +
+                                    "<AssemblyResolution>" +
+                                    "<TestDirectory useLoadContext=\"true\" />" +


smadala · 2018-05-17T12:23:04Z

test/SettingsMigrator.UnitTests/SettingsMigrator.UnitTests.csproj

+  <PropertyGroup>
+    <OutputType Condition=" '$(TargetFramework)' != 'net451' ">Exe</OutputType>
+    <TargetFrameworks>net451</TargetFrameworks>
+    <AssemblyName>SettingsMigrator.UnitTests</AssemblyName>


Add code analysis. https://github.com/Microsoft/vstest/pull/1601/files#diff-22d6e2912c8e609e0d75d1e27de88591R12

smadala · 2018-05-17T12:23:18Z

test/SettingsMigrator.UnitTests/SettingsMigrator.UnitTests.csproj

+  <Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
+  <PropertyGroup>
+    <OutputType Condition=" '$(TargetFramework)' != 'net451' ">Exe</OutputType>
+    <TargetFrameworks>net451</TargetFrameworks>


This can be TargetFramework.

smadala · 2018-05-17T12:36:07Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+                                    "<AssemblyResolution>" +
+                                    "<TestDirectory useLoadContext=\"true\" />" +
+                                    "</AssemblyResolution>" +
+                                    "</UnitTestRunConfig>" +


Add test for parallelTestCount.

smadala · 2018-05-17T12:48:29Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+                                    "</MSTest>" +
+                                    "</RunSettings>";
+
+        const string OldTestSettings = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +


Create separate file for testsettings to runsettings .

smadala · 2018-05-17T12:50:58Z

src/SettingsMigrator/Migrator.cs

+            }
+
+            // Remove the embedded testSettings node if it exists.
+            RemoveEmbeddedTestSettings(runSettingsXmlDoc);


remove outside

smadala · 2018-05-17T12:53:59Z

src/SettingsMigrator/Migrator.cs

+            {
+                var newExecutionNode = newXmlDoc.CreateNode(XmlNodeType.Element, ExecutionNodeName, null);
+
+                if (string.IsNullOrEmpty(parallelTestCount))


smadala · 2018-05-17T13:04:05Z

src/SettingsMigrator/Program.cs

+        static void Main(string[] args)
+        {
+            if (args.Length != 2)
+            {


Add tests for this.

smadala · 2018-05-17T13:18:58Z

src/SettingsMigrator/App.config

+<?xml version="1.0" encoding="utf-8" ?>
+<configuration>
+    <startup> 
+        <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.6.1" />


Why is .NETFramework,Version=v4.6.1?

WE can remove app.config.

smadala · 2018-05-17T13:32:51Z

src/SettingsMigrator/Migrator.cs

+            }
+
+            // LegacySettings node.
+            if (testSettingsNodes.Deployment != null || testSettingsNodes.Script != null || testSettingsNodes.UnitTestConfig != null ||


Move if checks to inside function(AddLegacyNodes, AddRunTimeoutNode, AddDataCollectorNodes).

Move the long ones. Leaving the short ones since i find that more readable

smadala · 2018-05-17T13:39:24Z

src/SettingsMigrator/Migrator.cs

+                if (testSettingsNodes.Timeout != null && (testSettingsNodes.Timeout.Attributes[AgentNotRespondingTimeoutAttribute] != null ||
+                    testSettingsNodes.Timeout.Attributes[DeploymentTimeoutAttribute] != null || testSettingsNodes.Timeout.Attributes[ScriptTimeoutAttribute] != null))
+                {
+                    Console.WriteLine(string.Format(CultureInfo.CurrentCulture, CommandLineResources.UnsupportedAttributes));


Put the errors and warning in red and yellow color.

Show the exact setting which is not supported.

Modify string.

Not doing any color coding for now. Modified the string

smadala · 2018-05-17T13:42:46Z

src/SettingsMigrator/Resources/Resources.resx

+    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
+  </resheader>
+  <data name="IgnoringLegacySettings" xml:space="preserve">
+    <value>Any LegacySettings node already present in the runsettings will be removed.</value>


Make statement as removing. Show the content which is getting removed.

Not showing the content, We are specifying the node which is being removed

smadala · 2018-05-17T13:53:49Z

src/SettingsMigrator/Migrator.cs

+        {
+            var testSettingsNodes = new TestSettingsNodes();
+
+            using (XmlTextReader reader = new XmlTextReader(testSettingsPath))


Please share testsettings file with all possible configuration.

smadala · 2018-05-17T14:15:36Z

test/SettingsMigrator.UnitTests/SettingsMigrator.UnitTests.csproj

+      <FromP2P>true</FromP2P>
+    </ProjectReference>
+  </ItemGroup>
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">


All these Reference required?

smadala · 2018-05-17T14:17:27Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+    [TestClass]
+    public class MigratorUnitTests


Nit: MigratorTests

smadala · 2018-05-17T14:18:16Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+        [TestMethod]
+        public void MigratorGeneratesCorrectRunsettingsForEmbeddedTestSettings()
+        {
+            var migrator = new Migrator();


DRY: line 17 to 23.

Remove to common coded as much as needed

smadala · 2018-05-17T14:19:09Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+
+            using (XmlTextReader reader = new XmlTextReader(newRunsettingsPath))
+            {
+                reader.Namespaces = false;


DRY: Line number 36 to 64?

Remove to common coded as much as needed

smadala · 2018-05-17T14:21:05Z

src/SettingsMigrator/Migrator.cs

+                                                "<RunSettings></RunSettings>";
+        const string AgentNotRespondingTimeoutAttribute = "agentNotRespondingTimeout";
+        const string DeploymentTimeoutAttribute = "deploymentTimeout";
+        const string ScriptTimeoutAttribute = "scriptTimeout";


mayankbansal018 · 2018-05-17T14:27:19Z

src/SettingsMigrator/SettingsMigrator.csproj

+    <Prefer32Bit Condition="'$(TargetFramework)' == 'net451'">true</Prefer32Bit>
+    <ApplicationManifest>app.manifest</ApplicationManifest>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(TargetFramework)' != 'netcoreapp2.0'">


Please remove this, since we are not building this for netcore

mayankbansal018 · 2018-05-17T14:29:28Z

src/SettingsMigrator/Program.cs

+    /// <summary>
+    /// Entry point for SettingsMigrator.
+    /// </summary>
+    class Program


Please make it Public, if it doesn't contain code that needs to be tested, make it static as well

smadala · 2018-05-18T10:16:49Z

src/SettingsMigrator/App.config

+<?xml version="1.0" encoding="utf-8" ?>
+<configuration>
+    <startup> 
+        <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.6.1" />


WE can remove app.config.

smadala · 2018-05-18T10:34:35Z

src/SettingsMigrator/Migrator.cs

+                if (testSettingsNodes.Timeout != null && (testSettingsNodes.Timeout.Attributes[AgentNotRespondingTimeoutAttribute] != null ||
+                    testSettingsNodes.Timeout.Attributes[DeploymentTimeoutAttribute] != null || testSettingsNodes.Timeout.Attributes[ScriptTimeoutAttribute] != null))
+                {
+                    Console.WriteLine(string.Format(CultureInfo.CurrentCulture, CommandLineResources.UnsupportedAttributes));


Modify string.

smadala · 2018-05-18T10:39:44Z

src/SettingsMigrator/Program.cs

+
+            string oldFilePath = args[0];
+
+            if (!Path.IsPathRooted(oldFilePath))


@cltshivash Why should the path need to be rooted?

smadala · 2018-05-18T10:47:17Z

src/SettingsMigrator/Resources/Resources.Designer.cs

+        }
+
+        /// <summary>
+        ///   Looks up a localized string similar to Valid usage: SettingsMigrator.exe &lt;Full path to testsettings file or runsettings file to be migrated&gt; &lt;Full path to new runsettings file&gt;


Update this help.

smadala · 2018-05-18T10:49:57Z

test/SettingsMigrator.UnitTests/MigratorUnitTests.cs

+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+    [TestClass]
+    public class MigratorUnitTests


Change this to MigratorTests.

smadala · 2018-05-18T11:01:04Z

test/SettingsMigrator.UnitTests/SettingsMigrator.UnitTests.csproj

+	<Content Include="oldRunSettingsWithDataCollector.runsettings" CopyToOutputDirectory="Always" />
+  </ItemGroup>
+  <ItemGroup>
+    <Reference Include="System.Runtime" />


These reference not required.?

smadala · 2018-05-18T11:02:02Z

test/SettingsMigrator.UnitTests/oldRunSettings.runsettings

@@ -0,0 +1,6 @@
+<RunSettings>
+	<MSTest>


Keep embedded in file name.

smadala · 2018-05-18T11:04:10Z

test/SettingsMigrator.UnitTests/oldTestSettings.testsettings

+    <DeploymentItem filename=".\test.txt" />
+  </Deployment>
+  <Scripts setupScript=".\setup.bat" cleanupScript=".\cleanup.bat" />
+  <Execution parallelTestCount="2" hostProcessPlatform="MSIL">


Why hostProcessPlatform not migrating?

Will be done separately

smadala · 2018-05-18T11:08:22Z

test/SettingsMigrator.UnitTests/SettingsMigrator.UnitTests.csproj

+  <Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
+  <PropertyGroup>
+    <TargetFramework>net451</TargetFramework>
+    <AssemblyName>SettingsMigrator.UnitTests</AssemblyName>


Add code analysis.

smadala · 2018-05-18T11:13:15Z

src/SettingsMigrator/Program.cs

+        /// Main entry point. Hands off execution to Migrator.
+        /// </summary>
+        /// <param name="args">Arguments provided on the command line.</param>
+        public static void Main(string[] args)


Add tests for this.

kaadhina added 2 commits May 16, 2018 16:11

Migrator

f6d11dc

Adding project to sln

2188d5c

kaadhina requested a review from singhsarab May 16, 2018 10:46

singhsarab reviewed May 16, 2018

View reviewed changes

smadala changed the title ~~Users/kavipriya/settingsmigrator~~ Add a tool to migrate testsettings to runsettings May 16, 2018

Refactoring

6922920

singhsarab reviewed May 17, 2018

View reviewed changes

PR comments

dab18be

singhsarab requested review from smadala and mayankbansal018 May 17, 2018 08:56

singhsarab reviewed May 17, 2018

View reviewed changes

kaadhina added 3 commits May 17, 2018 14:39

More UTs

528f31a

Fixing message

29649c1

Fixing datacollector node

d67f77b

smadala suggested changes May 17, 2018

View reviewed changes

smadala reviewed May 17, 2018

View reviewed changes

mayankbansal018 reviewed May 17, 2018

View reviewed changes

kaadhina added 2 commits May 18, 2018 12:33

PR comments

2e1b318

Resx changes

acce98b

smadala reviewed May 18, 2018

View reviewed changes

kaadhina added 11 commits May 18, 2018 17:40

PR comments

3aaa2fb

Comments

645273c

PR comments

09226c5

Fix

4c5d7c3

Signproj

67f2cd3

Required reference

07a5ba7

Nuspec update

e38c75b

References

c7ce3c4

Fix reference

fc16f53

Fix references

204fa47

Refactor

b31a07a

smadala approved these changes May 23, 2018

View reviewed changes

kaadhina added 3 commits May 23, 2018 14:22

Merge branch 'master' into users/kavipriya/settingsmigrator

107c88d

Fixing file count

5d67152

Merge branch 'master' into users/kavipriya/settingsmigrator

acece85

singhsarab merged commit 30d840b into microsoft:master May 23, 2018

		@@ -0,0 +1,123 @@
		<?xml version="1.0" encoding="utf-8"?>
		<root>

		}


		public void MigrateTestSettings(string oldTestSettingsPath, string newRunSettingsPath)


		string oldFilePath = args[0];

		if (!Path.IsPathRooted(oldFilePath))

Add a tool to migrate testsettings to runsettings #1600

Add a tool to migrate testsettings to runsettings #1600

Conversation

kaadhina commented May 16, 2018

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab commented May 17, 2018

Choose a reason for hiding this comment

singhsarab commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab May 17, 2018 • edited Loading

Choose a reason for hiding this comment

smadala May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab May 16, 2018 •

edited

Loading

singhsarab May 17, 2018 •

edited

Loading

smadala May 17, 2018 •

edited

Loading