make testplatform compatible with old testhost #971

Faizan2304 · 2017-08-02T19:01:44Z

Issue

**Issue1: ** TestHost of version 15.0.0-preview throw exception if any unknown runsettings node (for example TestSessionTimeout is present in runsettings and it got stuck forever.

**issue2: ** If testhost throw exception before starting test run, it doesn't send any testruncomplete event or it doesn't abort process. As a result of it vstest.console keep waiting forever for testhost to send testruncomplete.

#970

Fix

Don't send these newly added runsettings (which are unknown to old testhosts ) to old test hosts.

smadala · 2017-08-03T05:15:22Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

@@ -103,6 +105,45 @@ public static void UpdateCollectSourceInformation(XPathNavigator runSettingsNavi
        }

        /// <summary>
+        /// Remove the <c>RunConfiguration.DesignMode</c> value for a run settings.
+        /// </summary>
+        public static void RemoveDesignMode(XPathNavigator runSettingsNavigator)


DRY: Can we have generalize method here, rather than one method for each element?

We cant, generalized method (RemoveNodeFromRunConfiguration) is getting called from this method. We cant generalize because in that case we have to pass full path(RunSettings/RunConfiguration/DesignMode) and name of node to delete it and we want to have those info in this class only.

Fixed in next commit

smadala · 2017-08-03T05:24:56Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs

+                            document.Load(reader);
+
+                            var navigator = document.CreateNavigator();
+


Rather than removing new ones, Can we just keep old ones?

Theoretically it seems right but i think it will be little big work.

smadala · 2017-08-03T05:26:09Z

src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs

@@ -87,6 +87,11 @@ public void Initialize(IEnumerable<string> pathToAdditionalExtensions)

                this.activeTestRun.RunTests();
            }
+            catch(Exception e)
+            {
+                this.testRunEventsHandler.HandleLogMessage(ObjectModel.Logging.TestMessageLevel.Error, e.Message);


Pass e.ToString(), Having stacktrace helps finding RC quickly.

Fixed in next commit.

smadala · 2017-08-03T05:32:44Z

test/Microsoft.TestPlatform.Utilities.UnitTests/InferRunSettingsHelperTests.cs

+        }
+
+        [TestMethod]
+        public void RemoveDesignModeShouldRemoveDesignModeNode()


These tests seems to be redundant. Didn't DataTestMethod covering the same functionality?

No DataTestMethod (RemoveNodeFromRunConfigurationShouldNotModifyXmlIfNavigatorIsNotAtRootNode) is covering the scenario where we are sending wrong navigator, that is its a negative test. RemoveDesignModeShouldRemoveDesignModeNode is positive test which are checking the actual functionality of function RemoveDesignMode

smadala · 2017-08-03T13:35:13Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                        var listOfInValidRunConfigurationSettings = new List<string>();
+
+                        // These are the list of valid RunConfiguration setting name which old testhost understand.
+                        var listOfValidRunConfigurationSettings = new List<string>


Use Set here.

Fixed in next commit

smadala · 2017-08-03T13:41:33Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                                string settingsName = string.Join(", ", listOfInValidRunConfigurationSettings);
+                                EqtTrace.Warning(string.Format("InferRunSettingsHelper.MakeRunsettingsCompatible: Removing the following settings: {0} from RunSettings file. To use those settings please move to latest version of Microsoft.NET.Test.Sdk", settingsName));
+                            }
+


Nit: remove additional empty lines.

codito · 2017-08-03T14:16:23Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs

@@ -110,6 +110,11 @@ public DotnetTestHostManager()
        internal virtual bool IsVersionCheckRequired => !this.hostPackageVersion.StartsWith("15.0.0");

        /// <summary>
+        /// Gets a value indicating whether the test host supports protocol version check
+        /// </summary>
+        internal virtual bool TestHostCannotHandleNewRunSettingsNode => this.hostPackageVersion.StartsWith("15.0.0-preview");


Should this be virtual?

No, no need, i will remove virtual in next commit.

codito · 2017-08-03T14:17:18Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs

+        {
+            var updatedRunSettingsXml = runsettingsXml;
+
+            var property = this.testHostManager.GetType().GetRuntimeProperties().FirstOrDefault(p => string.Equals(p.Name, oldTestHostPropertyName, StringComparison.OrdinalIgnoreCase));


Is it okay to run this for all test host managers?

Yes, we have to. There is no way to know dotnetTesthostManager.

codito · 2017-08-03T14:18:57Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+
+                        } while (runSettingsNavigator.MoveToNext());
+
+                        // Delele all invalid RunConfiguration Settings


s/Delele/Delete

codito · 2017-08-03T14:19:39Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                        var listOfInValidRunConfigurationSettings = new List<string>();
+
+                        // These are the list of valid RunConfiguration setting name which old testhost understand.
+                        var listOfValidRunConfigurationSettings = new HashSet<string>


Is this a constant set?

codito · 2017-08-03T14:21:01Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

+                        {
+                            if(!listOfValidRunConfigurationSettings.Contains(runSettingsNavigator.LocalName))
+                            {
+                                listOfInValidRunConfigurationSettings.Add(runSettingsNavigator.LocalName);


Why not remove the node here directly?

Its because when we delete a node, navigator jumps to parent and we lost track of last visited child.

codito · 2017-08-03T14:26:37Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs

@@ -121,11 +122,13 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsH
                        isDebug: (testRunCriteria.TestHostLauncher != null && testRunCriteria.TestHostLauncher.IsDebug),
                        testCaseFilter: testRunCriteria.TestCaseFilter);

+                    // This is workaround for the bug https://github.com/Microsoft/vstest/issues/970
+                    var runsettings = this.RemoveNodesFromRunsettingsIfRequired(testRunCriteria.TestRunSettings);


Do we need unit and integration tests for this?

Added acceptance test for this.

codito · 2017-08-03T14:27:00Z

src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs

@@ -87,6 +87,11 @@ public void Initialize(IEnumerable<string> pathToAdditionalExtensions)

                this.activeTestRun.RunTests();
            }
+            catch(Exception e)


Do we need unit tests for this?

Yes :). Added in next commit.

codito · 2017-08-04T12:06:20Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/ExecutionManagerTests.cs

+        [TestMethod]
+        public void StartTestRunShouldAbortTheRunIfAnyExceptionComesForTheProvidedTests()
+        {
+            var testExecutionContext = new TestExecutionContext(


Can we remove DRY violation and make this an instance variable?

codito · 2017-08-04T12:07:41Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/ExecutionManagerTests.cs

+            var mockTestRunEventsHandler = new Mock<ITestRunEventsHandler>();
+
+            // Call StartTestRun with faulty runsettings so that it will throw exception
+            this.executionManager.StartTestRun(new Dictionary<string, IEnumerable<string>>(), @"<RunSettings><RunConfiguration><TestSessionTimeout>0</TestSessionTimeout></RunConfiguration></RunSettings>", testExecutionContext, null, mockTestRunEventsHandler.Object);


StartTestRunWithTests?

codito · 2017-08-04T12:11:24Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs

+        /// </summary>
+        /// <param name="runsettingsXml">runsettings string</param>
+        /// <returns>runsetting after removing unrequired nodes</returns>
+        protected string RemoveNodesFromRunsettingsIfRequired(string runsettingsXml, ITestRunEventsHandler eventHandler)


RemoveNodesFromRunsettingsIfRequired(string runsettingsXml, Action<TestMessageLevel, string> logMessage)

Refactor in ProxyExecutionManager to provide this function. Exception handling in PEM can use the same function.

fix for issue microsoft#970

5140273

Faizan2304 requested review from codito, singhsarab and mayankbansal018 August 2, 2017 19:01

msftclas added the cla-not-required label Aug 2, 2017

Merge remote-tracking branch 'upstream/master'

048f3e3

smadala reviewed Aug 3, 2017

View reviewed changes

faahmad added 5 commits August 3, 2017 12:53

Address PR comment

77cdf1b

Addressed Pr comment

adc3fdd

Merge remote-tracking branch 'upstream/master'

8156616

Merge branch 'master' into issue970

a608cc3

Remove unused code

636a3bf

smadala approved these changes Aug 3, 2017

View reviewed changes

Address PR comment

9237cfc

codito suggested changes Aug 3, 2017

View reviewed changes

faahmad added 3 commits August 4, 2017 16:31

Addressed PR comment

50576e9

Merge remote-tracking branch 'upstream/master'

1443b0b

Merge branch 'master' into issue970

fbaee11

codito approved these changes Aug 4, 2017

View reviewed changes

faahmad added 2 commits August 4, 2017 18:55

Addressed PR comment

509be7c

Code cleanup

0635942

Faizan2304 merged commit 5dc8ff1 into microsoft:master Aug 4, 2017

Faizan2304 deleted the issue970 branch August 4, 2017 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make testplatform compatible with old testhost #971

make testplatform compatible with old testhost #971

Faizan2304 commented Aug 2, 2017 •

edited

Loading

smadala Aug 3, 2017

Faizan2304 Aug 3, 2017

Faizan2304 Aug 3, 2017

smadala Aug 3, 2017

Faizan2304 Aug 3, 2017

smadala Aug 3, 2017

Faizan2304 Aug 3, 2017

smadala Aug 3, 2017

Faizan2304 Aug 3, 2017

smadala Aug 3, 2017

Faizan2304 Aug 3, 2017

smadala Aug 3, 2017

Faizan2304 Aug 3, 2017

codito Aug 3, 2017

Faizan2304 Aug 4, 2017

codito Aug 3, 2017

Faizan2304 Aug 4, 2017 •

edited

Loading

codito Aug 3, 2017

Faizan2304 Aug 4, 2017

codito Aug 3, 2017

Faizan2304 Aug 4, 2017

codito Aug 3, 2017

Faizan2304 Aug 4, 2017

codito Aug 3, 2017

Faizan2304 Aug 4, 2017

codito Aug 3, 2017

Faizan2304 Aug 4, 2017

codito Aug 4, 2017

Faizan2304 Aug 4, 2017

codito Aug 4, 2017

Faizan2304 Aug 4, 2017

codito Aug 4, 2017

Faizan2304 Aug 4, 2017

		document.Load(reader);

		var navigator = document.CreateNavigator();


		} while (runSettingsNavigator.MoveToNext());

		// Delele all invalid RunConfiguration Settings

make testplatform compatible with old testhost #971

make testplatform compatible with old testhost #971

Conversation

Faizan2304 commented Aug 2, 2017 • edited Loading

Issue

Fix

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

Faizan2304 Aug 4, 2017 • 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

Faizan2304 commented Aug 2, 2017 •

edited

Loading

Faizan2304 Aug 4, 2017 •

edited

Loading