Write warnings outside of appdomain

src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerationResult.cs

@@ -0,0 +1,14 @@
+// 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.TestPlatform.MSTest.TestAdapter.ObjectModel;
+
+namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery;
+
+/// <summary>
+/// Helps us communicate results that were created inside of AppDomain, when AppDomains are available and enabled.
+/// </summary>
+/// <param name="TestElements">The test elements that were discovered.</param>
+/// <param name="Warnings">Warnings that happened during discovery.</param>
+[Serializable]
+internal sealed record AssemblyEnumerationResult(List<UnitTestElement> TestElements, List<string> Warnings);

src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs

@@ -62,12 +62,10 @@ public AssemblyEnumerator(MSTestSettings settings) =>
     /// Enumerates through all types in the assembly in search of valid test methods.
     /// </summary>
     /// <param name="assemblyFileName">The assembly file name.</param>
-    /// <param name="warnings">Contains warnings if any, that need to be passed back to the caller.</param>
     /// <returns>A collection of Test Elements.</returns>
-    internal ICollection<UnitTestElement> EnumerateAssembly(
-        string assemblyFileName,
-        List<string> warnings)
+    internal AssemblyEnumerationResult EnumerateAssembly(string assemblyFileName)
     {
+        List<string> warnings = new();
         DebugEx.Assert(!StringEx.IsNullOrWhiteSpace(assemblyFileName), "Invalid assembly file name.");
         var tests = new List<UnitTestElement>();
         // Contains list of assembly/class names for which we have already added fixture tests.
@@ -117,7 +115,7 @@ internal ICollection<UnitTestElement> EnumerateAssembly(
             tests.AddRange(testsInType);
         }
 
-        return tests;
+        return new AssemblyEnumerationResult(tests, warnings);
     }
 
     /// <summary>

src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumeratorWrapper.cs

@@ -49,7 +49,9 @@ internal sealed class AssemblyEnumeratorWrapper
             }
 
             // Load the assembly in isolation if required.
-            return GetTestsInIsolation(fullFilePath, runSettings, warnings);
+            AssemblyEnumerationResult result = GetTestsInIsolation(fullFilePath, runSettings);
+            warnings.AddRange(result.Warnings);
+            return result.TestElements;
         }
         catch (FileNotFoundException ex)
         {
@@ -95,7 +97,7 @@ internal sealed class AssemblyEnumeratorWrapper
         }
     }
 
-    private static ICollection<UnitTestElement> GetTestsInIsolation(string fullFilePath, IRunSettings? runSettings, List<string> warnings)
+    private static AssemblyEnumerationResult GetTestsInIsolation(string fullFilePath, IRunSettings? runSettings)
     {
         using MSTestAdapter.PlatformServices.Interface.ITestSourceHost isolationHost = PlatformServiceProvider.Instance.CreateTestSourceHost(fullFilePath, runSettings, frameworkHandle: null);
 
@@ -114,6 +116,10 @@ private static ICollection<UnitTestElement> GetTestsInIsolation(string fullFileP
             PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning(Resource.OlderTFMVersionFound);
         }
 
-        return assemblyEnumerator.EnumerateAssembly(fullFilePath, warnings);
+        // This method runs inside of appdomain, when appdomains are available and enabled.
+        // Be careful how you pass data from the method. We were previously passing in a collection
+        // of strings normally (by reference), and we were mutating that collection in the appdomain.
+        // But this does not mutate the collection outside of appdomain, so we lost all warnings that happened inside.
+        return assemblyEnumerator.EnumerateAssembly(fullFilePath);
     }
 }

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestDiscoveryWarningsTests.cs

@@ -0,0 +1,132 @@
+// 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.Testing.Platform.Acceptance.IntegrationTests;
+using Microsoft.Testing.Platform.Acceptance.IntegrationTests.Helpers;
+using Microsoft.Testing.Platform.Helpers;
+
+namespace MSTest.Acceptance.IntegrationTests;
+
+[TestClass]
+public class TestDiscoveryWarningsTests : AcceptanceTestBase<TestDiscoveryWarningsTests.TestAssetFixture>
+{
+    private const string AssetName = "TestDiscoveryWarnings";
+    private const string BaseClassAssetName = "TestDiscoveryWarningsBaseClass";
+
+    [TestMethod]
+    [DynamicData(nameof(TargetFrameworks.AllForDynamicData), typeof(TargetFrameworks))]
+    public async Task DiscoverTests_ShowsWarningsForTestsThatFailedToDiscover(string currentTfm)
+    {
+        var testHost = TestHost.LocateFrom(AssetFixture.TargetAssetPath, AssetName, currentTfm);
+
+        if (currentTfm.StartsWith("net4", StringComparison.OrdinalIgnoreCase))
+        {
+            // .NET Framework will isolate the run into appdomain, there we did not write the warnings out
+            // so before running the discovery, we want to ensure that the tests do run in appdomain.
+            // We check for appdomain directly in the test, so if tests fail we did not run in appdomain.
+            TestHostResult testHostSuccessResult = await testHost.ExecuteAsync();
+
+            testHostSuccessResult.AssertExitCodeIs(ExitCodes.Success);
+        }
+
+        // Delete the TestDiscoveryWarningsBaseClass.dll from the test bin folder on purpose, to break discovering
+        // because the type won't be loaded on runtime, and mstest will write warning.
+        File.Delete(Path.Combine(testHost.DirectoryName, $"{BaseClassAssetName}.dll"));
+
+        TestHostResult testHostResult = await testHost.ExecuteAsync("--list-tests");
+
+        testHostResult.AssertExitCodeIsNot(ExitCodes.Success);
+        testHostResult.AssertOutputContains("System.IO.FileNotFoundException: Could not load file or assembly 'TestDiscoveryWarningsBaseClass");
+    }
+
+    public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.NuGetGlobalPackagesFolder)
+    {
+        public string TargetAssetPath => GetAssetPath(AssetName);
+
+        public string BaseTargetAssetPath => GetAssetPath(BaseClassAssetName);
+
+        public override IEnumerable<(string ID, string Name, string Code)> GetAssetsToGenerate()
+        {
+            yield return (BaseClassAssetName, BaseClassAssetName,
+            BaseClassSourceCode.PatchTargetFrameworks(TargetFrameworks.All));
+
+            yield return (AssetName, AssetName,
+            SourceCode.PatchTargetFrameworks(TargetFrameworks.All)
+            .PatchCodeWithReplace("$MSTestVersion$", MSTestVersion));
+        }
+
+        private const string SourceCode = """
+#file TestDiscoveryWarnings.csproj
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <EnableMSTestRunner>true</EnableMSTestRunner>
+    <TargetFrameworks>$TargetFrameworks$</TargetFrameworks>
+    <LangVersion>latest</LangVersion>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <ProjectReference Include="../TestDiscoveryWarningsBaseClass/TestDiscoveryWarningsBaseClass.csproj" />
+  </ItemGroup>
+  <ItemGroup>
+    <PackageReference Include="MSTest.TestAdapter" Version="$MSTestVersion$" />
+    <PackageReference Include="MSTest.TestFramework" Version="$MSTestVersion$" />
+  </ItemGroup>
+
+</Project>
+
+#file UnitTest1.cs
+
+using Base;
+
+using System;
+using System.Threading.Tasks;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+[TestClass]
+public class TestClass1 : BaseClass
+{
+    [TestMethod]
+    public void Test1_1()
+    {
+#if NETFRAMEWORK
+        // Ensure we run in appdomain, and not directly in host, because we want to ensure that warnings are correctly passed
+        // outside of the appdomain to the rest of the engine.
+        //\
+        // We set this friendly appdomain name in src\Adapter\MSTestAdapter.PlatformServices\Services\TestSourceHost.cs:163
+        StringAssert.StartsWith(AppDomain.CurrentDomain.FriendlyName, "TestSourceHost: Enumerating source");
+#endif
+    }
+}
+
+[TestClass]
+public class TestClass2
+{
+    [TestMethod]
+    public void Test2_1() {}
+}
+""";
+
+        private const string BaseClassSourceCode = """
+#file TestDiscoveryWarningsBaseClass.csproj
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFrameworks>$TargetFrameworks$</TargetFrameworks>
+    <IsPackable>false</IsPackable>
+    <LangVersion>latest</LangVersion>
+  </PropertyGroup>
+
+</Project>
+
+
+#file UnitTest1.cs
+namespace Base;
+
+public class BaseClass
+{
+}
+""";
+    }
+}

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/ThreadingTests.cs

@@ -188,7 +188,7 @@ public async Task LifecycleAttributesTaskThreading_WhenMainIsNotSTA_RunsettingsA
             return;
         }
 
-        var testHost = TestHost.LocateFrom(AssetFixture.LifecycleAttributesTaskProjectPath, TestAssetFixture.LifecycleWithParallelAttributesTaskProjectName, tfm);
+        var testHost = TestHost.LocateFrom(AssetFixture.LifecycleWithParallelAttributesTaskProjectNamePath, TestAssetFixture.LifecycleAttributesTaskProjectName, tfm);
         string runSettingsFilePath = Path.Combine(testHost.DirectoryName, "sta.runsettings");
         TestHostResult testHostResult = await testHost.ExecuteAsync($"--settings {runSettingsFilePath}", environmentVariables: new()
         {
@@ -225,7 +225,7 @@ public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.
         public const string STAThreadProjectName = "STATestThreading";
         public const string LifecycleAttributesVoidProjectName = "LifecycleAttributesVoid";
         public const string LifecycleAttributesTaskProjectName = "LifecycleAttributesTask";
-        public const string LifecycleWithParallelAttributesTaskProjectName = "LifecycleAttributesTask";
+        public const string LifecycleWithParallelAttributesTaskProjectName = "LifecycleWithParallelAttributesTask";
         public const string LifecycleAttributesValueTaskProjectName = "LifecycleAttributesValueTask";
 
         public string ProjectPath => GetAssetPath(ProjectName);
@@ -267,7 +267,7 @@ public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.
                 .PatchCodeWithReplace("$ParallelAttribute$", string.Empty)
                 .PatchCodeWithReplace("$MSTestVersion$", MSTestVersion));
 
-            yield return (LifecycleWithParallelAttributesTaskProjectName, LifecycleWithParallelAttributesTaskProjectName,
+            yield return (LifecycleWithParallelAttributesTaskProjectName, LifecycleAttributesTaskProjectName,
                 LifecycleAttributesTaskSource
                 .PatchTargetFrameworks(TargetFrameworks.All)
                 .PatchCodeWithReplace("$ParallelAttribute$", "[assembly: Parallelize(Workers = 0, Scope = ExecutionScope.MethodLevel)]")

test/UnitTests/MSTestAdapter.UnitTests/Discovery/AssemblyEnumeratorTests.cs

@@ -217,7 +217,9 @@ public void EnumerateAssemblyShouldReturnEmptyListWhenNoDeclaredTypes()
         _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("DummyAssembly", false))
             .Returns(mockAssembly.Object);
 
-        Verify(_assemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings).Count == 0);
+        AssemblyEnumerationResult result = _assemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
+        Verify(result.TestElements.Count == 0);
     }
 
     public void EnumerateAssemblyShouldReturnEmptyListWhenNoTestElementsInAType()
@@ -235,7 +237,9 @@ public void EnumerateAssemblyShouldReturnEmptyListWhenNoTestElementsInAType()
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
             .Returns((List<UnitTestElement>)null!);
 
-        Verify(_assemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings).Count == 0);
+        AssemblyEnumerationResult result = _assemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
+        Verify(result.TestElements.Count == 0);
     }
 
     public void EnumerateAssemblyShouldReturnTestElementsForAType()
@@ -254,9 +258,10 @@ public void EnumerateAssemblyShouldReturnTestElementsForAType()
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
             .Returns(new List<UnitTestElement> { unitTestElement });
 
-        ICollection<UnitTestElement> testElements = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
 
-        Verify(new Collection<UnitTestElement> { unitTestElement }.SequenceEqual(testElements));
+        Verify(new Collection<UnitTestElement> { unitTestElement }.SequenceEqual(result.TestElements));
     }
 
     public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForAType()
@@ -276,9 +281,10 @@ public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForAType()
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
             .Returns(expectedTestElements);
 
-        ICollection<UnitTestElement> testElements = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
 
-        Verify(expectedTestElements.SequenceEqual(testElements));
+        Verify(expectedTestElements.SequenceEqual(result.TestElements));
     }
 
     public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForMoreThanOneType()
@@ -298,11 +304,12 @@ public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForMoreThanOneTyp
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
             .Returns(expectedTestElements);
 
-        ICollection<UnitTestElement> testElements = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
 
         expectedTestElements.Add(unitTestElement);
         expectedTestElements.Add(unitTestElement);
-        Verify(expectedTestElements.SequenceEqual(testElements));
+        Verify(expectedTestElements.SequenceEqual(result.TestElements));
     }
 
     public void EnumerateAssemblyShouldNotLogWarningsIfNonePresent()
@@ -320,8 +327,9 @@ public void EnumerateAssemblyShouldNotLogWarningsIfNonePresent()
             .Returns(mockAssembly.Object);
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(warningsFromTypeEnumerator));
 
-        testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
-        Verify(_warnings.Count == 0);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
+        Verify(result.Warnings.Count == 0);
     }
 
     public void EnumerateAssemblyShouldLogWarningsIfPresent()
@@ -340,12 +348,12 @@ public void EnumerateAssemblyShouldLogWarningsIfPresent()
             .Returns([typeof(InternalTestClass).GetTypeInfo()]);
         _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("DummyAssembly", false))
             .Returns(mockAssembly.Object);
-        testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
-            .Callback(() => _warnings.AddRange(warningsFromTypeEnumerator));
+        testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(It.IsAny<List<string>>()))
+            .Callback<List<string>>((w) => w.AddRange(warningsFromTypeEnumerator));
 
-        testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
 
-        Verify(warningsFromTypeEnumerator.SequenceEqual(_warnings));
+        Verify(warningsFromTypeEnumerator.SequenceEqual(result.Warnings));
     }
 
     public void EnumerateAssemblyShouldHandleExceptionsWhileEnumeratingAType()
@@ -363,9 +371,10 @@ public void EnumerateAssemblyShouldHandleExceptionsWhileEnumeratingAType()
             .Returns(mockAssembly.Object);
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings)).Throws(exception);
 
-        testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
 
-        Verify(_warnings.ToList().Contains(
+        Verify(result.Warnings.Contains(
             string.Format(
                 CultureInfo.CurrentCulture,
                 Resource.CouldNotInspectTypeDuringDiscovery,