Follow-up to #8386: address review comments on TestContext.Properties flow

[Evangelink]

Follow-up to #8386 (which merged moments before the review feedback could be applied).

What this PR does

Source changes

1. Safe publication of lifecycle snapshots (addresses one of @copilot-pull-request-reviewer's blocking concerns) — TestAssemblyInfo.PostAssemblyInitProperties and TestClassInfo.PostClassInitProperties now use Volatile.Read / Volatile.Write so consumers on the cached-result fast paths (which intentionally bypass the per-class / per-assembly semaphores) safely observe the snapshot published by the thread that ran the init method.
2. Snapshot-enumeration safety (addresses the other @copilot-pull-request-reviewer concern) — TestContextImplementation.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 and are out of scope, which is consistent with the long-standing thread-affinity expectation of AssemblyInitialize / ClassInitialize.
3. Merge precedence documented (addresses one of @Evangelink's moderate findings in the expert review) — TestContextImplementation.MergeProperties XML doc now spells out that lifecycle snapshots WIN over keys already present in the bag (e.g. runsettings TestRunParameters) on collision.

Tests

1. MergePropertiesShouldOverrideSeededSourceLevelParameters — unit test asserting that MergeProperties overwrites a key seeded at construction time (the runsettings precedence case).
2. TestContextPropertyFlowForceCleanupTests acceptance suite (addresses another of @Evangelink's findings: no test exercised ClassCleanupManager.ForceCleanup) — triggers ForceCleanup via --maximum-failed-tests=1, asserts that AssemblyInit / ClassInit properties flow into the ClassCleanup and AssemblyCleanup contexts invoked through the fallback path, and re-confirms that ClassInit-set properties never leak into AssemblyCleanup (mirroring the normal-path scoping rule).

Notes on the 17 [TestMethod] comments from @copilot-pull-request-reviewer

Those are false positives — MSTestAdapter.PlatformServices.UnitTests uses the internal TestContainer base from TestFramework.ForTestingMSTest. Any public parameterless method is treated as a test method, no [TestMethod] attribute is required (the original 800+ tests in those files follow the same pattern). No change needed.

Verification

• dotnet build of MSTestAdapter.PlatformServices.UnitTests (net9.0): 0 warnings, 0 errors.
• MSTestAdapter.PlatformServices.UnitTests (net9.0): 805 / 805 pass (was 804 — added the new MergePropertiesShouldOverrideSeededSourceLevelParameters).
• dotnet build of MSTest.Acceptance.IntegrationTests: 0 warnings, 0 errors.

Refs #8386. Fixes the open review threads on the same PR.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Evangelink]

Review Summary — PR #8396

Dimension	Result
1. Algorithmic Correctness	✅
2. Threading & Concurrency	❌ See inline
3. Error Handling	✅
4. Public API Surface	✅ (all changes are internal)
5. No init on New Public API	✅ N/A
6. Localization	✅ N/A (no user-facing strings)
7. Performance	✅
8. Cross-TFM Correctness	✅ Volatile is correct on all TFMs; field keyword requires LangVersion>preview which is set
9. IPC Contract	✅ N/A
10. Security	✅ N/A
11. Test Coverage	✅ New unit test + acceptance test added
12. Test Quality	✅ TestFramework.ForTestingMSTest auto-discovers parameterless public methods; missing [TestMethod] is correct
13. Documentation / Comments	✅ Excellent XML docs — one inconsistency flagged in inline comment
14. Naming Conventions	✅
15. StyleCop / EditorConfig	✅
16. Backward Compatibility	✅ All internal
17. Scope Discipline	✅ Tight follow-up scope
18. Code Simplicity	✅
19. Resource / Disposal	✅ N/A
20. Async Correctness	✅ N/A
21. Build / Packaging	✅ N/A

One blocking issue

MergeProperties is not under the same lock as CaptureLifecycleProperties, yet the doc comment on CaptureLifecycleProperties explicitly claims safety against concurrent MergeProperties calls (line 339). This is either a documentation error or a missing lock. Since MergeProperties is internal and framework-controlled, adding lock (_properties) { ... } around its loop body is the correct fix and directly matches the documented contract.

Everything else looks good

• Volatile.Read / Volatile.Write on the property-backing fields are the correct primitives for the publish/subscribe pattern used here (one writer completes init under a semaphore, many readers later observe the result on the fast path). The acquire/release semantics are sufficient.
• CaptureLifecycleProperties locking on _properties is safe because Dictionary is a valid lock target as long as all code that iterates or structurally modifies it takes the same lock.
• The field keyword (C# 13 semi-auto property) compiles because the project sets <LangVersion>preview</LangVersion>.
• The acceptance test using --maximum-failed-tests 1 is a reasonable approach; the sentinel-string check via Assert.Contains on StandardOutput is consistent with the rest of the acceptance suite.
• The new unit test without [TestMethod] is intentional and correct for TestFramework.ForTestingMSTest (confirmed: "All public parameterless methods will be recognized as test methods").

Generated by Expert Code Review (on open) for issue #8396 · ● 5M

[Copilot]

Copilot's findings

• Files reviewed: 18/18 changed files
• Comments generated: 1