Test runtime provider Interface changes #593

mayankbansal018 · 2017-03-10T13:18:45Z

Passing runsettings as xml

to analyze settings themselves

msftclas · 2017-03-10T13:18:49Z

@mayankbansal018,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

singhsarab · 2017-03-11T17:30:32Z

src/Microsoft.TestPlatform.CrossPlatEngine/Hosting/DefaultTestHostManager.cs

@@ -220,9 +221,10 @@ public IEnumerable<string> GetTestPlatformExtensions(IEnumerable<string> sources
            return Enumerable.Empty<string>();
        }

-        public bool CanExecuteCurrentRunConfiguration(RunConfiguration runConfiguration)
+        public bool CanExecuteCurrentRunConfiguration(string runConfiguration)


Nit:
Add XML comments for public methods.
Should we rename the arg to settingsXml just to make it intuitive ?
Prefer using var instead.

singhsarab · 2017-03-11T17:31:29Z

src/Microsoft.TestPlatform.Common/Hosting/TestRunTimeProviderManager.cs

@@ -73,7 +73,7 @@ public ITestRuntimeProvider GetTestHostManagerByUri(string hostUri)
            return null;
        }

-        public ITestRuntimeProvider GetTestHostManagerByRunConfiguration(RunConfiguration runConfiguration)
+        public ITestRuntimeProvider GetTestHostManagerByRunConfiguration(string runConfiguration)


Rationale behind this change ?

We want the implementer of ITestRuntimeProvider to have the full settings so they can decide on what ever parameters they want to, on whether or not can they Host test for this session.
This also allows them to add custom sections in runsettings, & process them here, rather than having the TP understand and process them.

nit: rename parameter as runsettings.

codito · 2017-03-16T11:30:03Z

src/Microsoft.TestPlatform.ObjectModel/TestProcessStartInfo.cs

-        /// <summary>
-        /// Callback on process exit
-        /// </summary>
-        public Action<Process, string> ErrorReceivedCallback { get; set; }


codito · 2017-03-16T11:30:26Z

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

@@ -155,7 +155,7 @@ public void SetupChannelShouldAddExitCallbackToTestHostStartInfo()

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

-            Assert.IsNotNull(startInfo.ErrorReceivedCallback);
+            //Assert.IsNotNull(startInfo.ErrorReceivedCallback);


Remove this?

smadala · 2017-03-16T14:26:44Z

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

@@ -64,7 +65,7 @@ public class DotnetTestHostManager : ITestRuntimeProvider
        /// </summary>
        private Action<Process, string> ErrorReceivedCallback => ((process, data) =>
        {
-            if (data != null)
+            if (!string.IsNullOrEmpty(data))
            {
                // if incoming data stream is huge empty entire testError stream, & limit data stream to MaxCapacity
                if (data.Length > this.testHostProcessStdError.MaxCapacity)


please avoid logging multiple times, this.messageLogger.SendMessage in *TestHostManager.cs.

mayankbansal018 · 2017-03-16T16:04:59Z

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

@@ -117,6 +118,8 @@ public DotnetTestHostManager()
            this.processHelper = processHelper;
            this.fileHelper = fileHelper;
            this.dotnetHostHelper = dotnetHostHelper;
+
+            (this.testHostLauncher as DefaultTestHostLauncher)?.SetErrorCallBack(this.ErrorReceivedCallback);


I don't like this, but to break errorCallBack action from TestProcessStartInfo, this seemed one solution.
Another possible solution would have been to make it part of ITestHostLauncher, which then passes it down to IProcessHelper.

If this doesn't look good at all, let me know, will not push this in.

DotNetTestHost Code Refactor(Removed ITestHostlauncher)

codito · 2017-03-17T11:33:13Z

src/Microsoft.TestPlatform.CrossPlatEngine/Helpers/Interfaces/IProcessHelper.cs

+        /// checks if the process has exited, & returns exitcode.
+        /// </summary>
+        /// <param name="process">process</param>
+        /// <returns>process exited</returns>


False if process has not exited, True otherwise. Set exitCode only if process has exited.

codito · 2017-03-17T11:38:53Z

src/Microsoft.TestPlatform.CrossPlatEngine/Helpers/ProcessHelper.cs

+        private void ProcessExited(Process process, EventArgs e)
+        {
+            // wait for process to flush out error, & out streams
+            process.WaitForExit();


Please check if this can lead to a deadlock.

Exited event is only raised once, so this should be safe

codito · 2017-03-17T11:42:11Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Helpers/ProcessHelperTests.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 TestPlatform.CrossPlatEngine.UnitTests


Is this intentional?

Yes, all the test in ProcessHelperTest were earlier written to verify ErrorCallBack method, since then it has been moved to individual Runtime providers

Delete it please in that case.

codito · 2017-03-17T11:43:48Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Hosting/DotnetTestHostManagerTests.cs

+            this.defaultTestProcessStartInfo = this.dotnetHostManager.GetTestHostProcessStartInfo(new[] { defaultSourcePath }, null, this.defaultConnectionInfo);
+        }
+
+        public void ErrorCallBackTestHelper(string errorMessage, int exitCode)


Should be private

codito · 2017-03-17T11:44:04Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Hosting/DotnetTestHostManagerTests.cs


            this.mockTestHostLauncher.Verify(thl => thl.LaunchTestHost(It.Is<TestProcessStartInfo>(x => x.Arguments.Equals(expectedArgs))), Times.Once);
        }
-
+        


nit: remove

codito · 2017-03-17T11:45:59Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Hosting/DotnetTestHostManagerTests.cs

+        }
+
+        [TestMethod]
+        public async Task DotNetCoreNoErrorMessageIfDataIsEmptyAsync()


Merge with above test case. Use DataTestMethod with DataRow.

codito · 2017-03-17T11:50:40Z

src/Microsoft.TestPlatform.CrossPlatEngine/Hosting/DefaultTestHostManager.cs

            }
-            catch (OperationCanceledException ex)
+
+            if (this.processHelper.TryGetExitCode(process, out exitCode))


Missing scenario: process doesn't write anything on stderr and exits -> HostExited should be called.

codito · 2017-03-17T11:52:16Z

src/Microsoft.TestPlatform.CrossPlatEngine/Hosting/DefaultTestHostManager.cs

@@ -108,64 +68,67 @@ internal DefaultTestHostManager(Architecture architecture, Framework framework,
            this.Shared = shared;
        }

+        public event EventHandler<HostProviderEventArgs> HostLaunched;


Document that HostLaunched will not be called for custom launchers.

This should be documented as part of design doc right?

I'd suggest to put it in code. It will be available in intellisense.

Added required test cases

mayankbansal018 · 2017-03-20T09:28:45Z

@dotnet-bot Test this please

Passing runsettings as xml for test runtime providers,

8980b89

to analyze settings themselves

mayankbansal018 self-assigned this Mar 10, 2017

mayankbansal018 requested a review from codito March 10, 2017 13:18

msftclas added the cla-not-required label Mar 10, 2017

Merge branch 'master' into hostextensibilty-framework

4a97485

singhsarab reviewed Mar 11, 2017

View reviewed changes

mayankbansal018 added 2 commits March 15, 2017 18:34

Merge branch 'master' into hostextensibilty-framework

60e78fa

Clean up for ITestRunTimeProvider

57d40a9

codito reviewed Mar 16, 2017

View reviewed changes

smadala reviewed Mar 16, 2017

View reviewed changes

mayankbansal018 added 2 commits March 16, 2017 20:43

Fixing test Default, DotNet test host providers

84cec8d

Build error

1401de2

mayankbansal018 commented Mar 16, 2017

View reviewed changes

mayankbansal018 added 2 commits March 17, 2017 15:02

Test Correction

4319ca2

DotNetTestHost Code Refactor(Removed ITestHostlauncher)

Resharper Refactoring

daeb211

codito reviewed Mar 17, 2017

View reviewed changes

mayankbansal018 added 3 commits March 17, 2017 18:09

PR Comments

940b4ac

build error

29d0122

Documentation for methods corrected

b985a0a

Added required test cases

codito approved these changes Mar 20, 2017

View reviewed changes

Clean up API's

927eb4f

mayankbansal018 merged commit d189e71 into microsoft:master Mar 20, 2017

mayankbansal018 deleted the hostextensibilty-framework branch March 20, 2017 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test runtime provider Interface changes #593

Test runtime provider Interface changes #593

mayankbansal018 commented Mar 10, 2017

msftclas commented Mar 10, 2017

singhsarab Mar 11, 2017

singhsarab Mar 11, 2017

mayankbansal018 Mar 15, 2017

codito Mar 16, 2017

codito Mar 16, 2017

codito Mar 16, 2017

smadala Mar 16, 2017

mayankbansal018 Mar 16, 2017

codito Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 Mar 17, 2017

codito Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 Mar 17, 2017

codito Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 Mar 17, 2017

codito Mar 17, 2017

mayankbansal018 commented Mar 20, 2017


		this.mockTestHostLauncher.Verify(thl => thl.LaunchTestHost(It.Is<TestProcessStartInfo>(x => x.Arguments.Equals(expectedArgs))), Times.Once);
		}

Test runtime provider Interface changes #593

Test runtime provider Interface changes #593

Conversation

mayankbansal018 commented Mar 10, 2017

msftclas commented Mar 10, 2017

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

mayankbansal018 commented Mar 20, 2017