diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/ClassCleanupManager.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/ClassCleanupManager.cs index 7a952b0b61..750fd6a2ee 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/ClassCleanupManager.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/ClassCleanupManager.cs @@ -24,26 +24,31 @@ public ClassCleanupManager(IEnumerable testsToRun) public bool ShouldRunEndOfAssemblyCleanup => _remainingTestCountsByClass.IsEmpty; - public void MarkTestComplete(TestMethodInfo testMethodInfo, out bool shouldRunEndOfClassCleanup) + public void MarkTestComplete(TestMethod testMethod, out bool isLastTestInClass) { - shouldRunEndOfClassCleanup = false; - lock (_remainingTestCountsByClass) { - if (!_remainingTestCountsByClass.TryGetValue(testMethodInfo.TestClassName, out int remainingCount)) + if (!_remainingTestCountsByClass.TryGetValue(testMethod.FullClassName, out int remainingCount)) { - return; + throw ApplicationStateGuard.Unreachable(); } remainingCount--; - _remainingTestCountsByClass[testMethodInfo.TestClassName] = remainingCount; - if (remainingCount == 0) + _remainingTestCountsByClass[testMethod.FullClassName] = remainingCount; + isLastTestInClass = remainingCount == 0; + } + } + + public void MarkClassComplete(string fullClassName) + { + lock (_remainingTestCountsByClass) + { + if (!_remainingTestCountsByClass.TryRemove(fullClassName, out int remainingTests) || + remainingTests != 0) { - _remainingTestCountsByClass.TryRemove(testMethodInfo.TestClassName, out _); - if (testMethodInfo.Parent.HasExecutableCleanupMethod) - { - shouldRunEndOfClassCleanup = true; - } + // We failed to remove the class, or we are incorrectly marking the class as complete while there are remaining tests. + // This should never happen. + throw ApplicationStateGuard.Unreachable(); } } } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs index 9acd31c7f9..6689d8cb5f 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs @@ -620,20 +620,9 @@ async Task DoRunAsync() return testFailedException; } - internal async Task RunClassCleanupAsync(ITestContext testContext, ClassCleanupManager classCleanupManager, TestMethodInfo testMethodInfo, TestResult[] results) + internal async Task RunClassCleanupAsync(ITestContext testContext, TestResult[] results) { - DebugEx.Assert(testMethodInfo.Parent == this, "Parent of testMethodInfo should be this TestClassInfo."); - - classCleanupManager.MarkTestComplete(testMethodInfo, out bool shouldRunEndOfClassCleanup); - if (!shouldRunEndOfClassCleanup) - { - return; - } - - // TODO: Looks like 'ClassCleanupMethod is null && BaseClassCleanupMethods.Count == 0' is always false? - // shouldRunEndOfClassCleanup should be false if there are no class cleanup methods at all. - if ((ClassCleanupMethod is null && BaseClassCleanupMethods.Count == 0) - || IsClassCleanupExecuted) + if (!HasExecutableCleanupMethod || IsClassCleanupExecuted) { // DoRun will already do nothing for this condition. So, we gain a bit of performance. return; diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs index 3077192c56..3e2ddda049 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs @@ -204,15 +204,23 @@ internal async Task RunSingleTestAsync(TestMethod testMethod, IDic } testContextForClassCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, testMethod.FullClassName, testContextProperties, messageLogger, testContextForTestExecution.Context.CurrentTestOutcome); - if (testMethodInfo is not null) + + _classCleanupManager.MarkTestComplete(testMethod, out bool isLastTestInClass); + if (isLastTestInClass && testMethodInfo is not null) { - await testMethodInfo.Parent.RunClassCleanupAsync(testContextForClassCleanup, _classCleanupManager, testMethodInfo, result).ConfigureAwait(false); + await testMethodInfo.Parent.RunClassCleanupAsync(testContextForClassCleanup, result).ConfigureAwait(false); + + // Mark the class as complete when all class cleanups are complete. When all classes are complete we progress to running assembly cleanup. + // Class is not complete until after all class cleanups are done, to prevent running assembly cleanup too early. + // Do not mark the class as complete when the last test method in the class completed. That is too early, we need to run class cleanups before marking class as complete. + _classCleanupManager.MarkClassComplete(testMethod.FullClassName); } - if (testMethodInfo?.Parent.Parent.IsAssemblyInitializeExecuted == true) + if (testMethodInfo?.Parent.Parent.IsAssemblyInitializeExecuted == true && + _classCleanupManager.ShouldRunEndOfAssemblyCleanup) { testContextForAssemblyCleanup = PlatformServiceProvider.Instance.GetTestContext(testMethod: null, null, testContextProperties, messageLogger, testContextForClassCleanup.Context.CurrentTestOutcome); - await RunAssemblyCleanupIfNeededAsync(testContextForAssemblyCleanup, _classCleanupManager, _typeCache, result).ConfigureAwait(false); + await RunAssemblyCleanupAsync(testContextForAssemblyCleanup, _typeCache, result).ConfigureAwait(false); } return result; @@ -268,13 +276,8 @@ private static async Task RunAssemblyInitializeIfNeededAsync(TestMet return result; } - private static async Task RunAssemblyCleanupIfNeededAsync(ITestContext testContext, ClassCleanupManager classCleanupManager, TypeCache typeCache, TestResult[] results) + private static async Task RunAssemblyCleanupAsync(ITestContext testContext, TypeCache typeCache, TestResult[] results) { - if (!classCleanupManager.ShouldRunEndOfAssemblyCleanup) - { - return; - } - try { IEnumerable assemblyInfoCache = typeCache.AssemblyInfoListWithExecutableCleanupMethods; diff --git a/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AssemblyCleanupTests.cs b/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AssemblyCleanupTests.cs new file mode 100644 index 0000000000..b68db53c81 --- /dev/null +++ b/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AssemblyCleanupTests.cs @@ -0,0 +1,127 @@ +// 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 sealed class AssemblyCleanupTests : AcceptanceTestBase +{ + [TestMethod] + public async Task AssemblyCleanupShouldRunAfterAllClassCleanupsHaveCompleted() + { + var testHost = TestHost.LocateFrom(AssetFixture.ProjectPath, TestAssetFixture.ProjectName, TargetFrameworks.NetCurrent); + TestHostResult testHostResult = await testHost.ExecuteAsync("--settings my.runsettings", cancellationToken: TestContext.CancellationToken); + + testHostResult.AssertExitCodeIs(ExitCodes.Success); + testHostResult.AssertOutputContainsSummary(failed: 0, passed: 2, skipped: 0); + testHostResult.AssertOutputContains(""" + TestClass1.Test1. + TestClass1.Cleanup1 started. + TestClass1.Cleanup1 finished. + In AsmCleanup + """); + } + + public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.NuGetGlobalPackagesFolder) + { + public const string ProjectName = "AssemblyCleanupTests"; + + public string ProjectPath => GetAssetPath(ProjectName); + + public override IEnumerable<(string ID, string Name, string Code)> GetAssetsToGenerate() + { + yield return (ProjectName, ProjectName, + SourceCode + .PatchTargetFrameworks(TargetFrameworks.All) + .PatchCodeWithReplace("$MSTestVersion$", MSTestVersion)); + } + + private const string SourceCode = """ +#file AssemblyCleanupTests.csproj + + + + Exe + true + $TargetFrameworks$ + preview + + + + + + + + + + PreserveNewest + + + + + +#file TestClass1.cs +using System; +using System.Collections.Generic; +using System.Text; +using System.Threading; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)] + +[TestClass] +public class TestClass1 +{ + public static bool ClassCleanupFinished { get; private set; } + + [TestMethod] + public void Test1() + { + Console.WriteLine("TestClass1.Test1."); + } + + [ClassCleanup] + public static void Cleanup1() + { + Console.WriteLine("TestClass1.Cleanup1 started."); + Thread.Sleep(4000); + Console.WriteLine("TestClass1.Cleanup1 finished."); + } +} + +[TestClass] +public class TestClass2 +{ + [TestMethod] + public void Test2() + { + } + + [ClassCleanup] + public static void Cleanup2() + => Thread.Sleep(2000); +} + +[TestClass] +public static class Asm +{ + [AssemblyCleanup] + public static void AsmCleanup() + => Console.WriteLine("In AsmCleanup"); +} + +#file my.runsettings + + + false + + +"""; + } + + public TestContext TestContext { get; set; } +} diff --git a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/ClassCleanupManagerTests.cs b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/ClassCleanupManagerTests.cs index 2ff33df6a7..3d2211b3fe 100644 --- a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/ClassCleanupManagerTests.cs +++ b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/ClassCleanupManagerTests.cs @@ -18,10 +18,9 @@ public class ClassCleanupManagerTests : TestContainer public void AssemblyCleanupRunsAfterAllTestsFinishEvenIfWeScheduleTheSameTestMultipleTime() { ReflectHelper reflectHelper = Mock.Of(); - MethodInfo methodInfo = typeof(FakeTestClass).GetMethod(nameof(FakeTestClass.FakeTestMethod), BindingFlags.Instance | BindingFlags.NonPublic)!; MethodInfo classCleanupMethodInfo = typeof(FakeTestClass).GetMethod(nameof(FakeTestClass.FakeClassCleanupMethod), BindingFlags.Instance | BindingFlags.NonPublic)!; // Full class name must agree between unitTestElement.TestMethod.FullClassName and testMethod.FullClassName; - string fullClassName = methodInfo.DeclaringType!.FullName!; + string fullClassName = typeof(FakeTestClass).FullName!; TestMethod testMethod = new(nameof(FakeTestClass.FakeTestMethod), fullClassName, typeof(FakeTestClass).Assembly.FullName!, displayName: null); // Setting 2 of the same test to run, we should run assembly cleanup after both these tests @@ -39,14 +38,14 @@ public void AssemblyCleanupRunsAfterAllTestsFinishEvenIfWeScheduleTheSameTestMul // This needs to be set, to allow running class cleanup. ClassCleanupMethod = classCleanupMethodInfo, }; - TestMethodInfo testMethodInfo = new(methodInfo, testClassInfo, null!); - classCleanupManager.MarkTestComplete(testMethodInfo, out bool shouldRunEndOfClassCleanup); + classCleanupManager.MarkTestComplete(testMethod, out bool shouldRunEndOfClassCleanup); // The cleanup should not run here yet, we have 1 remaining test to run. shouldRunEndOfClassCleanup.Should().BeFalse(); classCleanupManager.ShouldRunEndOfAssemblyCleanup.Should().BeFalse(); - classCleanupManager.MarkTestComplete(testMethodInfo, out shouldRunEndOfClassCleanup); + classCleanupManager.MarkTestComplete(testMethod, out shouldRunEndOfClassCleanup); + classCleanupManager.MarkClassComplete(fullClassName); // The cleanup should run here. shouldRunEndOfClassCleanup.Should().BeTrue(); classCleanupManager.ShouldRunEndOfAssemblyCleanup.Should().BeTrue(); diff --git a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/UnitTestRunnerTests.cs b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/UnitTestRunnerTests.cs index 3c8903d8de..4b268870c9 100644 --- a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/UnitTestRunnerTests.cs +++ b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/UnitTestRunnerTests.cs @@ -24,21 +24,20 @@ public sealed class UnitTestRunnerTests : TestContainer private readonly TestablePlatformServiceProvider _testablePlatformServiceProvider; private readonly Mock _mockMessageLogger; - private UnitTestRunner _unitTestRunner; - public UnitTestRunnerTests() { _testRunParameters = []; _testablePlatformServiceProvider = new TestablePlatformServiceProvider(); _mockMessageLogger = new Mock(); PlatformServiceProvider.Instance = _testablePlatformServiceProvider; - - _unitTestRunner = new UnitTestRunner(GetSettingsWithDebugTrace(false), []); } private TestMethod CreateTestMethod(string methodName, string typeFullName, string assemblyName, string? displayName) => new(typeFullName, methodName, null, methodName, typeFullName, assemblyName, displayName, null); + private UnitTestRunner CreateUnitTestRunner(UnitTestElement[] testsToRun) + => new(GetSettingsWithDebugTrace(false), testsToRun); + protected override void Dispose(bool disposing) { if (!IsDisposed) @@ -80,14 +79,16 @@ public void ConstructorShouldPopulateSettings() public async Task RunSingleTestShouldThrowIfTestMethodIsNull() { - Func func = () => _unitTestRunner.RunSingleTestAsync(null!, null!, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([]); + Func func = () => unitTestRunner.RunSingleTestAsync(null!, null!, null!); await func.Should().ThrowAsync(); } public async Task RunSingleTestShouldThrowIfTestRunParametersIsNull() { TestMethod testMethod = CreateTestMethod("M", "C", "A", displayName: null); - Func func = () => _unitTestRunner.RunSingleTestAsync(testMethod, null!, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + Func func = () => unitTestRunner.RunSingleTestAsync(testMethod, null!, null!); await func.Should().ThrowAsync(); } @@ -98,7 +99,8 @@ public async Task RunSingleTestShouldReturnTestResultIndicateATestNotFoundIfTest _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -115,7 +117,8 @@ public async Task RunSingleTestShouldReturnTestResultIndicatingNotRunnableTestIf _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); string expectedMessage = string.Format( CultureInfo.InvariantCulture, @@ -138,7 +141,8 @@ public async Task ExecuteShouldSkipTestAndFillInClassIgnoreMessageIfIgnoreAttrib _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -155,7 +159,8 @@ public async Task ExecuteShouldSkipTestAndSkipFillingIgnoreMessageIfIgnoreAttrib _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -172,7 +177,8 @@ public async Task ExecuteShouldSkipTestAndFillInMethodIgnoreMessageIfIgnoreAttri _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -189,7 +195,8 @@ public async Task ExecuteShouldSkipTestAndSkipFillingIgnoreMessageIfIgnoreAttrib _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -206,7 +213,8 @@ public async Task ExecuteShouldSkipTestAndFillInClassIgnoreMessageIfIgnoreAttrib _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -223,7 +231,8 @@ public async Task ExecuteShouldSkipTestAndFillInMethodIgnoreMessageIfIgnoreAttri _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -239,7 +248,8 @@ public async Task RunSingleTestShouldReturnTestResultIndicatingFailureIfThereIsA _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); string expectedMessage = string.Format( CultureInfo.InvariantCulture, @@ -262,7 +272,8 @@ public async Task RunSingleTestShouldReturnTestResultsForAPassingTestMethod() _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -280,7 +291,8 @@ public async Task RunSingleTestShouldSetTestsAsInProgressInTestContext() .Returns(Assembly.GetExecutingAssembly()); // Asserting in the test method execution flow itself. - TestResult[] results = await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + UnitTestRunner unitTestRunner = CreateUnitTestRunner([new UnitTestElement(testMethod)]); + TestResult[] results = await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); results.Should().NotBeNull(); results.Length.Should().Be(1); @@ -290,11 +302,11 @@ public async Task RunSingleTestShouldSetTestsAsInProgressInTestContext() public async Task RunSingleTestShouldCallAssemblyInitializeAndClassInitializeMethodsInOrder() { var mockReflectHelper = new Mock(); - _unitTestRunner = new UnitTestRunner(new MSTestSettings(), [], mockReflectHelper.Object); Type type = typeof(DummyTestClassWithInitializeMethods); MethodInfo methodInfo = type.GetMethod("TestMethod")!; TestMethod testMethod = CreateTestMethod(methodInfo.Name, type.FullName!, "A", displayName: null); + var unitTestRunner = new UnitTestRunner(new MSTestSettings(), [new UnitTestElement(testMethod)], mockReflectHelper.Object); _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A")) .Returns(Assembly.GetExecutingAssembly()); @@ -306,7 +318,7 @@ public async Task RunSingleTestShouldCallAssemblyInitializeAndClassInitializeMet DummyTestClassWithInitializeMethods.AssemblyInitializeMethodBody = () => validator <<= 2; DummyTestClassWithInitializeMethods.ClassInitializeMethodBody = () => validator >>= 2; - await _unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); + await unitTestRunner.RunSingleTestAsync(testMethod, _testRunParameters, null!); validator.Should().Be(1); }