From 80f35da18a72a7e91317abbe64a105876b2fdfed Mon Sep 17 00:00:00 2001 From: samadala Date: Thu, 4 Jan 2018 12:06:11 +0530 Subject: [PATCH 1/5] Add exit callback for custom testhost launcher --- .../TestRequestSender.cs | 4 ++-- .../Interfaces/System/IProcessHelper.cs | 2 ++ .../common/System/ProcessHelper.cs | 12 ++++++++++++ .../netstandard1.0/System/ProcessHelper.cs | 6 ++++++ .../uap10.0/System/ProcessHelper.cs | 5 +++++ .../Hosting/DefaultTestHostManager.cs | 1 + 6 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs index deae160a5c..4b975a15c8 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs @@ -313,8 +313,8 @@ public void OnClientProcessExit(string stdError) this.clientExitErrorMessage = stdError; this.clientExited.Set(); - // Note that we're not explicitly disconnecting the communication channel; wait for the - // channel to determine the disconnection on its own. + // Break communication loop. + this.communicationEndpoint.Stop(); } /// diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs index eca17c7099..e1c01ab5d7 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs @@ -97,5 +97,7 @@ public interface IProcessHelper /// /// Reference of process to terminate. void TerminateProcess(object process); + + void SetExitCallback(int processId, Action exitCallBack); } } diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index ec924ed47b..7fca335073 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -142,6 +142,18 @@ public void SetExitCallback(int processId, Action callbackAction) }; } + /// + public void SetExitCallback(int processId, Action callbackAction) + { + var process = Process.GetProcessById(processId); + + process.EnableRaisingEvents = true; + process.Exited += (sender, args) => + { + callbackAction.Invoke(process); + }; + } + /// public void TerminateProcess(object process) { diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs index 7cd2ca470c..6bf7badc3d 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs @@ -67,6 +67,12 @@ public void SetExitCallback(int parentProcessId, Action callbackAction) throw new NotImplementedException(); } + /// + public void SetExitCallback(int parentProcessId, Action callbackAction) + { + throw new NotImplementedException(); + } + /// public void TerminateProcess(object process) { diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs index 69525fa74a..7248e708cb 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs @@ -66,6 +66,11 @@ public void SetExitCallback(int parentProcessId, Action callbackAction) { } + /// + public void SetExitCallback(int parentProcessId, Action callbackAction) + { + } + /// public void TerminateProcess(object process) { diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs index 8c25a98106..d758197d69 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs +++ b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs @@ -385,6 +385,7 @@ private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToke { int processId = this.customTestHostLauncher.LaunchTestHost(testHostStartInfo); this.testHostProcess = Process.GetProcessById(processId); + this.processHelper.SetExitCallback(processId, this.ExitCallBack); } } catch (OperationCanceledException ex) From 039bf33741c55828c3e042a5429f3c89463a297b Mon Sep 17 00:00:00 2001 From: samadala Date: Thu, 4 Jan 2018 15:16:23 +0530 Subject: [PATCH 2/5] Refactor the setexitcall --- .../Interfaces/System/IProcessHelper.cs | 4 +-- .../common/System/ProcessHelper.cs | 27 +++++++------------ .../netstandard1.0/System/ProcessHelper.cs | 6 ----- .../uap10.0/System/ProcessHelper.cs | 5 ---- .../Hosting/DotnetTestHostManager.cs | 1 + src/datacollector/Program.cs | 2 +- src/testhost.x86/DefaultEngineInvoker.cs | 2 +- .../Processors/PortArgumentProcessor.cs | 2 +- .../Processors/PortArgumentProcessorTests.cs | 2 +- 9 files changed, 15 insertions(+), 36 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs index e1c01ab5d7..f8a9826629 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs @@ -90,14 +90,12 @@ public interface IProcessHelper /// /// Callback on process exit. /// - void SetExitCallback(int processId, Action callbackAction); + void SetExitCallback(int processId, Action callbackAction); /// /// Terminates a process. /// /// Reference of process to terminate. void TerminateProcess(object process); - - void SetExitCallback(int processId, Action exitCallBack); } } diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 7fca335073..b479245ca3 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -130,28 +130,19 @@ public bool TryGetExitCode(object process, out int exitCode) return false; } - /// - public void SetExitCallback(int processId, Action callbackAction) - { - var process = Process.GetProcessById(processId); - - process.EnableRaisingEvents = true; - process.Exited += (sender, args) => - { - callbackAction.Invoke(); - }; - } - /// public void SetExitCallback(int processId, Action callbackAction) { - var process = Process.GetProcessById(processId); - - process.EnableRaisingEvents = true; - process.Exited += (sender, args) => + try + { + var process = Process.GetProcessById(processId); + process.EnableRaisingEvents = true; + process.Exited += (sender, args) => callbackAction.Invoke(process); + } + catch (ArgumentException) { - callbackAction.Invoke(process); - }; + // Ignore the exception if process is not running. + } } /// diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs index 6bf7badc3d..7390a926a1 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/System/ProcessHelper.cs @@ -61,12 +61,6 @@ public bool TryGetExitCode(object process, out int exitCode) throw new NotImplementedException(); } - /// - public void SetExitCallback(int parentProcessId, Action callbackAction) - { - throw new NotImplementedException(); - } - /// public void SetExitCallback(int parentProcessId, Action callbackAction) { diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs index 7248e708cb..c3b915397b 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/uap10.0/System/ProcessHelper.cs @@ -61,11 +61,6 @@ public bool TryGetExitCode(object process, out int exitCode) throw new NotImplementedException(); } - /// - public void SetExitCallback(int parentProcessId, Action callbackAction) - { - } - /// public void SetExitCallback(int parentProcessId, Action callbackAction) { diff --git a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs index f7ff9a232c..77c10fabc6 100644 --- a/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs +++ b/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs @@ -340,6 +340,7 @@ private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToke { var processId = this.testHostLauncher.LaunchTestHost(testHostStartInfo); this.testHostProcess = Process.GetProcessById(processId); + this.processHelper.SetExitCallback(processId, this.ExitCallBack); } } catch (OperationCanceledException ex) diff --git a/src/datacollector/Program.cs b/src/datacollector/Program.cs index ce3fe24ea1..dd281f8376 100644 --- a/src/datacollector/Program.cs +++ b/src/datacollector/Program.cs @@ -77,7 +77,7 @@ private static void Run(string[] args) var processHelper = new ProcessHelper(); processHelper.SetExitCallback( parentProcessId, - () => + (obj) => { EqtTrace.Info("DataCollector: ParentProcess '{0}' Exited.", parentProcessId); Environment.Exit(1); diff --git a/src/testhost.x86/DefaultEngineInvoker.cs b/src/testhost.x86/DefaultEngineInvoker.cs index 3ceb07f7ab..49aaad36d0 100644 --- a/src/testhost.x86/DefaultEngineInvoker.cs +++ b/src/testhost.x86/DefaultEngineInvoker.cs @@ -90,7 +90,7 @@ public void Invoke(IDictionary argsDictionary) var processHelper = new ProcessHelper(); processHelper.SetExitCallback( parentProcessId, - () => + (obj) => { EqtTrace.Info("DefaultEngineInvoker: ParentProcess '{0}' Exited.", parentProcessId); new PlatformEnvironment().Exit(1); diff --git a/src/vstest.console/Processors/PortArgumentProcessor.cs b/src/vstest.console/Processors/PortArgumentProcessor.cs index 790eff89c3..0e6b42af3f 100644 --- a/src/vstest.console/Processors/PortArgumentProcessor.cs +++ b/src/vstest.console/Processors/PortArgumentProcessor.cs @@ -203,7 +203,7 @@ private static IDesignModeClient InitializeDesignMode(int parentProcessId, IProc { if (parentProcessId > 0) { - processHelper.SetExitCallback(parentProcessId, () => + processHelper.SetExitCallback(parentProcessId, (obj) => { EqtTrace.Info($"PortArgumentProcessor: parent process:{parentProcessId} exited."); DesignModeClient.Instance?.HandleParentProcessExit(); diff --git a/test/vstest.console.UnitTests/Processors/PortArgumentProcessorTests.cs b/test/vstest.console.UnitTests/Processors/PortArgumentProcessorTests.cs index 921687b64f..897ea3cb4e 100644 --- a/test/vstest.console.UnitTests/Processors/PortArgumentProcessorTests.cs +++ b/test/vstest.console.UnitTests/Processors/PortArgumentProcessorTests.cs @@ -124,7 +124,7 @@ public void ExecutorInitializeShouldSetProcessExitCallback() this.executor.Initialize(port.ToString()); - this.mockProcessHelper.Verify(ph => ph.SetExitCallback(processId, It.IsAny()), Times.Once); + this.mockProcessHelper.Verify(ph => ph.SetExitCallback(processId, It.IsAny>()), Times.Once); } [TestMethod] From 9aa96daf36e4b5c89e19af9209cd27d718e3fdbe Mon Sep 17 00:00:00 2001 From: samadala Date: Thu, 4 Jan 2018 16:53:26 +0530 Subject: [PATCH 3/5] Add unit tests --- .../Hosting/DefaultTestHostManagerTests.cs | 12 ++++++++++++ .../Hosting/DotnetTestHostManagerTests.cs | 14 ++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs index 82c4a7b1e3..23da236bcf 100644 --- a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs +++ b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs @@ -380,6 +380,18 @@ public void LaunchTestHostShouldUseCustomHostIfSet() Assert.AreEqual(currentProcess.Id, this.testHostId); } + [TestMethod] + public void LaunchTestHostShouldSetExitCallbackInCaseCustomHost() + { + var mockCustomLauncher = new Mock(); + this.testHostManager.SetCustomLauncher(mockCustomLauncher.Object); + var currentProcess = Process.GetCurrentProcess(); + mockCustomLauncher.Setup(mc => mc.LaunchTestHost(It.IsAny())).Returns(currentProcess.Id); + this.testHostManager.LaunchTestHostAsync(this.startInfo, CancellationToken.None).Wait(); + + this.mockProcessHelper.Verify(ph => ph.SetExitCallback(currentProcess.Id, It.IsAny>())); + } + [TestMethod] public void GetTestSourcesShouldReturnAppropriateSourceIfAppxRecipeIsProvided() { diff --git a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs index de3e373bf3..4bf97e46c9 100644 --- a/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs +++ b/test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs @@ -369,6 +369,20 @@ public async Task LaunchTestHostShouldLaunchProcessWithConnectionInfo() this.mockTestHostLauncher.Verify(thl => thl.LaunchTestHost(It.Is(x => x.Arguments.Equals(expectedArgs))), Times.Once); } + [TestMethod] + public void LaunchTestHostShouldSetExitCallBackInCaseCustomHost() + { + var expectedProcessId = Process.GetCurrentProcess().Id; + this.mockTestHostLauncher.Setup(thl => thl.LaunchTestHost(It.IsAny())).Returns(expectedProcessId); + this.mockFileHelper.Setup(ph => ph.Exists("testhost.dll")).Returns(true); + + var startInfo = this.GetDefaultStartInfo(); + this.dotnetHostManager.SetCustomLauncher(this.mockTestHostLauncher.Object); + this.dotnetHostManager.LaunchTestHostAsync(startInfo, CancellationToken.None).Wait(); + + this.mockProcessHelper.Verify(ph => ph.SetExitCallback(expectedProcessId, It.IsAny>())); + } + [TestMethod] public void GetTestHostProcessStartInfoShouldIncludeTestHostPathFromSourceDirectoryIfDepsFileNotFound() { From 0848eafca059e3e49790a8a91ea2b9919970de83 Mon Sep 17 00:00:00 2001 From: samadala Date: Thu, 4 Jan 2018 17:52:25 +0530 Subject: [PATCH 4/5] Add comment --- .../TestRequestSender.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs index 4b975a15c8..9e68a20d34 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs @@ -313,7 +313,7 @@ public void OnClientProcessExit(string stdError) this.clientExitErrorMessage = stdError; this.clientExited.Set(); - // Break communication loop. + // Break communication loop. In somecases(E.g: When tests creates child processes to testhost) communication channel won't break if testhost exits. this.communicationEndpoint.Stop(); } From 659b945436e4e966ad2f71e8ba0ff13043baa633 Mon Sep 17 00:00:00 2001 From: samadala Date: Fri, 5 Jan 2018 12:10:40 +0530 Subject: [PATCH 5/5] Invoke callback if process is not running --- .../common/System/ProcessHelper.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index b479245ca3..98099294d7 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -51,10 +51,10 @@ public object LaunchProcess(string processPath, string arguments, string working process.Exited += (sender, args) => { // Call WaitForExit without again to ensure all streams are flushed, - // Add timeout to avoid indefinite waiting on child process exist. var p = sender as Process; try { + // Add timeout to avoid indefinite waiting on child process exit. p.WaitForExit(500); } catch (InvalidOperationException) @@ -137,11 +137,13 @@ public void SetExitCallback(int processId, Action callbackAction) { var process = Process.GetProcessById(processId); process.EnableRaisingEvents = true; - process.Exited += (sender, args) => callbackAction.Invoke(process); + process.Exited += (sender, args) => callbackAction?.Invoke(sender); } catch (ArgumentException) { - // Ignore the exception if process is not running. + // Process.GetProcessById() throws ArgumentException if process is not running(identifier might be expired). + // Invoke callback immediately. + callbackAction?.Invoke(null); } }