Flow TestContext.Properties through Assembly/Class lifecycle#8386
Conversation
Properties written to TestContext.Properties in [AssemblyInitialize] now flow to every [ClassInitialize], test method, [ClassCleanup] and [AssemblyCleanup]. Properties written in [ClassInitialize] flow to test methods and [ClassCleanup] of that class. Implementation: - TestContextImplementation: new internal CaptureLifecycleProperties() and MergeProperties(); defensive switch from Add to indexer assignment for the per-context label keys. - TestAssemblyInfo.PostAssemblyInitProperties: snapshot captured inside the existing _assemblyInfoExecuteSyncSemaphore after AssemblyInit body completes successfully. - TestClassInfo.PostClassInitProperties: snapshot captured inside the existing _testClassExecuteSyncSemaphore after ClassInit completes (includes base-chain class-init writes via InheritanceBehavior). - UnitTestRunner.RunSingleTestAsync: merges snapshots into class-init, test-execution, class-cleanup and assembly-cleanup contexts. The class-cleanup merge is gated on isLastTestInClass to avoid wasted copies on every test. - ClassCleanupManager.ForceCleanup: same merges on the fallback contexts. Per-context labels (FullyQualifiedTestClassName, TestName) are excluded from snapshots and preserved on merge so per-test identity stays intact. Snapshots are shallow (reference-type values are aliased across all flowed contexts) - documented in the new XML doc-comments. Class-init properties are intentionally NOT flowed to AssemblyCleanup because AssemblyCleanup is assembly-scoped and picking one class would be arbitrary. Tests: - 7 unit tests for MergeProperties/CaptureLifecycleProperties. - 4 unit tests for TestAssemblyInfo snapshot capture. - 4 unit tests for TestClassInfo snapshot capture (incl. base+derived chain). - New TestContextPropertyFlowTests acceptance suite covering AssemblyInit to tests, ClassInit to tests, override precedence, cross-class isolation, AssemblyCleanup excluding class-init props, no leakage between sibling tests, and [DataRow] shared bag. No public API changes. Fixes #5986 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enables TestContext.Properties values written during [AssemblyInitialize] and [ClassInitialize] to flow through subsequent MSTest lifecycle phases by snapshotting the property bag after init and merging those snapshots into later contexts.
Changes:
- Added internal snapshot/merge helpers to
TestContextImplementationand adjusted label seeding to be overwrite-safe. - Captured post-init property snapshots on
TestAssemblyInfo/TestClassInfoand merged them at key lifecycle points (class init, test execution, cleanups). - Added unit + acceptance coverage for merge/snapshot semantics and end-to-end lifecycle visibility.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs | Adds MergeProperties/CaptureLifecycleProperties and makes label seeding overwrite-safe. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs | Captures a post-assembly-init properties snapshot. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.cs | Captures a post-class-init properties snapshot. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs | Merges captured snapshots into class-init, test execution, and cleanup contexts. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/ClassCleanupManager.cs | Merges snapshots in ForceCleanup fallback cleanup contexts. |
| test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.cs | Adds unit tests for merge/snapshot behavior and label seeding change. |
| test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestAssemblyInfoTests.cs | Adds unit tests for post-assembly-init snapshot capture. |
| test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestClassInfoTests.cs | Adds unit tests for post-class-init snapshot capture (including base/derived chain). |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestContextPropertyFlowTests.cs | Adds acceptance suite to validate end-to-end lifecycle property flow across TFMs. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 18
| public void MergePropertiesShouldAddNewKeysIntoThePropertyBag() | ||
| { |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| public void MergePropertiesShouldOverwriteExistingKeys() | ||
| { |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| public void MergePropertiesShouldIgnoreNull() | ||
| { |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| public void MergePropertiesShouldNotOverwritePerContextLabels() | ||
| { |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| public void CaptureLifecyclePropertiesShouldReturnAllPropertiesExceptPerContextLabels() | ||
| { |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| _testAssemblyInfo.PostAssemblyInitProperties["AnotherKey"].Should().Be(42); | ||
| } | ||
|
|
||
| public async Task RunAssemblyInitializeShouldExcludePerContextLabelsFromPostAssemblyInitProperties() |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| _testAssemblyInfo.PostAssemblyInitProperties.Should().ContainKey("UserKey"); | ||
| } | ||
|
|
||
| public async Task RunAssemblyInitializeShouldLeavePostAssemblyInitPropertiesNullWhenAssemblyInitMethodIsNull() |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| _testAssemblyInfo.PostAssemblyInitProperties.Should().BeNull(); | ||
| } | ||
|
|
||
| public async Task RunAssemblyInitializeShouldLeavePostAssemblyInitPropertiesNullOnFailure() |
There was a problem hiding this comment.
False positive: MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest rather than vanilla MSTest. In that framework any public parameterless method is treated as a test method (see the 800+ existing tests in this file that also have no [TestMethod] attribute). No change needed.
Tracked via the follow-up PR #8396 alongside the other review items.
| var snapshot = new Dictionary<string, object?>(_properties.Count); | ||
| foreach (KeyValuePair<string, object?> kvp in _properties) | ||
| { | ||
| if (kvp.Key == FullyQualifiedTestClassNameLabel || kvp.Key == TestNameLabel) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| snapshot[kvp.Key] = kvp.Value; | ||
| } | ||
|
|
||
| return new ReadOnlyDictionary<string, object?>(snapshot); |
There was a problem hiding this comment.
Addressed in the follow-up PR #8396: CaptureLifecycleProperties now enumerates _properties under a lock so two snapshot calls cannot trip over each other. The doc-comment is explicit that writes via the public TestContext.Properties indexer bypass this lock — a lifecycle method that spawns a background thread which keeps mutating Properties past method return is treated as user error and out of scope, consistent with the long-standing thread-affinity expectation of AssemblyInitialize / ClassInitialize.
| // 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 = testContextImpl.CaptureLifecycleProperties(); |
There was a problem hiding this comment.
Addressed in the follow-up PR #8396: PostAssemblyInitProperties (and the matching PostClassInitProperties on TestClassInfo) now use Volatile.Read / Volatile.Write, replacing the temporary TODO left in the merged commit. The publishing thread does the Volatile.Write before the IsAssemblyInitializeExecuted flag flip; consumers Volatile-read the snapshot directly (the call site does not gate on the executed flag), so the snapshot field is the only thing that needs an acquire/release pair to be safely observed on the bypass-the-semaphore fast path.
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary
This PR correctly implements property-flow from AssemblyInitialize / ClassInitialize into downstream contexts (class init, test execution, class cleanup, assembly cleanup). The core design is sound, the acceptance test covers the critical scenarios (cross-class isolation, override precedence, assembly-cleanup scoping), and the unit tests are well-structured.
Findings
| Severity | Dimension | Finding |
|---|---|---|
| MODERATE | Threading & Concurrency | PostAssemblyInitProperties (and pre-existing ExecutionContext / AssemblyInitializationException) are published without a memory barrier on the fast path that skips the semaphore. Acknowledged via TODO; recommend tracking as a follow-up. |
| MODERATE | Test Completeness | ClassCleanupManager.ForceCleanup (triggered by --maximum-failed-tests) now merges lifecycle properties, but no test exercises this path to verify property visibility. |
| MODERATE | Algorithmic Correctness | MergeProperties uses overwrite semantics, so lifecycle properties silently win over sourceLevelParameters (runsettings) on key collision. This is likely the right priority order, but it should be called out in the doc/tests. |
Clean dimensions
Backward compatibility ✅ (all new surface is internal), no init accessors ✅, no PublicAPI.Unshipped.txt required ✅, cross-TFM compatibility ✅, CaptureLifecycleProperties correctly excludes per-context labels ✅, snapshot immutability (ReadOnlyDictionary wrapper) ✅, idempotency of MergeProperties ✅, assembly-cleanup correctly excluded from class-init snapshot ✅.
Generated by Expert Code Review (on open) for issue #8386 · ● 15M
|
Filed follow-up PR #8396 to address the post-merge review feedback. Quick map:
|
Summary
Flows
TestContext.Propertieswritten during[AssemblyInitialize]and[ClassInitialize]down through the rest of the test lifecycle. Fixes #5986.Today, every call to
PlatformServiceProvider.GetTestContextbuilds a brand-newTestContextImplementationwith its own private dictionary copied from the per-test seed. So acontext.Properties[X] = ...written insideAssemblyInitializeis stored in a context that's thrown away when the method returns - subsequent tests, class-init, and cleanups never see it. (And on tests #2+ the assembly-init context is never even passed in because of the cached-result short-circuit.) The MSTest docs say this scenario should work; this PR makes it work.Behavior after this change
[AssemblyInitialize]are visible in[ClassInitialize], the test class ctor,[TestInitialize], the test method,[TestCleanup],[ClassCleanup]and[AssemblyCleanup]of every test in the assembly.[ClassInitialize]are visible in tests,[TestInitialize],[TestCleanup]and[ClassCleanup]of that class. They override any conflicting assembly-init value within that class's scope.[AssemblyCleanup]deliberately does not see[ClassInitialize]properties (assembly-scoped, picking any one class would be arbitrary).Propertiesstill don't propagate to sibling tests (no behavioral regression).Design
After each init body completes successfully, capture a shallow snapshot of the live property bag onto the corresponding
TestAssemblyInfo/TestClassInfo, then merge those snapshots into all subsequent contexts:TestAssemblyInfo.PostAssemblyInitProperties_assemblyInfoExecuteSyncSemaphoreAssemblyInitializebody returnsTestClassInfo.PostClassInitProperties_testClassExecuteSyncSemaphoreRunClassInitializeAsyncreturns (includes base-chain writes)PostAssemblyInitPropertiesPostAssemblyInitProperties+PostClassInitProperties(merged in place beforeTestMethodRunner.ExecuteAsync)isLastTestInClass)PostAssemblyInitProperties+PostClassInitPropertiesPostAssemblyInitPropertiesClassCleanupManager.ForceCleanupfallback contextsSnapshots exclude the per-context labels (
FullyQualifiedTestClassName,TestName) andMergePropertiesrefuses to overwrite them, so per-test identity stays intact. Snapshots are shallow (reference-type values are aliased across all flowed contexts) - documented on the new XML doc-comments.Files
Source (5)
TestContextImplementation.cs- new internalCaptureLifecycleProperties()+MergeProperties(); defensive switch from_properties.Addto indexer assignment for the label keys so a seeded bag never throws.TestAssemblyInfo.cs- newPostAssemblyInitPropertiescapture point.TestClassInfo.cs- newPostClassInitPropertiescapture point.UnitTestRunner.cs- 4 merge sites + perf tweak (class-cleanup merge moved inside theisLastTestInClassguard).ClassCleanupManager.cs- merges in the fallback path.Tests (4)
TestContextImplementationTests- 7 new tests coveringMergeProperties(skip labels, null-tolerant, overwrite semantics),CaptureLifecycleProperties(snapshot independence, shallow/aliasing), and the defensive ctor change.TestAssemblyInfoTests- 4 new tests (capture on success, label exclusion, null when no init method, null on failure).TestClassInfoTests- 4 new tests (capture on success, null when no init method, null on failure, base+derived chain).TestContextPropertyFlowTestsacceptance suite (its own asset, runs on net462/net8.0/net10.0) covering: AssemblyInit→tests, ClassInit→tests, ClassInit override of AssemblyInit value, ClassCleanup observes both, AssemblyCleanup observes AssemblyInit only, cross-class isolation, no per-test leakage,[DataRow]shared bag.API surface
None. Public API unchanged; all new types/properties are internal.
Verification
build.cmd -pack -c Release-> 0 warnings, 0 errors.MSTestAdapter.PlatformServices.UnitTestspass on net9.0.TestContextTestsacceptance tests still pass.Fixes #5986