Do not crash on Debug.Assert

scripts/build/TestPlatform.Dependencies.props

@@ -42,3 +42,4 @@
   <Import Project="$(DotNetPackageVersionPropsPath)" Condition="'$(DotNetPackageVersionPropsPath)' != ''" />
 
 </Project>
+

src/testhost.x86/DebugAssertException.cs

@@ -0,0 +1,17 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.TestPlatform.TestHost
+{
+    using System;
+
+    internal sealed class DebugAssertException : Exception
+    {
+        public DebugAssertException(string message, string stackTrace) : base(message)
+        {
+            StackTrace = stackTrace;
+        }
+
+        public override string StackTrace { get; }
+    }
+}

src/testhost.x86/DefaultEngineInvoker.cs

@@ -84,6 +84,10 @@ public void Invoke(IDictionary<string, string> argsDictionary)
 #endif
             }
 
+#if NETCOREAPP
+            TestHostTraceListener.Setup();
+#endif
+
             this.SetParentProcessExitCallback(argsDictionary);
 
             this.requestHandler.ConnectionInfo =
@@ -92,7 +96,8 @@ public void Invoke(IDictionary<string, string> argsDictionary)
             // Initialize Communication with vstest.console
             this.requestHandler.InitializeCommunication();
 
-            // Initialize DataCollection Communication if data collection port is provided.
+            // skipping because 0 is the default value, and also the value the the callers use when they
+            // call with the parameter specified, but without providing an actual port
             var dcPort = CommandLineArgumentsHelper.GetIntArgFromDict(argsDictionary, DataCollectionPortArgument);
             if (dcPort > 0)
             {
@@ -176,22 +181,35 @@ private void ConnectToDatacollector(int dcPort)
         private void SetParentProcessExitCallback(IDictionary<string, string> argsDictionary)
         {
             // Attach to exit of parent process
-            var parentProcessId = CommandLineArgumentsHelper.GetIntArgFromDict(argsDictionary, ParentProcessIdArgument);
-            EqtTrace.Info("DefaultEngineInvoker.SetParentProcessExitCallback: Monitoring parent process with id: '{0}'",
-                parentProcessId);
+            var hasParentProcessArgument = CommandLineArgumentsHelper.TryGetIntArgFromDict(argsDictionary, ParentProcessIdArgument, out var parentProcessId);
 
-            // In remote scenario we cannot monitor parent process, so we expect user to pass parentProcessId as -1
-            if (parentProcessId != -1)
+            if (!hasParentProcessArgument)
             {
-                this.processHelper.SetExitCallback(
-                    parentProcessId,
-                    (obj) =>
-                    {
-                        EqtTrace.Info("DefaultEngineInvoker.SetParentProcessExitCallback: ParentProcess '{0}' Exited.",
-                            parentProcessId);
-                        new PlatformEnvironment().Exit(1);
-                    });
+                throw new ArgumentException($"Argument {ParentProcessIdArgument} was not specified.");
+            }
+
+            EqtTrace.Info("DefaultEngineInvoker.SetParentProcessExitCallback: Monitoring parent process with id: '{0}'", parentProcessId);
+
+            if (parentProcessId == -1)
+            {
+                // In remote scenario we cannot monitor parent process, so we expect user to pass parentProcessId as -1
+                return;
+            }
+
+            if (parentProcessId == 0)
+            {
+                //TODO: should there be a warning / error in this case, on windows and linux we are most likely not started by this PID 0, because it's Idle process on Windows, and Swapper on Linux, and similarly in docker
+                // Trying to attach to 0 will cause access denied error on Windows
             }
+
+            this.processHelper.SetExitCallback(
+                parentProcessId,
+                (obj) =>
+                {
+                    EqtTrace.Info("DefaultEngineInvoker.SetParentProcessExitCallback: ParentProcess '{0}' Exited.",
+                        parentProcessId);
+                    new PlatformEnvironment().Exit(1);
+                });
         }
 
         private static TestHostConnectionInfo GetConnectionInfo(IDictionary<string, string> argsDictionary)

src/testhost.x86/TestHostTraceListener.cs

@@ -0,0 +1,104 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+
+namespace Microsoft.VisualStudio.TestPlatform.TestHost
+{
+#if NETCOREAPP
+    using Microsoft.VisualStudio.TestPlatform.ObjectModel;
+    using System;
+    using System.Diagnostics;
+    using System.Linq;
+    using System.Reflection;
+
+    internal class TestHostTraceListener : DefaultTraceListener
+    {
+        public static void Setup()
+        {
+            EqtTrace.Info("Setting up debug trace listener.");
+            // in the majority of cases there will be only a single DefaultTraceListener in this collection
+            // and we will replace that with our listener, in case there are listeners of different types we keep
+            // them as is
+            for (var i = 0; i < Trace.Listeners.Count; i++)
+            {
+                var listener = Trace.Listeners[i];
+                if (listener is DefaultTraceListener)
+                {
+                    EqtTrace.Verbose($"TestPlatformTraceListener.Setup: Replacing listener {0} with { nameof(TestHostTraceListener) }.", Trace.Listeners[i]);
+                    Trace.Listeners[i] = new TestHostTraceListener();
+                }
+            }
+
+            EqtTrace.Verbose("TestPlatformTraceListener.Setup: Added test platform trace listener.");
+
+#if NETCOREAPP2_1
+            try
+            {
+                // workaround for netcoreapp2.1 where the trace listener api is not called when 
+                // Debug.Assert fails. This method is internal, but the class is on purpose keeping the 
+                // callback settable so tests can set the callback
+                var field = typeof(Debug).GetField("s_ShowDialog", BindingFlags.Static | BindingFlags.NonPublic);
+                var value = field.GetValue(null);
+                field.SetValue(null, (Action<string, string, string, string>)ShowDialog);
+            }
+            catch (Exception ex)
+            {
+                EqtTrace.Error("TestPlatformTraceListener.Setup: Failed to replace inner callback to ShowDialog in Debug.Assert. Calls to Debug.Assert with crash the test host process. {0}", ex);
+            }
+#endif
+        }
+
+        public override void Fail(string message)
+        {
+            throw GetException(message);
+        }
+
+        public override void Fail(string message, string detailMessage)
+        {
+            throw GetException((message + Environment.NewLine + detailMessage));
+        }
+
+        public static void ShowDialog(string stackTrace, string message, string detailMessage, string _)
+        {
+            var text = !string.IsNullOrEmpty(message)
+                ? !string.IsNullOrEmpty(detailMessage)
+                    ? (message + Environment.NewLine + detailMessage)
+                    : message
+                : null;
+            throw GetException(text);
+        }
+
+        private static DebugAssertException GetException(string message)
+        {
+            var debugTypes = new Type[] { typeof(Debug), typeof(Trace) };
+            var stack = new StackTrace(true);
+
+            var debugMethodFound = false;
+            var frameCount = 0;
+            MethodBase method = null;
+            foreach (var f in stack.GetFrames())
+            {
+                var m = f.GetMethod();
+                var declaringType = m?.DeclaringType;
+                if (!debugMethodFound && (declaringType == typeof(Debug) || declaringType == typeof(Trace)))
+                {
+                    method = m;
+                    debugMethodFound = true;
+                }
+
+                if (debugMethodFound)
+                {
+                    frameCount++;
+                }
+            }
+
+            var stackTrace = string.Join(Environment.NewLine, stack.ToString().Split(Environment.NewLine).TakeLast(frameCount));
+            var methodName = method != null ? $"{method.DeclaringType.Name}.{method.Name}" : "<method>";
+            var wholeMessage = $"Method {methodName} failed with '{message}', and was translated to { typeof(DebugAssertException).FullName } to avoid terminating the process hosting the test.";
+
+            return new DebugAssertException(wholeMessage, stackTrace);
+        }
+    }
+
+#endif
+}

src/testhost/testhost.csproj

@@ -16,7 +16,9 @@
     <AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects>
   </PropertyGroup>
   <ItemGroup>
+    <Compile Include="..\testhost.x86\DebugAssertException.cs" Link="DebugAssertException.cs" />
     <Compile Include="..\testhost.x86\Program.cs;..\testhost.x86\DefaultEngineInvoker.cs;..\testhost.x86\IEngineInvoker.cs;..\testhost.x86\AppDomainEngineInvoker.cs;..\testhost.x86\Friends.cs;..\testhost.x86\UnitTestClient.cs" />
+    <Compile Include="..\testhost.x86\TestHostTraceListener.cs" Link="TestHostTraceListener.cs" />
     <None Include="app.config" />
   </ItemGroup>
   <ItemGroup>

test/Microsoft.TestPlatform.AcceptanceTests/DebugAssertTests.cs

@@ -0,0 +1,31 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.TestPlatform.AcceptanceTests
+{
+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+    using System;
+
+    [TestClass]
+    public class DebugAssertTests : AcceptanceTestBase
+    {
+        [TestMethod]
+        // this is core only, there is nothing we can do about Debug.Assert crashing the process on framework
+        [NetCoreTargetFrameworkDataSource(useDesktopRunner: false)]
+        public void RunningTestWithAFailingDebugAssertDoesNotCrashTheHostingProcess(RunnerInfo runnerInfo)
+        {
+            // when debugging this test in case it starts failing, be aware that the default behavior of Debug.Assert 
+            // is to not crash the process when we are running in debug, and debugger is attached
+            AcceptanceTestBase.SetTestEnvironment(this.testEnvironment, runnerInfo);
+
+            var assemblyPath = this.BuildMultipleAssemblyPath("CrashingOnDebugAssertTestProject.dll").Trim('\"');
+            var arguments = PrepareArguments(assemblyPath, null, null, this.FrameworkArgValue, runnerInfo.InIsolationValue);
+            this.InvokeVsTest(arguments);
+
+            // this will have failed tests when our trace listener works and crash the testhost process when it does not
+            // because crashing processes is what a failed Debug.Assert does by default, unless you have a debugger attached
+            this.ValidateSummaryStatus(passedTestsCount: 4, failedTestsCount: 4, 0);
+            StringAssert.Contains(this.StdOut, "threw exception: Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException: Method Debug.Assert failed");
+        }
+    }
+}

test/TestAssets/CrashingOnDebugAssertTestProject/DebugTests.cs

@@ -0,0 +1,62 @@
+using System.Diagnostics;
+using Microsoft.VisualStudio.TestPlatform.ObjectModel;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace CrashingOnDebugAssertTestProject
+{
+    // Release profile in this project needs to be the same as 
+    // Debug profile, to define TRACE and DEBUG constants and don't
+    // optimize the code, otherwise we will not run Debug.Assert in 
+    // our acceptance tests on build server, because that is built as Release
+    [TestClass]
+    public class DebugTests
+    {
+        [TestMethod]
+        public void DebugAssertFailsTheTest()
+        {
+            Debug.Assert(false);
+        }
+
+        [TestMethod]
+        public void DebugFailFailsTheTest()
+        {
+            Debug.Fail("fail");
+        }
+
+        [TestMethod]
+        public void TraceAssertFailsTheTest()
+        {
+            Trace.Assert(false);
+        }
+
+        [TestMethod]
+        public void TraceFailFailsThetest()
+        {
+            Trace.Fail("fail");
+        }
+
+        [TestMethod]
+        public void TraceWriteDoesNotFailTheTest()
+        {
+            Trace.Write("hello");
+        }
+
+        [TestMethod]
+        public void TraceWriteLineDoesNotFailTheTest()
+        {
+            Trace.WriteLine("hello");
+        }
+
+        [TestMethod]
+        public void DebugWriteDoesNotFailTheTest()
+        {
+            Debug.Write("hello");
+        }
+
+        [TestMethod]
+        public void DebugWriteLineDoesNotFailTheTest()
+        {
+            Debug.WriteLine("hello");
+        }
+    }
+}

test/TestAssets/TestAssets.sln/TestAssets.sln

@@ -51,6 +51,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "EnvironmentVariablesTestPro
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ProjectFileRunSettingsTestProject", "..\ProjectFileRunSettingsTestProject\ProjectFileRunSettingsTestProject.csproj", "{A0113409-56D3-4060-BD94-A4BA4739712D}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "CrashingOnDebugAssertTestProject", "..\CrashingOnDebugAssertTestProject\CrashingOnDebugAssertTestProject.csproj", "{7A04F7AC-09E4-426C-A599-110DFA693200}"
+EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{DCD7C4DA-B8CC-46D0-AA21-1340DD1EB5ED}"
 	ProjectSection(SolutionItems) = preProject
 		..\NuGet.config = ..\NuGet.config
@@ -364,6 +366,18 @@ Global
 		{A0113409-56D3-4060-BD94-A4BA4739712D}.Release|x64.Build.0 = Release|Any CPU
 		{A0113409-56D3-4060-BD94-A4BA4739712D}.Release|x86.ActiveCfg = Release|Any CPU
 		{A0113409-56D3-4060-BD94-A4BA4739712D}.Release|x86.Build.0 = Release|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Debug|x64.ActiveCfg = Debug|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Debug|x64.Build.0 = Debug|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Debug|x86.ActiveCfg = Debug|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Debug|x86.Build.0 = Debug|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Release|Any CPU.Build.0 = Release|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Release|x64.ActiveCfg = Release|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Release|x64.Build.0 = Release|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Release|x86.ActiveCfg = Release|Any CPU
+		{7A04F7AC-09E4-426C-A599-110DFA693200}.Release|x86.Build.0 = Release|Any CPU
 		{1FF723F6-3A09-41F6-B85C-C4BE9C4374F0}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
 		{1FF723F6-3A09-41F6-B85C-C4BE9C4374F0}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{1FF723F6-3A09-41F6-B85C-C4BE9C4374F0}.Debug|x64.ActiveCfg = Debug|Any CPU

test/coverlet.collector/coverlet.collector.csproj

@@ -5,10 +5,6 @@
     <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
   </PropertyGroup>
 
-  <ItemGroup>
-    <None Include="..\..\scripts\key.snk" Link="key.snk" />
-  </ItemGroup>
-
   <ItemGroup>
     <ProjectReference Include="..\..\src\Microsoft.TestPlatform.ObjectModel\Microsoft.TestPlatform.ObjectModel.csproj" />
   </ItemGroup>