Handle correctly waiting for process exit on Unix systems (#3410)

Co-authored-by: Amaury Levé <amauryleve@microsoft.com>

src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs

@@ -6,6 +6,8 @@
 using System.Diagnostics;
 using System.IO;
 using System.Reflection;
+using System.Threading;
+using System.Threading.Tasks;
 
 using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;
 
@@ -80,21 +82,38 @@ void InitializeAndStart()
 
             if (exitCallBack != null)
             {
-                process.Exited += (sender, args) =>
+                process.Exited += async (sender, args) =>
                 {
-                    // Call WaitForExit without again to ensure all streams are flushed,
-                    var p = sender as Process;
-                    try
-                    {
-                        // Add timeout to avoid indefinite waiting on child process exit.
-                        p.WaitForExit(500);
-                    }
-                    catch (InvalidOperationException)
+                    if (sender is Process p)
                     {
+                        try
+                        {
+                            // NOTE: When receiving an exit event, we want to give some time to the child process
+                            // to close properly (i.e. flush output, error stream...). Despite this simple need,
+                            // the actual implementation needs to be complex, especially for Unix systems.
+                            // See ticket https://github.com/microsoft/vstest/issues/3375 to get the links to all
+                            // issues, discussions and documentations.
+                            //
+                            // On .NET 5 and later, the solution is simple, we can simply use WaitForExitAsync which
+                            // correctly ensure that some time is given to the child process (or any grandchild) to
+                            // flush before exit happens.
+                            //
+                            // For older frameworks, the solution is more tricky but it seems we can get the expected
+                            // behavior using the parameterless 'WaitForExit()' combined with an awaited Task.Run call.
+                            var cts = new CancellationTokenSource(500);
+#if NET5_0_OR_GREATER
+                            await p.WaitForExitAsync(cts.Token);
+#else
+                            await Task.Run(() => p.WaitForExit(), cts.Token);
+#endif
+                        }
+                        catch (Exception ex) when (ex is InvalidOperationException or TaskCanceledException)
+                        {
+                        }
                     }
 
                     // If exit callback has code that access Process object, ensure that the exceptions handling should be done properly.
-                    exitCallBack(p);
+                    exitCallBack(sender);
                 };
             }
 

test/Microsoft.TestPlatform.AcceptanceTests/ProcessesInteractionTests.cs

@@ -0,0 +1,49 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.IO;
+
+using Microsoft.TestPlatform.TestUtilities;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Microsoft.TestPlatform.AcceptanceTests;
+
+[TestClass]
+public class ProcessesInteractionTests : AcceptanceTestBase
+{
+    /// <summary>
+    /// Having an invalid framework is a way to reproduce an issue we had on Unix, where we were
+    /// not handling correctly the process exit (causing us to not let time to the process to
+    /// flush its output and error streams).See https://github.com/microsoft/vstest/issues/3375
+    /// </summary>
+    [TestMethod]
+    [NetCoreTargetFrameworkDataSource]
+    public void WhenTestHostProcessExitsBecauseTheTargetedRuntimeIsNoFoundThenTheMessageIsCapturedFromTheErrorOutput(RunnerInfo runnerInfo)
+    {
+        // Arrange
+        SetTestEnvironment(_testEnvironment, runnerInfo);
+        const string testAssetProjectName = "SimpleTestProjectMessedUpTargetFramework";
+        var assemblyPath = GetAssetFullPath(testAssetProjectName + ".dll", Core21TargetFramework);
+        UpdateRuntimeConfigJsonWithInvalidFramework(assemblyPath, testAssetProjectName);
+        using var tempDir = new TempDirectory();
+
+        // Act
+        var arguments = PrepareArguments(assemblyPath, GetTestAdapterPath(), "", FrameworkArgValue, tempDir.Path);
+        InvokeVsTest(assemblyPath);
+
+        // Assert
+        ExitCodeEquals(1);
+        StdErrorContains("The framework 'Microsoft.NETCore.App', version '0.0.0' (x64) was not found.");
+
+        static void UpdateRuntimeConfigJsonWithInvalidFramework(string assemblyPath, string testAssetProjectName)
+        {
+            // On the contrary to other tests, we need to modify the test asset we are using to replace
+            // the target framework with an invalid framework. This is why we have a specific test asset
+            // that's only meant to be used by this project.
+            var runtimeConfigJson = Path.Combine(Path.GetDirectoryName(assemblyPath), testAssetProjectName + ".runtimeconfig.json");
+            var fileContent = File.ReadAllText(runtimeConfigJson);
+            var updatedContent = fileContent.Replace("\"version\": \"2.1.0\"", "\"version\": \"0.0.0\"");
+            File.WriteAllText(runtimeConfigJson, updatedContent);
+        }
+    }
+}

test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBase.cs

@@ -599,6 +599,11 @@ protected static void ExecuteApplication(string path, string args, out string st
             throw new ArgumentException("Executable path must not be null or whitespace.", nameof(path));
         }
 
+        if (!File.Exists(path))
+        {
+            throw new ArgumentException($"Executable path '{path}' could not be found.", nameof(path));
+        }
+
         var executableName = Path.GetFileName(path);
 
         using var process = new Process();

test/TestAssets/SimpleTestProjectMessedUpTargetFramework/SimpleTestProjectMessedUpTargetFramework.csproj

@@ -0,0 +1,25 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <!-- Imports Common TestAssets props. -->
+  <Import Project="..\..\..\scripts\build\TestAssets.props" />
+
+  <PropertyGroup>
+    <TargetFrameworks>netcoreapp2.1</TargetFrameworks>
+    <TargetFrameworks Condition=" '$(DotNetBuildFromSource)' == 'true' ">netcoreapp3.1</TargetFrameworks>
+    <TestProject>true</TestProject>
+    <IsTestProject>true</IsTestProject>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="MSTest.TestFramework">
+      <Version>$(MSTestFrameworkVersion)</Version>
+    </PackageReference>
+    <PackageReference Include="MSTest.TestAdapter">
+      <Version>$(MSTestAdapterVersion)</Version>
+    </PackageReference>
+    <PackageReference Include="Microsoft.NET.Test.Sdk">
+      <Version>$(NETTestSdkVersion)</Version>
+    </PackageReference>
+  </ItemGroup>
+
+</Project>

test/TestAssets/SimpleTestProjectMessedUpTargetFramework/UnitTest1.cs

@@ -0,0 +1,16 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace SimpleTestProjectMessedUpTargetFramework
+{
+    [TestClass]
+    public class UnitTest1
+    {
+        [TestMethod]
+        public void TestMethod1()
+        {
+        }
+    }
+}

test/TestAssets/TestAssets.sln

@@ -94,6 +94,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "AttachmentProcessorDataColl
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ArchitectureSwitch", "ArchitectureSwitch\ArchitectureSwitch.csproj", "{452352E1-71CA-436E-8165-F284EE36C924}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SimpleTestProjectMessedUpTargetFramework", "SimpleTestProjectMessedUpTargetFramework\SimpleTestProjectMessedUpTargetFramework.csproj", "{08C44607-EB80-4EE5-927D-08C34AA277AF}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -620,6 +622,18 @@ Global
 		{452352E1-71CA-436E-8165-F284EE36C924}.Release|x64.Build.0 = Release|Any CPU
 		{452352E1-71CA-436E-8165-F284EE36C924}.Release|x86.ActiveCfg = Release|Any CPU
 		{452352E1-71CA-436E-8165-F284EE36C924}.Release|x86.Build.0 = Release|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Debug|x64.ActiveCfg = Debug|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Debug|x64.Build.0 = Debug|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Debug|x86.ActiveCfg = Debug|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Debug|x86.Build.0 = Debug|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Release|Any CPU.Build.0 = Release|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Release|x64.ActiveCfg = Release|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Release|x64.Build.0 = Release|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Release|x86.ActiveCfg = Release|Any CPU
+		{08C44607-EB80-4EE5-927D-08C34AA277AF}.Release|x86.Build.0 = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE