Skip to content

Commit b1ee076

Browse files
nohwndnohwnd
authored
Write warnings outside of appdomain (#5371)
1 parent e45f7a5 commit b1ee076

8 files changed

Lines changed: 242 additions & 38 deletions

File tree

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel;
5+
6+
namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery;
7+
8+
/// <summary>
9+
/// Helps us communicate results that were created inside of AppDomain, when AppDomains are available and enabled.
10+
/// </summary>
11+
/// <param name="TestElements">The test elements that were discovered.</param>
12+
/// <param name="Warnings">Warnings that happened during discovery.</param>
13+
[Serializable]
14+
internal sealed record AssemblyEnumerationResult(List<UnitTestElement> TestElements, List<string> Warnings);

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,10 @@ public AssemblyEnumerator(MSTestSettings settings) =>
6262
/// Enumerates through all types in the assembly in search of valid test methods.
6363
/// </summary>
6464
/// <param name="assemblyFileName">The assembly file name.</param>
65-
/// <param name="warnings">Contains warnings if any, that need to be passed back to the caller.</param>
6665
/// <returns>A collection of Test Elements.</returns>
67-
internal ICollection<UnitTestElement> EnumerateAssembly(
68-
string assemblyFileName,
69-
List<string> warnings)
66+
internal AssemblyEnumerationResult EnumerateAssembly(string assemblyFileName)
7067
{
68+
List<string> warnings = new();
7169
DebugEx.Assert(!StringEx.IsNullOrWhiteSpace(assemblyFileName), "Invalid assembly file name.");
7270
var tests = new List<UnitTestElement>();
7371
// Contains list of assembly/class names for which we have already added fixture tests.
@@ -117,7 +115,7 @@ internal ICollection<UnitTestElement> EnumerateAssembly(
117115
tests.AddRange(testsInType);
118116
}
119117

120-
return tests;
118+
return new AssemblyEnumerationResult(tests, warnings);
121119
}
122120

123121
/// <summary>

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ internal sealed class AssemblyEnumeratorWrapper
4949
}
5050

5151
// Load the assembly in isolation if required.
52-
return GetTestsInIsolation(fullFilePath, runSettings, warnings);
52+
AssemblyEnumerationResult result = GetTestsInIsolation(fullFilePath, runSettings);
53+
warnings.AddRange(result.Warnings);
54+
return result.TestElements;
5355
}
5456
catch (FileNotFoundException ex)
5557
{
@@ -95,7 +97,7 @@ internal sealed class AssemblyEnumeratorWrapper
9597
}
9698
}
9799

98-
private static ICollection<UnitTestElement> GetTestsInIsolation(string fullFilePath, IRunSettings? runSettings, List<string> warnings)
100+
private static AssemblyEnumerationResult GetTestsInIsolation(string fullFilePath, IRunSettings? runSettings)
99101
{
100102
using MSTestAdapter.PlatformServices.Interface.ITestSourceHost isolationHost = PlatformServiceProvider.Instance.CreateTestSourceHost(fullFilePath, runSettings, frameworkHandle: null);
101103

@@ -114,6 +116,10 @@ private static ICollection<UnitTestElement> GetTestsInIsolation(string fullFileP
114116
PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning(Resource.OlderTFMVersionFound);
115117
}
116118

117-
return assemblyEnumerator.EnumerateAssembly(fullFilePath, warnings);
119+
// This method runs inside of appdomain, when appdomains are available and enabled.
120+
// Be careful how you pass data from the method. We were previously passing in a collection
121+
// of strings normally (by reference), and we were mutating that collection in the appdomain.
122+
// But this does not mutate the collection outside of appdomain, so we lost all warnings that happened inside.
123+
return assemblyEnumerator.EnumerateAssembly(fullFilePath);
118124
}
119125
}

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

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using Microsoft.Testing.Platform.Acceptance.IntegrationTests;
5+
using Microsoft.Testing.Platform.Acceptance.IntegrationTests.Helpers;
6+
using Microsoft.Testing.Platform.Helpers;
7+
8+
namespace MSTest.Acceptance.IntegrationTests;
9+
10+
[TestClass]
11+
public class TestDiscoveryWarningsTests : AcceptanceTestBase<TestDiscoveryWarningsTests.TestAssetFixture>
12+
{
13+
private const string AssetName = "TestDiscoveryWarnings";
14+
private const string BaseClassAssetName = "TestDiscoveryWarningsBaseClass";
15+
16+
[TestMethod]
17+
[DynamicData(nameof(TargetFrameworks.AllForDynamicData), typeof(TargetFrameworks))]
18+
public async Task DiscoverTests_ShowsWarningsForTestsThatFailedToDiscover(string currentTfm)
19+
{
20+
var testHost = TestHost.LocateFrom(AssetFixture.TargetAssetPath, AssetName, currentTfm);
21+
22+
if (currentTfm.StartsWith("net4", StringComparison.OrdinalIgnoreCase))
23+
{
24+
// .NET Framework will isolate the run into appdomain, there we did not write the warnings out
25+
// so before running the discovery, we want to ensure that the tests do run in appdomain.
26+
// We check for appdomain directly in the test, so if tests fail we did not run in appdomain.
27+
TestHostResult testHostSuccessResult = await testHost.ExecuteAsync();
28+
29+
testHostSuccessResult.AssertExitCodeIs(ExitCodes.Success);
30+
}
31+
32+
// Delete the TestDiscoveryWarningsBaseClass.dll from the test bin folder on purpose, to break discovering
33+
// because the type won't be loaded on runtime, and mstest will write warning.
34+
File.Delete(Path.Combine(testHost.DirectoryName, $"{BaseClassAssetName}.dll"));
35+
36+
TestHostResult testHostResult = await testHost.ExecuteAsync("--list-tests");
37+
38+
testHostResult.AssertExitCodeIsNot(ExitCodes.Success);
39+
testHostResult.AssertOutputContains("System.IO.FileNotFoundException: Could not load file or assembly 'TestDiscoveryWarningsBaseClass");
40+
}
41+
42+
public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.NuGetGlobalPackagesFolder)
43+
{
44+
public string TargetAssetPath => GetAssetPath(AssetName);
45+
46+
public string BaseTargetAssetPath => GetAssetPath(BaseClassAssetName);
47+
48+
public override IEnumerable<(string ID, string Name, string Code)> GetAssetsToGenerate()
49+
{
50+
yield return (BaseClassAssetName, BaseClassAssetName,
51+
BaseClassSourceCode.PatchTargetFrameworks(TargetFrameworks.All));
52+
53+
yield return (AssetName, AssetName,
54+
SourceCode.PatchTargetFrameworks(TargetFrameworks.All)
55+
.PatchCodeWithReplace("$MSTestVersion$", MSTestVersion));
56+
}
57+
58+
private const string SourceCode = """
59+
#file TestDiscoveryWarnings.csproj
60+
<Project Sdk="Microsoft.NET.Sdk">
61+
62+
<PropertyGroup>
63+
<OutputType>Exe</OutputType>
64+
<EnableMSTestRunner>true</EnableMSTestRunner>
65+
<TargetFrameworks>$TargetFrameworks$</TargetFrameworks>
66+
<LangVersion>latest</LangVersion>
67+
</PropertyGroup>
68+
69+
<ItemGroup>
70+
<ProjectReference Include="../TestDiscoveryWarningsBaseClass/TestDiscoveryWarningsBaseClass.csproj" />
71+
</ItemGroup>
72+
<ItemGroup>
73+
<PackageReference Include="MSTest.TestAdapter" Version="$MSTestVersion$" />
74+
<PackageReference Include="MSTest.TestFramework" Version="$MSTestVersion$" />
75+
</ItemGroup>
76+
77+
</Project>
78+
79+
#file UnitTest1.cs
80+
81+
using Base;
82+
83+
using System;
84+
using System.Threading.Tasks;
85+
using Microsoft.VisualStudio.TestTools.UnitTesting;
86+
87+
[TestClass]
88+
public class TestClass1 : BaseClass
89+
{
90+
[TestMethod]
91+
public void Test1_1()
92+
{
93+
#if NETFRAMEWORK
94+
// Ensure we run in appdomain, and not directly in host, because we want to ensure that warnings are correctly passed
95+
// outside of the appdomain to the rest of the engine.
96+
//\
97+
// We set this friendly appdomain name in src\Adapter\MSTestAdapter.PlatformServices\Services\TestSourceHost.cs:163
98+
StringAssert.StartsWith(AppDomain.CurrentDomain.FriendlyName, "TestSourceHost: Enumerating source");
99+
#endif
100+
}
101+
}
102+
103+
[TestClass]
104+
public class TestClass2
105+
{
106+
[TestMethod]
107+
public void Test2_1() {}
108+
}
109+
""";
110+
111+
private const string BaseClassSourceCode = """
112+
#file TestDiscoveryWarningsBaseClass.csproj
113+
<Project Sdk="Microsoft.NET.Sdk">
114+
115+
<PropertyGroup>
116+
<TargetFrameworks>$TargetFrameworks$</TargetFrameworks>
117+
<IsPackable>false</IsPackable>
118+
<LangVersion>latest</LangVersion>
119+
</PropertyGroup>
120+
121+
</Project>
122+
123+
124+
#file UnitTest1.cs
125+
namespace Base;
126+
127+
public class BaseClass
128+
{
129+
}
130+
""";
131+
}
132+
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ public async Task LifecycleAttributesTaskThreading_WhenMainIsNotSTA_RunsettingsA
188188
return;
189189
}
190190

191-
var testHost = TestHost.LocateFrom(AssetFixture.LifecycleAttributesTaskProjectPath, TestAssetFixture.LifecycleWithParallelAttributesTaskProjectName, tfm);
191+
var testHost = TestHost.LocateFrom(AssetFixture.LifecycleWithParallelAttributesTaskProjectNamePath, TestAssetFixture.LifecycleAttributesTaskProjectName, tfm);
192192
string runSettingsFilePath = Path.Combine(testHost.DirectoryName, "sta.runsettings");
193193
TestHostResult testHostResult = await testHost.ExecuteAsync($"--settings {runSettingsFilePath}", environmentVariables: new()
194194
{
@@ -225,7 +225,7 @@ public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.
225225
public const string STAThreadProjectName = "STATestThreading";
226226
public const string LifecycleAttributesVoidProjectName = "LifecycleAttributesVoid";
227227
public const string LifecycleAttributesTaskProjectName = "LifecycleAttributesTask";
228-
public const string LifecycleWithParallelAttributesTaskProjectName = "LifecycleAttributesTask";
228+
public const string LifecycleWithParallelAttributesTaskProjectName = "LifecycleWithParallelAttributesTask";
229229
public const string LifecycleAttributesValueTaskProjectName = "LifecycleAttributesValueTask";
230230

231231
public string ProjectPath => GetAssetPath(ProjectName);
@@ -267,7 +267,7 @@ public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.
267267
.PatchCodeWithReplace("$ParallelAttribute$", string.Empty)
268268
.PatchCodeWithReplace("$MSTestVersion$", MSTestVersion));
269269

270-
yield return (LifecycleWithParallelAttributesTaskProjectName, LifecycleWithParallelAttributesTaskProjectName,
270+
yield return (LifecycleWithParallelAttributesTaskProjectName, LifecycleAttributesTaskProjectName,
271271
LifecycleAttributesTaskSource
272272
.PatchTargetFrameworks(TargetFrameworks.All)
273273
.PatchCodeWithReplace("$ParallelAttribute$", "[assembly: Parallelize(Workers = 0, Scope = ExecutionScope.MethodLevel)]")

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

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,9 @@ public void EnumerateAssemblyShouldReturnEmptyListWhenNoDeclaredTypes()
217217
_testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("DummyAssembly", false))
218218
.Returns(mockAssembly.Object);
219219

220-
Verify(_assemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings).Count == 0);
220+
AssemblyEnumerationResult result = _assemblyEnumerator.EnumerateAssembly("DummyAssembly");
221+
_warnings.AddRange(result.Warnings);
222+
Verify(result.TestElements.Count == 0);
221223
}
222224

223225
public void EnumerateAssemblyShouldReturnEmptyListWhenNoTestElementsInAType()
@@ -235,7 +237,9 @@ public void EnumerateAssemblyShouldReturnEmptyListWhenNoTestElementsInAType()
235237
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
236238
.Returns((List<UnitTestElement>)null!);
237239

238-
Verify(_assemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings).Count == 0);
240+
AssemblyEnumerationResult result = _assemblyEnumerator.EnumerateAssembly("DummyAssembly");
241+
_warnings.AddRange(result.Warnings);
242+
Verify(result.TestElements.Count == 0);
239243
}
240244

241245
public void EnumerateAssemblyShouldReturnTestElementsForAType()
@@ -254,9 +258,10 @@ public void EnumerateAssemblyShouldReturnTestElementsForAType()
254258
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
255259
.Returns(new List<UnitTestElement> { unitTestElement });
256260

257-
ICollection<UnitTestElement> testElements = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
261+
AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
262+
_warnings.AddRange(result.Warnings);
258263

259-
Verify(new Collection<UnitTestElement> { unitTestElement }.SequenceEqual(testElements));
264+
Verify(new Collection<UnitTestElement> { unitTestElement }.SequenceEqual(result.TestElements));
260265
}
261266

262267
public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForAType()
@@ -276,9 +281,10 @@ public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForAType()
276281
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
277282
.Returns(expectedTestElements);
278283

279-
ICollection<UnitTestElement> testElements = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
284+
AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
285+
_warnings.AddRange(result.Warnings);
280286

281-
Verify(expectedTestElements.SequenceEqual(testElements));
287+
Verify(expectedTestElements.SequenceEqual(result.TestElements));
282288
}
283289

284290
public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForMoreThanOneType()
@@ -298,11 +304,12 @@ public void EnumerateAssemblyShouldReturnMoreThanOneTestElementForMoreThanOneTyp
298304
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
299305
.Returns(expectedTestElements);
300306

301-
ICollection<UnitTestElement> testElements = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
307+
AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
308+
_warnings.AddRange(result.Warnings);
302309

303310
expectedTestElements.Add(unitTestElement);
304311
expectedTestElements.Add(unitTestElement);
305-
Verify(expectedTestElements.SequenceEqual(testElements));
312+
Verify(expectedTestElements.SequenceEqual(result.TestElements));
306313
}
307314

308315
public void EnumerateAssemblyShouldNotLogWarningsIfNonePresent()
@@ -320,8 +327,9 @@ public void EnumerateAssemblyShouldNotLogWarningsIfNonePresent()
320327
.Returns(mockAssembly.Object);
321328
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(warningsFromTypeEnumerator));
322329

323-
testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
324-
Verify(_warnings.Count == 0);
330+
AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
331+
_warnings.AddRange(result.Warnings);
332+
Verify(result.Warnings.Count == 0);
325333
}
326334

327335
public void EnumerateAssemblyShouldLogWarningsIfPresent()
@@ -340,12 +348,12 @@ public void EnumerateAssemblyShouldLogWarningsIfPresent()
340348
.Returns([typeof(InternalTestClass).GetTypeInfo()]);
341349
_testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("DummyAssembly", false))
342350
.Returns(mockAssembly.Object);
343-
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings))
344-
.Callback(() => _warnings.AddRange(warningsFromTypeEnumerator));
351+
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(It.IsAny<List<string>>()))
352+
.Callback<List<string>>((w) => w.AddRange(warningsFromTypeEnumerator));
345353

346-
testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
354+
AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
347355

348-
Verify(warningsFromTypeEnumerator.SequenceEqual(_warnings));
356+
Verify(warningsFromTypeEnumerator.SequenceEqual(result.Warnings));
349357
}
350358

351359
public void EnumerateAssemblyShouldHandleExceptionsWhileEnumeratingAType()
@@ -363,9 +371,10 @@ public void EnumerateAssemblyShouldHandleExceptionsWhileEnumeratingAType()
363371
.Returns(mockAssembly.Object);
364372
testableAssemblyEnumerator.MockTypeEnumerator.Setup(te => te.Enumerate(_warnings)).Throws(exception);
365373

366-
testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly", _warnings);
374+
AssemblyEnumerationResult result = testableAssemblyEnumerator.EnumerateAssembly("DummyAssembly");
375+
_warnings.AddRange(result.Warnings);
367376

368-
Verify(_warnings.ToList().Contains(
377+
Verify(result.Warnings.Contains(
369378
string.Format(
370379
CultureInfo.CurrentCulture,
371380
Resource.CouldNotInspectTypeDuringDiscovery,

0 commit comments

Comments
 (0)