diff --git a/scripts/build/TestPlatform.Dependencies.props b/scripts/build/TestPlatform.Dependencies.props index 055803abb0..a4296403ae 100644 --- a/scripts/build/TestPlatform.Dependencies.props +++ b/scripts/build/TestPlatform.Dependencies.props @@ -42,3 +42,4 @@ + diff --git a/src/testhost.x86/DebugAssertException.cs b/src/testhost.x86/DebugAssertException.cs new file mode 100644 index 0000000000..1255f5898e --- /dev/null +++ b/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; } + } +} diff --git a/src/testhost.x86/DefaultEngineInvoker.cs b/src/testhost.x86/DefaultEngineInvoker.cs index 24f8c5be5d..fe13f03904 100644 --- a/src/testhost.x86/DefaultEngineInvoker.cs +++ b/src/testhost.x86/DefaultEngineInvoker.cs @@ -84,6 +84,10 @@ public void Invoke(IDictionary argsDictionary) #endif } +#if NETCOREAPP + TestHostTraceListener.Setup(); +#endif + this.SetParentProcessExitCallback(argsDictionary); this.requestHandler.ConnectionInfo = @@ -92,7 +96,8 @@ public void Invoke(IDictionary 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 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 argsDictionary) diff --git a/src/testhost.x86/TestHostTraceListener.cs b/src/testhost.x86/TestHostTraceListener.cs new file mode 100644 index 0000000000..0c3fab7790 --- /dev/null +++ b/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)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}" : ""; + 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 +} \ No newline at end of file diff --git a/src/testhost/testhost.csproj b/src/testhost/testhost.csproj index 97ef49bce1..66928e9c0f 100644 --- a/src/testhost/testhost.csproj +++ b/src/testhost/testhost.csproj @@ -16,7 +16,9 @@ false + + diff --git a/test/Microsoft.TestPlatform.AcceptanceTests/DebugAssertTests.cs b/test/Microsoft.TestPlatform.AcceptanceTests/DebugAssertTests.cs new file mode 100644 index 0000000000..d2e5156bc1 --- /dev/null +++ b/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"); + } + } +} \ No newline at end of file diff --git a/test/TestAssets/CrashingOnDebugAssertTestProject/CrashingOnDebugAssertTestProject.csproj b/test/TestAssets/CrashingOnDebugAssertTestProject/CrashingOnDebugAssertTestProject.csproj new file mode 100644 index 0000000000..9c87d9e680 Binary files /dev/null and b/test/TestAssets/CrashingOnDebugAssertTestProject/CrashingOnDebugAssertTestProject.csproj differ diff --git a/test/TestAssets/CrashingOnDebugAssertTestProject/DebugTests.cs b/test/TestAssets/CrashingOnDebugAssertTestProject/DebugTests.cs new file mode 100644 index 0000000000..52d252f1aa --- /dev/null +++ b/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"); + } + } +} diff --git a/test/TestAssets/TestAssets.sln/TestAssets.sln b/test/TestAssets/TestAssets.sln/TestAssets.sln index cfdfa3af54..e09a9e28e7 100644 --- a/test/TestAssets/TestAssets.sln/TestAssets.sln +++ b/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 diff --git a/test/coverlet.collector/coverlet.collector.csproj b/test/coverlet.collector/coverlet.collector.csproj index 6ab15f7846..a881df2bb4 100644 --- a/test/coverlet.collector/coverlet.collector.csproj +++ b/test/coverlet.collector/coverlet.collector.csproj @@ -5,10 +5,6 @@ false - - - - diff --git a/test/testhost.UnitTests/TestHostTraceListenerTests.cs b/test/testhost.UnitTests/TestHostTraceListenerTests.cs new file mode 100644 index 0000000000..41680ef59c --- /dev/null +++ b/test/testhost.UnitTests/TestHostTraceListenerTests.cs @@ -0,0 +1,159 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// define trace and debug to trigger the Debug.Assert calls even when we build in Release +#define DEBUG + +namespace testhost.UnitTests +{ +#if NETCOREAPP + using Microsoft.VisualStudio.TestPlatform.TestHost; + using Microsoft.VisualStudio.TestTools.UnitTesting; + using System.Collections.Generic; + using System.Diagnostics; + + [TestClass] + public class TestHostTraceListenerTests + { + TraceListener[] listeners; + + [TestInitialize()] + public void Initialize() + { + this.listeners = new TraceListener[Trace.Listeners.Count]; + Trace.Listeners.CopyTo(this.listeners, 0); + // not using the TestHostTraceListener.Setup method here + // because that detects only default trace listeners and there won't + // be any when this is in production, so this would end up testing against + // an older version of the trace listener + Trace.Listeners.Clear(); + Trace.Listeners.Add(new TestHostTraceListener()); + } + + [TestCleanup()] + public void Cleanup() { + Trace.Listeners.Clear(); + foreach(var listener in this.listeners) + { + Trace.Listeners.Add(listener); + } + } + + [TestMethod] + [ExpectedException(typeof(DebugAssertException))] + public void DebugAssertThrowsDebugAssertException() + { + Debug.Assert(false); + } + + [TestMethod] + [ExpectedException(typeof(DebugAssertException))] + public void DebugFailThrowsDebugAssertException() + { + Debug.Fail("fail"); + } + + [TestMethod] + [ExpectedException(typeof(DebugAssertException))] + public void TraceAssertThrowsDebugAssertException() + { + Trace.Assert(false); + } + + [TestMethod] + [ExpectedException(typeof(DebugAssertException))] + public void TraceFailThrowsDebugAssertException() + { + 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"); + } + } + + [TestClass] + public class TestHostTraceListenerRegistrationTests + { + TraceListener[] listeners; + + [TestInitialize()] + public void Initialize() + { + this.listeners = new TraceListener[Trace.Listeners.Count]; + Trace.Listeners.CopyTo(this.listeners, 0); + } + + [TestCleanup()] + public void Cleanup() + { + Trace.Listeners.Clear(); + foreach (var listener in this.listeners) + { + Trace.Listeners.Add(listener); + } + } + + [TestMethod] + public void SetupReplacesDefaultTraceListener() + { + Trace.Listeners.Clear(); + Trace.Listeners.Add(new DefaultTraceListener()); + TestHostTraceListener.Setup(); + + // this is what will happen in the majority of cases, there will be a single + // trace listener that will be the default trace listener and we will replace it + // with ours + Assert.IsInstanceOfType(Trace.Listeners[0], typeof(TestHostTraceListener)); + } + + [TestMethod] + public void SetupKeepsNonDefaultTraceListeners() + { + Trace.Listeners.Clear(); + Trace.Listeners.Add(new DummyTraceListener()); + Trace.Listeners.Add(new DefaultTraceListener()); + Trace.Listeners.Add(new DummyTraceListener()); + TestHostTraceListener.Setup(); + + Assert.IsInstanceOfType(Trace.Listeners[0], typeof(DummyTraceListener)); + Assert.IsInstanceOfType(Trace.Listeners[1], typeof(TestHostTraceListener)); + Assert.IsInstanceOfType(Trace.Listeners[2], typeof(DummyTraceListener)); + } + + private class DummyTraceListener : TraceListener + { + public List Lines { get; } = new List(); + public override void Write(string message) + { + Lines.Add(message); + } + + public override void WriteLine(string message) + { + Lines.Add(message); + } + } + } +#endif +}