Make all communication timeouts configurable. #1538

smadala · 2018-04-09T15:35:18Z

Description

There are various timeout error on slow machines. This PR address to make all the timeout
configurable.
Refactor DefaultEngineInvoker.cs and Datacollector Program.cs to make them testable.

Related issue

Exxample output

PS C:\Users\samadala\src\vstest> $env:VSTEST_CONNECTION_TIMEOUT=1
PS C:\Users\samadala\src\vstest> C:\Users\samadala\src\vstest\artifacts\Debug\net451\win7-x64\vstest.console.exe C:\Users\samadala\src\vstest\test\TestAssets\SimpleTestProject\bin\Debug\net451\SimpleTestProject.dll /Inisolation
Microsoft (R) Test Execution Command Line Tool Version 15.7.0-dev
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: vstest.console process failed to connect to testhost process after 1 seconds. This may occur due to machine slowness, please set environment variable VSTEST_CONNECTION_TIMEOUT to increase timeout.
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.HandleConnectionFailure(Boolean connected, Int32 connTimeout)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources, CancellationToken cancellationToken)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyExecutionManager.StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsHandler eventHandler)

singhsarab · 2018-04-11T14:33:07Z

src/Microsoft.TestPlatform.PlatformAbstractions/net451/System/PlatformEnvironment.cs

+            }
+
+            return isValueSet;
+        }


Not required in Abstraction.
For testing, either have EnvironmentHelper or directly use it in the tests.

singhsarab · 2018-04-11T14:50:07Z

src/Microsoft.TestPlatform.ObjectModel/Constants.cs

+        /// <summary>
+        /// Environment variable name to increase communication timeouts between processes.
+        /// </summary>
+        public const string VstestTimeoutIncreaseByTimes = "VSTEST_TIMEOUT_INCREASE_BY_TIMES";


VSTEST_TIMEOUT_INCREASE_BY_TIMES [](start = 60, length = 32)

How about VSTEST_CONNECTION_TIMEOUT?
We should have a simple settings, also make it consistent across

Makes sense, Done.

singhsarab · 2018-04-11T14:56:17Z

src/datacollector/Resources/xlf/Resources.cs.xlf

+    <body>
+      <trans-unit id="DataCollectorFailedToConnetToVSTestConsole">
+        <source>Datacollector process failed to connect to vstest.console in {0} milliseconds, Set enviroment variable {1} to increase timeout.</source>
+        <target state="new">Datacollector process failed to connect to vstest.console in {0} milliseconds, Set enviroment variable {1} to increase timeout.</target>


Datacollector process failed to connect to vstest.console in {0} milliseconds [](start = 28, length = 77)

Datacollector process failed to connect to vstest.console in {0} milliseconds. Please set environment variable {1} to increase the connection timeout.

Specify time in Seconds, & take input in seconds

@singhsarab @mayankbansal018 Agreed. Done.

mayankbansal018 · 2018-04-16T12:33:49Z

src/datacollector/DataCollectionMain.cs

@@ -0,0 +1,155 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


Why did we create a new class?

For testing refactored it.

mayankbansal018 · 2018-04-17T09:00:21Z

src/Microsoft.TestPlatform.CoreUtilities/Helpers/EnvironmentHelper.cs

@@ -0,0 +1,32 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


do we want to add more in CoreUtilities dll? can we move this to Testplatform.Utilities?

CoreUtilities have basic utilities on BCL. TestPlatform.Utilities has OM + BCL Utilities.

mayankbansal018 · 2018-04-17T09:01:29Z

src/Microsoft.TestPlatform.CoreUtilities/Helpers/EnvironmentHelper.cs

+    using System;
+    using ObjectModel;
+
+    public class EnvironmentHelper


there is an IEnvironment Interface in platform abstractions, should it not move there?

We will go for PlatformAbstration if there is need to do #if defs. This function not required that.

mayankbansal018 · 2018-04-17T09:09:54Z

src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/ProxyDataCollectionManager.cs

@@ -207,20 +206,22 @@ public void Initialize()
            this.dataCollectionProcessId = this.dataCollectionLauncher.LaunchDataCollector(null, this.GetCommandLineArguments(this.dataCollectionPort));
            EqtTrace.Info("ProxyDataCollectionManager.Initialize: Launched datacollector processId: {0} port: {1}", this.dataCollectionProcessId, this.dataCollectionPort);

-            ChangeConnectionTimeoutIfRequired(dataCollectionProcessId);
+            var connectionTimeout = GetConnectionTimeout(dataCollectionProcessId);


nit: please use this, just to follow current coding convention

singhsarab · 2018-04-17T09:15:54Z

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

-        /// <param name="clientConnectionTimeout">Client Connection Timeout.</param>
-        protected ProxyOperationManager(IRequestData requestData, ITestRequestSender requestSender, ITestRuntimeProvider testHostManager, int clientConnectionTimeout)
+        /// <param name="processHelper">IProcessHelper implementation. </param>
+        internal ProxyOperationManager(IRequestData requestData,


remove this.

singhsarab · 2018-04-17T09:31:08Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs


            this.mockRequestSender.Setup(rs => rs.WaitForRequestHandlerConnection(100000)).Returns(true);
            this.testOperationManager.SetupChannel(Enumerable.Empty<string>(), CancellationToken.None);

            this.mockRequestSender.Verify(rs => rs.WaitForRequestHandlerConnection(100000), Times.Exactly(1));

-            this.connectionTimeout = 400;
+            this.connectionTimeout = EnvironmentHelper.DefaultConnectionTimeout;


remove this

singhsarab · 2018-04-17T09:32:32Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs


            this.testOperationManager.SetupChannel(Enumerable.Empty<string>(), CancellationToken.None);

-            this.mockRequestSender.Verify(rs => rs.WaitForRequestHandlerConnection(this.connectionTimeout), Times.Exactly(1));
+            this.mockRequestSender.Verify(rs => rs.WaitForRequestHandlerConnection(this.connectionTimeout));


revert this exactly change

singhsarab · 2018-04-17T09:35:23Z

...oft.TestPlatform.CrossPlatEngine.UnitTests/DataCollection/ProxyDataCollectionManagerTests.cs

            this.mockDataCollectionRequestSender.Setup(x => x.WaitForRequestHandlerConnection(timeout * 1000)).Returns(true);

            this.proxyDataCollectionManager.Initialize();
-            Environment.SetEnvironmentVariable(ProxyDataCollectionManager.TimeoutEnvironmentVaribleName, string.Empty);
+            Environment.SetEnvironmentVariable(EnvironmentHelper.VstestConnectionTimeout, string.Empty);


singhsarab · 2018-04-17T09:36:53Z

test/datacollector.UnitTests/DataCollectorMainTests.cs

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


singhsarab · 2018-04-17T09:37:59Z

test/datacollector.UnitTests/DataCollectorMainTests.cs

+            var message = Assert.ThrowsException<TestPlatformException>(() => this.dataCollectorMain.Run(args)).Message;
+            Assert.AreEqual(message,
+                string.Format(
+                    TestPlatform.DataCollector.Resources .Resources.DataCollectorFailedToConnetToVSTestConsole,


Nit: Connect*

singhsarab · 2018-04-17T09:38:46Z

test/datacollector.UnitTests/DataCollectorMainTests.cs

+        private Mock<IEnvironment> mockEnvironment;
+        private Mock<IDataCollectionRequestHandler> mockDataCollectionRequestHandler;
+        private DataCollectorMain dataCollectorMain;
+


extra line*

smadala · 2018-04-18T06:41:22Z

@singhsarab @mayankbansal018 Addressed all the comments. Please have a look once more.

mayankbansal018 · 2018-04-18T07:01:47Z

src/testhost.x86/DefaultEngineInvoker.cs

+            this.SetParentProcessExitCallback(argsDictionary);
+
+            this.requestHandler.ConnectionInfo =
+                DefaultEngineInvoker.GetConnectionInfo(out var endpoint, argsDictionary);


endpoint [](start = 63, length = 8)

remove endpoint, move logging in GetConnectionInfo()

mayankbansal018 · 2018-04-18T07:21:44Z

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

-                if (!this.testHostLaunched || !this.RequestSender.WaitForRequestHandlerConnection(connTimeout))
-                {
-                    var errorMsg = CrossPlatEngineResources.InitializationFailed;
+                var connected = true;


We could repurpose this.testHostLaunched to check for connectivity error

mayankbansal018 · 2018-04-18T07:24:13Z

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

+            {
+                EqtTrace.Info("TestHostManagerCallbacks.ExitCallBack: Testhost processId: {0} exited with exitcode: 0 error: '{1}'", procId, testHostProcessStdErrorStr);
+            }
+
            onHostExited(new HostProviderEventArgs(testHostProcessStdErrorStr, exitCode, procId));


testHostProcessStdErrorStr [](start = 51, length = 26)

remove if check simply log exitcode

If exit code is non-zero we are writing to error level.

mayankbansal018 · 2018-04-18T07:37:57Z

src/Microsoft.TestPlatform.CoreUtilities/Helpers/EnvironmentHelper.cs

+        public static int GetConnectionTimeout()
+        {
+            var envVarValue = Environment.GetEnvironmentVariable(EnvironmentHelper.VstestConnectionTimeout);
+            if (!string.IsNullOrEmpty(envVarValue) && int.TryParse(envVarValue, out int value))


if user sets a very small value say 10, should we honor it? I would say we keep 90 or 60 something as min, & users value will only be honored if it is > min val
More over here we are not even checking for -ve values

Handled -ve case. Keep min value 0, giving more choice to user.

…om/Microsoft/vstest into increase-timeout-for-version-check

singhsarab · 2018-04-18T09:35:41Z

test/testhost.UnitTests/DefaultEngineInvokerTests.cs

+                    CoreUtilitiesConstants.TesthostProcessName,
+                    CoreUtilitiesConstants.DatacollectorProcessName,
+                    EnvironmentHelper.DefaultConnectionTimeout,
+                    EnvironmentHelper.VstestConnectionTimeout));


We should have exact string match in tests.
If someone changes string in resources, these tests should catch that.

smadala added 5 commits April 5, 2018 00:19

Increase timeout for protocol version

9f6651a

Increase timeout for data collector process connection wait

75580ea

Add GetEnviromentVariable() to PlatformEnviroment

defe218

Make timeout uses VstestTimeoutIncreaseByTimes env var

119e272

Add datacollector resources

e5cbb02

smadala changed the title ~~[WIP] Make all communication timeout configurable.~~ Make all communication timeout configurable. Apr 11, 2018

smadala requested review from mayankbansal018, singhsarab and cltshivash April 11, 2018 14:25

singhsarab reviewed Apr 11, 2018

View reviewed changes

Use common connection timeout everywhere

83703fb

mayankbansal018 reviewed Apr 16, 2018

View reviewed changes

Update resource string and add tests

bbf787b

smadala changed the title ~~Make all communication timeout configurable.~~ Make all communication timeouts configurable. Apr 17, 2018

mayankbansal018 reviewed Apr 17, 2018

View reviewed changes

singhsarab reviewed Apr 17, 2018

View reviewed changes

smadala added 4 commits April 17, 2018 21:23

Make timeout config b/w datacollector and testhost

da303f6

Fix nits

7389af1

Update xlf files

be20f85

Fix TestRequesrHandlerTests

daf85b1

smadala added 2 commits April 18, 2018 12:06

Fix nits

018cbaa

Merge branch 'master' into increase-timeout-for-version-check

6bf095c

mayankbansal018 reviewed Apr 18, 2018

View reviewed changes

smadala added 2 commits April 18, 2018 14:23

Add tests for EnvironmentHelper

c85b80b

Merge branch 'increase-timeout-for-version-check' of https://github.c…

4244707

…om/Microsoft/vstest into increase-timeout-for-version-check

singhsarab reviewed Apr 18, 2018

View reviewed changes

smadala added 2 commits April 18, 2018 15:54

Fix Blame datacollector acceptance test failures

e0ef0d1

Don't use constants in tests directly

91d5c45

mayankbansal018 approved these changes Apr 18, 2018

View reviewed changes

smadala merged commit 1dc3cad into master Apr 18, 2018

smadala mentioned this pull request Apr 18, 2018

VSTest.console.exe fails due to connection timeout. #1544

Closed

smadala deleted the increase-timeout-for-version-check branch April 18, 2018 13:33

martincostello mentioned this pull request May 13, 2018

.NET Core 2.1 RC1 test runner hangs after tests complete but before results are reported #1586

Closed

smadala mentioned this pull request Jul 4, 2018

Make codecoverage.exe running event timeout configurable #1670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make all communication timeouts configurable. #1538

Make all communication timeouts configurable. #1538

smadala commented Apr 9, 2018 •

edited

Loading

singhsarab Apr 11, 2018 •

edited

Loading

smadala Apr 17, 2018

singhsarab Apr 11, 2018

smadala Apr 17, 2018

singhsarab Apr 11, 2018

mayankbansal018 Apr 16, 2018

smadala Apr 17, 2018

mayankbansal018 Apr 16, 2018

smadala Apr 17, 2018

mayankbansal018 Apr 17, 2018

smadala Apr 18, 2018

mayankbansal018 Apr 17, 2018

smadala Apr 18, 2018

mayankbansal018 Apr 17, 2018

singhsarab Apr 17, 2018

singhsarab Apr 17, 2018

singhsarab Apr 17, 2018

singhsarab Apr 17, 2018

singhsarab Apr 17, 2018

singhsarab Apr 17, 2018

singhsarab Apr 17, 2018

smadala commented Apr 18, 2018

mayankbansal018 Apr 18, 2018

smadala Apr 18, 2018

mayankbansal018 Apr 18, 2018

smadala Apr 18, 2018

mayankbansal018 Apr 18, 2018

smadala Apr 18, 2018

mayankbansal018 Apr 18, 2018

smadala Apr 18, 2018 •

edited

Loading

singhsarab Apr 18, 2018

		@@ -0,0 +1,155 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

		@@ -0,0 +1,32 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

Make all communication timeouts configurable. #1538

Make all communication timeouts configurable. #1538

Conversation

smadala commented Apr 9, 2018 • edited Loading

Description

Related issue

singhsarab Apr 11, 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

smadala commented Apr 18, 2018

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

smadala Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smadala commented Apr 9, 2018 •

edited

Loading

singhsarab Apr 11, 2018 •

edited

Loading

smadala Apr 18, 2018 •

edited

Loading