diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs index 4e8067a473..4134fffc51 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs @@ -85,8 +85,19 @@ internal set /// /// Gets or sets a value indicating whether AssemblyInitialize has been executed. + /// + /// Reads and writes use because this flag acts as the fast-path + /// guard that lets callers bypass . The + /// release semantics on the publishing write ensure that the prior + /// snapshot publication is also visible to any + /// reader that observes this flag as on the fast path. + /// /// - public bool IsAssemblyInitializeExecuted { get; internal set; } + public bool IsAssemblyInitializeExecuted + { + get => Volatile.Read(ref field); + internal set => Volatile.Write(ref field, value); + } /// /// Gets or sets the assembly initialization exception. @@ -110,8 +121,22 @@ internal set /// assembly-cleanup context, because AssemblyCleanup is assembly-scoped and runs /// once across many classes; including a single class's snapshot would be arbitrary. /// + /// + /// Reads and writes use so that callers on the + /// fast path (which intentionally bypasses + /// ) safely observe the snapshot published + /// by the thread that ran AssemblyInitialize. The publishing thread writes this + /// snapshot before writing , and both writes go + /// through , so any reader that observes + /// as is guaranteed to + /// also see the published snapshot. + /// /// - internal IReadOnlyDictionary? PostAssemblyInitProperties { get; private set; } + internal IReadOnlyDictionary? PostAssemblyInitProperties + { + get => Volatile.Read(ref field); + private set => Volatile.Write(ref field, value); + } /// /// Gets the assembly cleanup exception. @@ -190,13 +215,10 @@ public async Task RunAssemblyInitializeAsync(TestContext testContext // Capture a snapshot of TestContext.Properties so that values // set during AssemblyInitialize flow to subsequent contexts // (class init, test execution, class cleanup, assembly cleanup). - // TODO: PostAssemblyInitProperties is published outside the - // _assemblyInfoExecuteSyncSemaphore via the - // IsAssemblyInitializeExecuted fast path in this method. This - // is consistent with the existing pattern used by - // AssemblyInitializationException and ExecutionContext; - // revisit memory-barrier semantics for all three together - // if it becomes a problem. + // PostAssemblyInitProperties uses Volatile.Read/Write so that + // callers on the IsAssemblyInitializeExecuted fast path + // (which bypasses _assemblyInfoExecuteSyncSemaphore) safely + // observe the published snapshot. PostAssemblyInitProperties = testContextImpl.CaptureLifecycleProperties(); } }, diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs index c9e0190a0b..ce840a4190 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs @@ -159,8 +159,22 @@ internal set /// The snapshot is shallow: reference-type values stored in the bag are shared (aliased) /// across every context the snapshot is merged into. /// + /// + /// Reads and writes use so that callers on the cached-result fast + /// path of (which intentionally bypasses + /// ) safely observe the snapshot published by + /// the thread that ran ClassInitialize. The publishing thread writes this snapshot + /// before publishing _classInitializeResult, and both writes go through + /// , so any reader that observes the cached result via + /// is guaranteed to also see the + /// published snapshot. + /// /// - internal IReadOnlyDictionary? PostClassInitProperties { get; private set; } + internal IReadOnlyDictionary? PostClassInitProperties + { + get => Volatile.Read(ref field); + private set => Volatile.Write(ref field, value); + } /// /// Gets or sets the class cleanup method. @@ -334,20 +348,26 @@ internal async Task RunClassInitializeAsync(TestContext testContext) } private TestResult? TryGetClonedCachedClassInitializeResult() + { // Historically, we were not caching class initialize result, and were always going through the logic in GetResultOrRunClassInitialize. // When caching is introduced, we found out that using the cached instance can change the behavior in some cases. For example, // if you have Console.WriteLine in class initialize, those will be present on the TestResult. // Before caching was introduced, these logs will be only in the first class initialize result (attached to the first test run in class) // By re-using the cached instance, it's now part of all tests. // To preserve the original behavior, we clone the cached instance so we keep only the information we are sure should be reused. - => _classInitializeResult is null + // Volatile.Read pairs with the Volatile.Write performed when the result is published, so + // that fast-path readers (which bypass _testClassExecuteSyncSemaphore) also observe the + // PostClassInitProperties snapshot written before the result on the publishing thread. + TestResult? cached = Volatile.Read(ref _classInitializeResult); + return cached is null ? null : new() { - Outcome = _classInitializeResult.Outcome, - IgnoreReason = _classInitializeResult.IgnoreReason, - TestFailureException = _classInitializeResult.TestFailureException, + Outcome = cached.Outcome, + IgnoreReason = cached.IgnoreReason, + TestFailureException = cached.TestFailureException, }; + } internal async Task GetResultOrRunClassInitializeAsync(ITestContext testContext, string? initializationLogs, string? initializationErrorLogs, string? initializationTrace, string? initializationTestContextMessages) { @@ -366,7 +386,9 @@ internal async Task GetResultOrRunClassInitializeAsync(ITestContext if (ClassInitializeMethod is null && BaseClassInitMethods.Count == 0) { IsClassInitializeExecuted = true; - return _classInitializeResult = new() { Outcome = UnitTestOutcome.Passed }; + var emptyResult = new TestResult { Outcome = UnitTestOutcome.Passed }; + Volatile.Write(ref _classInitializeResult, emptyResult); + return emptyResult; } // At this point, maybe class initialize was executed by another thread such @@ -495,7 +517,12 @@ async Task DoRunAsync() result.TestContextMessages = initializationTestContextMessages + testContext.GetAndClearDiagnosticMessages(); } - _classInitializeResult = result; + // Publish with Volatile.Write so callers on the cached-result fast path of + // GetResultOrRunClassInitializeAsync (which bypasses _testClassExecuteSyncSemaphore) + // safely observe the prior PostClassInitProperties snapshot publication: the + // release semantics ensure the snapshot write is visible before this assignment + // becomes observable. + Volatile.Write(ref _classInitializeResult, result); return result; } } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs b/src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs index 8be73b0e13..bcfce57967 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs @@ -66,6 +66,11 @@ public override string ToString() /// Properties. /// private readonly Dictionary _properties; +#if NET9_0_OR_GREATER + private readonly Lock _propertiesLock = new(); +#else + private readonly object _propertiesLock = new(); +#endif private readonly IMessageLogger? _messageLogger; private readonly TestRunCancellationToken? _testRunCancellationToken; @@ -295,6 +300,14 @@ public void AddProperty(string propertyName, string propertyValue) /// , which are preserved). /// Used to flow properties set during AssemblyInitialize / ClassInitialize /// into subsequent contexts. + /// + /// Merge precedence: keys in WIN over keys already + /// present in this context's bag. This is intentional — lifecycle snapshots typically + /// flow on top of the seeded source-level parameters (e.g. TestRunParameters from + /// .runsettings), so a user's explicit assignment in AssemblyInitialize / + /// ClassInitialize overrides any same-named runsettings value for the rest of + /// the lifecycle (class init, tests, class cleanup, assembly cleanup). + /// /// /// The properties to merge in. May be . internal void MergeProperties(IReadOnlyDictionary? propertiesToMerge) @@ -304,15 +317,22 @@ internal void MergeProperties(IReadOnlyDictionary? propertiesTo return; } - foreach (KeyValuePair kvp in propertiesToMerge) + // Take the same internal lock as CaptureLifecycleProperties so a snapshot capture + // cannot race with a merge on the same context (which would otherwise corrupt the + // Dictionary iterator or cause a missed write). Writes via the public Properties + // indexer still bypass this lock - see the remarks on CaptureLifecycleProperties. + lock (_propertiesLock) { - // Never overwrite the per-context labels. - if (kvp.Key == FullyQualifiedTestClassNameLabel || kvp.Key == TestNameLabel) + foreach (KeyValuePair kvp in propertiesToMerge) { - continue; - } + // Never overwrite the per-context labels. + if (kvp.Key == FullyQualifiedTestClassNameLabel || kvp.Key == TestNameLabel) + { + continue; + } - _properties[kvp.Key] = kvp.Value; + _properties[kvp.Key] = kvp.Value; + } } } @@ -328,19 +348,34 @@ internal void MergeProperties(IReadOnlyDictionary? propertiesTo /// shared across every context the snapshot is later merged into. Mutations of those /// reference-type instances are visible everywhere. /// + /// + /// Enumeration is performed under a private synchronization lock so that snapshot + /// capture is safe against concurrent calls to this method or + /// on the same context. Note: writes made via the public indexer + /// do NOT take this lock, so a lifecycle method that spawns a background thread which + /// keeps mutating past method return can still race with the + /// capture - that is treated as user error and is consistent with the pre-existing + /// thread-affinity expectation of AssemblyInitialize / ClassInitialize. + /// /// /// A read-only snapshot of the current properties. internal IReadOnlyDictionary CaptureLifecycleProperties() { - var snapshot = new Dictionary(_properties.Count); - foreach (KeyValuePair kvp in _properties) + Dictionary snapshot; + lock (_propertiesLock) { - if (kvp.Key == FullyQualifiedTestClassNameLabel || kvp.Key == TestNameLabel) +#pragma warning disable IDE0028 // Collection initialization can be simplified - capacity hint is intentional. + snapshot = new Dictionary(_properties.Count); +#pragma warning restore IDE0028 + foreach (KeyValuePair kvp in _properties) { - continue; - } + if (kvp.Key == FullyQualifiedTestClassNameLabel || kvp.Key == TestNameLabel) + { + continue; + } - snapshot[kvp.Key] = kvp.Value; + snapshot[kvp.Key] = kvp.Value; + } } return new ReadOnlyDictionary(snapshot); diff --git a/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs b/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs index fc4c9c4040..780499b99d 100644 --- a/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs +++ b/test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs @@ -3,6 +3,7 @@ using Microsoft.Testing.Platform.Acceptance.IntegrationTests; using Microsoft.Testing.Platform.Acceptance.IntegrationTests.Helpers; +using Microsoft.Testing.Platform.Helpers; namespace MSTest.Acceptance.IntegrationTests; @@ -201,3 +202,165 @@ public void DataRowSeesFlowedProperties(int rowNumber) public TestContext TestContext { get; set; } = null!; } + +/// +/// Acceptance test for the ClassCleanupManager.ForceCleanup fallback path that runs +/// when execution stops early (e.g. via --maximum-failed-tests). It validates that +/// ClassCleanup and AssemblyCleanup invoked via that fallback still observe +/// the lifecycle property snapshots captured during AssemblyInitialize and +/// ClassInitialize. +/// +[TestClass] +public sealed class TestContextPropertyFlowForceCleanupTests : AcceptanceTestBase +{ + private const string MarkerDirectoryEnvVar = "FORCECLEANUP_MARKER_DIR"; + private const string ClassCleanupMarkerFileName = "class-cleanup.marker"; + private const string AssemblyCleanupMarkerFileName = "assembly-cleanup.marker"; + + [TestMethod] + [DynamicData(nameof(TargetFrameworks.AllForDynamicData), typeof(TargetFrameworks))] + public async Task ForceCleanupSeesAssemblyAndClassInitProperties(string tfm) + { + var testHost = TestHost.LocateFrom(AssetFixture.ProjectPath, TestAssetFixture.ProjectName, tfm); + + // Cleanup methods cannot signal success via Console.WriteLine because MSTest routes + // Console.Out through a per-TestContext capture during cleanup (and the captured + // output is discarded on the ForceCleanup fallback path). Use a marker directory + // on disk instead: the cleanup methods only create the marker files when their + // property-flow assertions pass. + string markerDirectory = Path.Combine(Path.GetTempPath(), "mstest-force-cleanup-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(markerDirectory); + try + { + TestHostResult testHostResult = await testHost.ExecuteAsync( + "--maximum-failed-tests 1", + environmentVariables: new() { [MarkerDirectoryEnvVar] = markerDirectory }, + cancellationToken: TestContext.CancellationToken); + + testHostResult.AssertExitCodeIs(ExitCode.TestExecutionStoppedForMaxFailedTests); + // ClassCleanup and AssemblyCleanup must see the AssemblyInit / ClassInit snapshots + // regardless of whether they are invoked via the normal end-of-class / end-of-assembly + // path or via the ForceCleanup fallback (which path runs depends on the race between + // the failing test and the graceful-stop request triggered by --maximum-failed-tests). + // The marker files are created only when the assertions inside the cleanup methods pass. + Assert.IsTrue( + File.Exists(Path.Combine(markerDirectory, ClassCleanupMarkerFileName)), + $"ClassCleanup marker file not found. StandardOutput:\n{testHostResult.StandardOutput}"); + Assert.IsTrue( + File.Exists(Path.Combine(markerDirectory, AssemblyCleanupMarkerFileName)), + $"AssemblyCleanup marker file not found. StandardOutput:\n{testHostResult.StandardOutput}"); + } + finally + { + try + { + Directory.Delete(markerDirectory, recursive: true); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // Best-effort cleanup of the temporary marker directory. + } + } + } + + public sealed class TestAssetFixture() : TestAssetFixtureBase() + { + public const string ProjectName = "TestContextPropertyFlowForceCleanup"; + + public string ProjectPath => GetAssetPath(ProjectName); + + public override (string ID, string Name, string Code) GetAssetsToGenerate() => (ProjectName, ProjectName, + SourceCode + .PatchTargetFrameworks(TargetFrameworks.All) + .PatchCodeWithReplace("$MSTestVersion$", MSTestVersion)); + + private const string SourceCode = """ +#file TestContextPropertyFlowForceCleanup.csproj + + + + Exe + true + $TargetFrameworks$ + preview + enable + true + + + + + + + + +#file UnitTest1.cs +using System; +using System.IO; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +[TestClass] +public sealed class ForceCleanupFlowTests +{ + public TestContext TestContext { get; set; } = null!; + + [AssemblyInitialize] + public static void AssemblyInit(TestContext context) + => context.Properties["AssemblyInitKey"] = "AssemblyInitValue"; + + [ClassInitialize] + public static void ClassInit(TestContext context) + => context.Properties["ClassInitKey"] = "ClassInitValue"; + + // Failing test triggers --maximum-failed-tests=1 graceful stop, leaving the + // remaining tests un-run; the normal end-of-class / end-of-assembly cleanup is + // skipped, so cleanup must come through the ForceCleanup fallback path. + [TestMethod] + public void TestA_Fails() => Assert.Fail("intentional fail to trigger graceful stop"); + + [TestMethod] + public void TestB_Passes() { } + + [TestMethod] + public void TestC_Passes() { } + + [ClassCleanup] + public static void ClassCleanup(TestContext context) + { + Assert.AreEqual("AssemblyInitValue", context.Properties["AssemblyInitKey"]); + Assert.AreEqual("ClassInitValue", context.Properties["ClassInitKey"]); + WriteMarker("class-cleanup.marker"); + } + + [AssemblyCleanup] + public static void AssemblyCleanup(TestContext context) + { + // AssemblyCleanup must see AssemblyInit-set values. ClassInit-set values are + // class-scoped and must NOT flow to AssemblyCleanup even via the fallback path. + Assert.AreEqual("AssemblyInitValue", context.Properties["AssemblyInitKey"]); + Assert.IsFalse( + context.Properties.ContainsKey("ClassInitKey"), + "Properties set by ClassInitialize must not flow to AssemblyCleanup, even via ForceCleanup."); + WriteMarker("assembly-cleanup.marker"); + } + + // Cleanup methods cannot reliably signal success via Console.WriteLine because MSTest routes + // Console.Out into the per-TestContext output buffer during cleanup (and that buffer is + // discarded on the ForceCleanup fallback path). Write to a marker file under the directory + // supplied by the test harness so the parent test can observe the cleanup ran with + // satisfied property-flow assertions. + private static void WriteMarker(string fileName) + { + string? markerDirectory = Environment.GetEnvironmentVariable("FORCECLEANUP_MARKER_DIR"); + if (string.IsNullOrEmpty(markerDirectory)) + { + return; + } + + File.WriteAllText(Path.Combine(markerDirectory, fileName), "ok"); + } +} +"""; + } + + public TestContext TestContext { get; set; } = null!; +} diff --git a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.cs b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.cs index 8c54c1a131..641197958c 100644 --- a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.cs +++ b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.cs @@ -441,6 +441,26 @@ public void MergePropertiesShouldOverwriteExistingKeys() _testContextImplementation.Properties["Key"].Should().Be("Overwritten"); } + public void MergePropertiesShouldOverrideSeededSourceLevelParameters() + { + // Seeded source-level parameters (the bag the runner forwards from runsettings + // TestRunParameters) sit in _properties at construction time; lifecycle snapshots + // from AssemblyInitialize / ClassInitialize MUST override them on key collision so + // a user's explicit assignment wins for the rest of the lifecycle. + var seeded = new Dictionary + { + ["RunSettingsKey"] = "FromRunSettings", + }; + _testContextImplementation = new TestContextImplementation(_testMethod.Object, null, seeded, null, null); + + _testContextImplementation.MergeProperties(new Dictionary + { + ["RunSettingsKey"] = "FromAssemblyInit", + }); + + _testContextImplementation.Properties["RunSettingsKey"].Should().Be("FromAssemblyInit"); + } + public void MergePropertiesShouldIgnoreNull() { _testContextImplementation = CreateTestContextImplementation(); @@ -517,6 +537,25 @@ public void CaptureLifecyclePropertiesShouldAliasReferenceTypeValues() ((List)snapshot["RefKey"]!).Should().BeEquivalentTo(new[] { 1, 2 }); } + public void CaptureLifecyclePropertiesAndMergePropertiesShouldNotLockOnExposedPropertyBag() + { + _testContextImplementation = CreateTestContextImplementation(); + + lock (_testContextImplementation.Properties) + { + Task.WhenAll( + Task.Run(() => _ = _testContextImplementation.CaptureLifecycleProperties()), + Task.Run(() => _testContextImplementation.MergeProperties(new Dictionary + { + ["Key"] = "Value", + }))) + .Wait(TimeSpan.FromSeconds(10)) + .Should().BeTrue(); + } + + _testContextImplementation.Properties["Key"].Should().Be("Value"); + } + public void ConstructorShouldNotThrowWhenSeededPropertiesAlreadyContainFullyQualifiedTestClassName() { _testMethod.Setup(tm => tm.FullClassName).Returns("A.C.M");