fix: assign TestDetails before TestContext is published to ClassHookContext.Tests

[thomhurst]

Fixes #6180

Problem

ContextProvider.CreateTestContext called classContext.AddTest(testContext) while TestContext.TestDetails was still null! — it was only assigned afterwards by the caller. AfterEvery(Class) hooks iterating ClassHookContext.Tests could therefore observe a partially-built context. For dynamic tests (AddDynamicTest) registered at runtime into a ClassHookContext shared by Type, this raced with sibling test construction → flaky NRE on test.Metadata.TestDetails.TestName.

Fix

Every call site already constructed the full TestDetails before calling CreateTestContext, so this just plumbs it through as a parameter and assigns it before AddTest publishes the context. The partially-initialized published state is removed entirely (rather than masked by filtering Tests). Shared ContextProvider/builder path → covers both source-gen and reflection modes.

• IContextProvider.CreateTestContext / ContextProvider.CreateTestContext: new TestDetails testDetails parameter, assigned before classContext.AddTest(...)
• Six engine call sites (TestBuilder ×2, TestBuilderPipeline ×4): pass testDetails, post-hoc assignment removed
• PublicAPI snapshots updated (exactly the new parameter, all 4 TFMs)
• Regression test ContextProviderTests pins the invariant: a context visible via ClassHookContext.Tests always has TestDetails set

Verification

• TUnit.UnitTests (net10.0): 219/219 passed, incl. new regression test
• TUnit.PublicAPI Core snapshots: 4/4 passed after accepting the diff
• TUnit.TestProject --treenode-filter "/*/*DynamicTests/*/*": 29/29 passed in source-gen and --reflection mode
• TUnit.TestProject --treenode-filter "/*/*/AfterEveryClassTests/*": passed

Note: the original race is timing-dependent and not deterministically reproducible; the regression test pins the publication invariant that closes it.

[claude]

Code Review

This is a clean and correct fix for the race condition described in #6180. The root-cause diagnosis is accurate and the approach chosen is the right one.

What the fix gets right

Atomic publication pattern: Moving assignment inside (before ) is the correct architectural choice. The alternative — filtering to hide partially-built contexts — would mask the symptom while leaving the broken invariant in place, and would be harder to reason about across the codebase. Making the context fully-formed before it becomes observable eliminates the entire class of partial-state bugs.

Call-site cleanup is consistent: All six call sites in TestBuilder and TestBuilderPipeline have the post-hoc context.Metadata.TestDetails = testDetails lines removed. The PR diff looks complete — no leftover assignments.

Test pins the invariant, not the timing: The regression test in ContextProviderTests.cs correctly validates the structural guarantee (a context visible in ClassHookContext.Tests always has TestDetails set) rather than trying to reproduce the non-deterministic race. That's the right approach.

Dual-mode coverage: Since ContextProvider lives in TUnit.Core and is shared, the fix applies to both source-gen and reflection paths without needing separate changes to the generated code paths.

---

One observation worth flagging

This is a breaking change to a public interface. IContextProvider.CreateTestContext is part of the public API surface (as confirmed by the four PublicAPI snapshot updates). Any external code implementing IContextProvider will fail to compile after this change. The PR description is thorough in other respects but doesn't call this out explicitly. If there are known external consumers of this interface, a changelog entry or migration note would be appropriate.

The API snapshots are correctly updated for all four TFMs (net8.0, net9.0, net10.0, net4.7), so the project's own API-gate tests will pass — this is just about downstream consumers.

---

Minor: parameter placement follows convention

Inserting testDetails before cancellationToken is correct — CancellationToken should always be last by .NET convention. Good.

---

Verdict

Approve. The fix eliminates the race by construction rather than masking it, the regression test is well-scoped, and the change is consistent across all call sites. The only thing worth a follow-up note (not a blocker) is documenting the breaking API change for downstream implementors of IContextProvider.

[claude]

Code Review

This is a clean and correct fix for the race condition described in #6180. The root-cause diagnosis is accurate and the approach chosen is the right one.

What the fix gets right

Atomic publication pattern: Moving TestDetails assignment inside CreateTestContext (before AddTest) is the correct architectural choice. The alternative — filtering Tests to hide partially-built contexts — would mask the symptom while leaving the broken invariant in place, and would be harder to reason about across the codebase. Making the context fully-formed before it becomes observable eliminates the entire class of partial-state bugs.

Call-site cleanup is consistent: All six call sites in TestBuilder and TestBuilderPipeline have the post-hoc context.Metadata.TestDetails = testDetails lines removed. The PR diff looks complete — no leftover assignments.

Test pins the invariant, not the timing: The regression test in ContextProviderTests.cs correctly validates the structural guarantee (a context visible in ClassHookContext.Tests always has TestDetails set) rather than trying to reproduce the non-deterministic race. That is the right approach.

Dual-mode coverage: Since ContextProvider lives in TUnit.Core and is shared, the fix applies to both source-gen and reflection paths without needing separate changes to the generated code paths.

---

One observation worth flagging

This is a breaking change to a public interface. IContextProvider.CreateTestContext is part of the public API surface (as confirmed by the four PublicAPI snapshot updates). Any external code implementing IContextProvider will fail to compile after this change. The PR description is thorough in other respects but does not call this out explicitly. If there are known external consumers of this interface, a changelog entry or migration note would be appropriate.

The API snapshots are correctly updated for all four TFMs (net8.0, net9.0, net10.0, net4.7), so the project own API-gate tests will pass — this is just about downstream consumers.

---

Minor: parameter placement follows convention

Inserting testDetails before cancellationToken is correct — CancellationToken should always be last by .NET convention. Good.

---

Verdict

Approve. The fix eliminates the race by construction rather than masking it, the regression test is well-scoped, and the change is consistent across all call sites. The only thing worth a follow-up (not a blocker) is documenting the breaking API change for downstream implementors of IContextProvider.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
BestPractice	1 medium

View in Codacy

🟢 Metrics 5 complexity

Metric	Results
Complexity	5

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Follow-up Review (updated commits ce74860 → 70c23d4)

Picking up from my previous review — two commits landed after it:

4f69cd46 — refactor: drop unused testName parameter

This is a good cleanup. The testName parameter in the original CreateTestContext signature was vestigial once testDetails.TestName was being passed into TestContext from inside the method. Removing the explicit parameter closes the potential for a caller accidentally passing a name that differed from testDetails.TestName, which would have been a subtle bug. The change is mechanically applied at all six call sites and in the test stub in SessionActivityLifecycleTests. All four TFM PublicAPI snapshots are updated correctly.

70c23d4f — chore: remove accidentally committed diff.txt

No code impact; clean housekeeping.

---

Previous observations resolved

My earlier note about the breaking public API change stands as written — the signature change is intentional and the snapshots are correctly updated. No new concerns arise from the follow-up refactor.

---

Final verdict

The PR is in good shape. The fix is correct, the follow-up refactor makes the API cleaner and removes a latent inconsistency risk, and the regression test pins the invariant. Approve.

[claude]

Code Review

This is a correct and well-executed fix. The existing Claude reviews have already covered the main points (atomic publication invariant, complete call-site cleanup, test pinning the structural guarantee rather than the non-deterministic race, dual-mode coverage). This review focuses on what hasn't yet been called out.

---

Residual testName parameter in TestContext constructor

The TestContext constructor still accepts string testName as its first parameter, but the constructor body does not store it anywhere — it is immediately discarded. Since this PR moved the only meaningful use of that value to testDetails.TestName inside ContextProvider.CreateTestContext, the constructor parameter is now dead weight.

// TestContext constructor accepts testName but never assigns it
public TestContext(string testName, IServiceProvider serviceProvider, ...)
{
    _testBuilderContext = testBuilderContext;
    CancellationToken = cancellationToken;
    // testName is not stored
    ...
}

The one call to that constructor now passes testDetails.TestName just to satisfy the parameter. This suggests two cleaner options:

1. Accept TestDetails directly in the constructor and drop the testName param entirely
2. Drop testName from the constructor signature and derive it from TestDetails at the point where DisplayName is needed

Option 1 would be the stronger fix — it closes the last path by which a TestContext can be constructed without a TestDetails (the TestNodeLocationTests local helper bypasses ContextProvider and must still post-hoc assign context.TestDetails = ...). This would make the invariant compiler-enforced rather than convention-enforced.

---

TestDetails { get; set; } = null! is still mutable after publication

The property is internal TestDetails TestDetails { get; set; } = null!. The PR correctly ensures it is set before AddTest, but set remains accessible to all internal callers. The TestNodeLocationTests helper demonstrates this — it bypasses ContextProvider and assigns the property post-construction.

This isn't a regression (it existed before), but the PR's stated goal was to make partial-state unobservable by construction. Changing the setter to init (or making the constructor accept TestDetails) would make that guarantee enforced by the type system rather than by code review convention. Worth a follow-up issue.

---

IContextProvider / ContextProvider visibility change — good

The public → internal change is correct and desirable. These types had no reason to be public surface; making them internal closes off a footgun where external code could supply a custom IContextProvider that violated the publication invariant. The PublicAPI snapshots correctly reflect the removal across all four TFMs.

---

Test completeness

The regression test in ContextProviderTests.cs correctly covers the single-threaded invariant. The concurrent case is not covered, which the PR description acknowledges is by design since it's non-deterministic — this is the right call.

One small note: the test calls GetOrCreateClassContext after CreateTestContext to fetch the class context. Capturing it before calling CreateTestContext and then asserting the returned context appears in that pre-fetched context's Tests would make the "published before return" ordering more explicit. Not a correctness issue.

---

Verdict

Approve. The fix correctly closes the race by ensuring TestDetails is assigned before AddTest publishes the context. The items above (testName dead parameter, mutable setter) are pre-existing issues that this PR has made more visible but did not introduce — worth a follow-up rather than blocking this merge.