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/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()
@@ -343,9 +351,10 @@ public void EnumerateAssemblyShouldLogWarningsIfPresent()
         testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
             .Callback(() => _warnings.AddRange(warningsFromTypeEnumerator));
 
-        testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
+        AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
+        _warnings.AddRange(result.Warnings);
 
-        Verify(warningsFromTypeEnumerator.SequenceEqual(_warnings));
+        Verify(warningsFromTypeEnumerator.SequenceEqual(result.Warnings));
     }
 
     public void EnumerateAssemblyShouldHandleExceptionsWhileEnumeratingAType()
@@ -363,9 +372,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,

test/Utilities/Microsoft.Testing.TestInfrastructure/TestAsset.cs

@@ -7,17 +7,32 @@ public class TestAsset : IDisposable
 {
     private const string FileTag = "#file";
 
-    private readonly TempDirectory _tempDirectory;
+    private readonly TempDirectory? _tempDirectory;
     private readonly string _assetCode;
+
     private bool _isDisposed;
 
-    public TestAsset(string targetPath, string assetCode)
+    public TestAsset(string assetName, string assetCode, TempDirectory tempDirectory)
+    {
+        Name = assetName;
+        _assetCode = assetCode;
+        TargetAssetPath = Path.Combine(tempDirectory.Path, assetName);
+        tempDirectory.CreateDirectory(assetName);
+    }
+
+    public TestAsset(string assetName, string assetCode)
+        : this(assetName, assetCode, new TempDirectory(subDirectory: null))
     {
+        Name = assetName;
         _assetCode = assetCode;
-        _tempDirectory = new(targetPath);
+        // Assign temp directory because we own it.
+        _tempDirectory = new TempDirectory(assetName);
+        TargetAssetPath = _tempDirectory.Path;
     }
 
-    public string TargetAssetPath => _tempDirectory.Path;
+    public string Name { get; }
+
+    public string TargetAssetPath { get; }
 
     public DotnetMuxerResult? DotnetResult { get; internal set; }
 
@@ -38,7 +53,7 @@ protected virtual void Dispose(bool disposing)
         {
             if (DotnetResult is null || DotnetResult.ExitCode == 0)
             {
-                _tempDirectory.Dispose();
+                _tempDirectory?.Dispose();
             }
         }
 
@@ -65,7 +80,20 @@ public static async Task<TestAsset> GenerateAssetAsync(string assetName, string
         foreach (string fileContent in splitFiles)
         {
             (string, string) fileInfo = ParseFile(fileContent);
-            await TempDirectory.WriteFileAsync(testAsset._tempDirectory.Path, fileInfo.Item1, fileInfo.Item2);
+            await TempDirectory.WriteFileAsync(testAsset.TargetAssetPath, fileInfo.Item1, fileInfo.Item2);
+        }
+
+        return testAsset;
+    }
+
+    public static async Task<TestAsset> GenerateAssetAsync(string assetName, string code, TempDirectory tempDirectory, bool addDefaultNuGetConfigFile = true, bool addPublicFeeds = false)
+    {
+        TestAsset testAsset = new(assetName, addDefaultNuGetConfigFile ? string.Concat(code, GetNuGetConfig(addPublicFeeds)) : code, tempDirectory);
+        string[] splitFiles = testAsset._assetCode.Split([FileTag], StringSplitOptions.RemoveEmptyEntries);
+        foreach (string fileContent in splitFiles)
+        {
+            (string, string) fileInfo = ParseFile(fileContent);
+            await TempDirectory.WriteFileAsync(testAsset.TargetAssetPath, fileInfo.Item1, fileInfo.Item2);
         }
 
         return testAsset;