[thomhurst]

Summary

• For SharedType.PerTestSession data sources, the constructor factory runs exactly once via Lazy<T> (ExecutionAndPublication mode). Whichever test's build context wins the ConcurrentDictionary race triggers the factory — so the constructor output only lands in that one test's _buildTimeOutput, making test assertions order-dependent.
• On macOS CI (parallel test building), Test_SharedDataSource_SecondTest could win the race, leaving Test_SharedDataSource_FirstTest with no constructor output → CI failure.
• Fix: after capturing buildContext.GetCapturedOutput() in TestBuilder, also write it to TestSessionContext.Current.OutputWriter so shared data source output is always accessible at the session level regardless of build ordering.
• Update TestBuildContextOutputCaptureTests.Test_SharedDataSource_FirstTest to check TestSessionContext.Current.GetStandardOutput() instead of TestContext.Current.GetStandardOutput().

Test plan

• TestBuildContextOutputCaptureTests passes on all frameworks locally
• CI passes on macOS (was previously flaky due to build-order race)

[claude]

Code Review

Good diagnosis and targeted fix for a real CI flakiness issue on macOS. The root cause analysis is accurate: for SharedType.PerTestSession sources, Lazy<T> (ExecutionAndPublication) runs the factory exactly once, so the captured build-time output only ends up in whichever test's buildContext won the parallel-build race. Routing to TestSessionContext is the right architectural choice here.

Strengths

• Minimal and well-targeted (18 changed lines, 2 files)
• Null-safe TestSessionContext.Current? prevents NPE if no session context exists
• Existing behaviour is preserved — the winning test's batch still gets output via SetBuildTimeOutput
• Excellent inline comments explaining the why (the Lazy race, ExecutionAndPublication, etc.)
• Test updated to match the new (correct) semantics

Concerns

1. Session routing applies to ALL data sources, not just PerTestSession

The new code runs unconditionally inside the if (\!string.IsNullOrEmpty(capturedOutput) || ...) block, so it fires for every data source that emits build-time output — PerClass, PerAssembly, unshared, etc. Only PerTestSession sources actually have the race condition described in the PR, because they use a single Lazy instance shared across all tests.

For non-session scoped sources, the winning batch already propagates output to its tests via SetBuildTimeOutput (they're in the same batch); routing to session is noise. A user checking TestSessionContext.Current.GetStandardOutput() will start seeing constructor output from logically-scoped data sources mixed in with session-level output. Consider guarding this on scope, or at minimum documenting the behaviour change:

That said, if the intent is "session output is a superset of all build-time output", this is fine — just make it explicit.

2. Output duplication for the "winning" test

When Test_SharedDataSource_FirstTest happens to win the build race, its output now appears in both TestContext.Current.GetStandardOutput() (via SetBuildTimeOutput) AND TestSessionContext.Current.GetStandardOutput(). The PR comment acknowledges you can no longer rely on TestContext.Current for this, but the reverse is also true: a test reading both contexts would see the output twice. Worth documenting or at least noting in the test class summary.

3. Test_SharedDataSource_SecondTest has no assertion on the fix

SecondTest intentionally skips output assertions and just verifies data.Value. Now that session routing guarantees the output is always at TestSessionContext, it would be more valuable to also assert from SecondTest that the session output contains the constructor string. This would give a regression test proving that both tests (regardless of which won the race) can access the shared output — which is the whole point of the fix.

Summary

The fix is correct and solves the reported flakiness. The concerns above are mostly about scope precision and test coverage rather than correctness. Concern #3 (adding an assertion to SecondTest) is the most actionable — it would give the fix a proper regression test. Concerns #1 and #2 are worth a conscious decision but not blockers.

[claude]

Code Review

Good diagnosis and targeted fix for a real CI flakiness issue on macOS. The root cause analysis is accurate: for SharedType.PerTestSession sources, Lazy<T> (ExecutionAndPublication) runs the factory exactly once, so the captured build-time output only ends up in whichever test's buildContext won the parallel-build race. Routing to TestSessionContext is the right architectural choice here.

Strengths

• Minimal and well-targeted (18 changed lines, 2 files)
• Null-safe TestSessionContext.Current?. prevents NPE if no session context exists
• Existing behaviour is preserved — the winning test batch still gets output via SetBuildTimeOutput
• Excellent inline comments explaining the why (the Lazy race, ExecutionAndPublication, etc.)
• Test updated to match the new correct semantics

Concerns

1. Session routing applies to ALL data sources, not just PerTestSession

The new code runs unconditionally inside the if (!string.IsNullOrEmpty(capturedOutput) || ...) block, so it fires for every data source that emits build-time output — PerClass, PerAssembly, unshared, etc. Only PerTestSession sources actually have the race condition described in the PR, because they use a single Lazy instance shared across all tests.

For non-session-scoped sources, the winning batch already propagates output to its tests via SetBuildTimeOutput (they are in the same batch), so routing to session is noise. A user checking TestSessionContext.Current.GetStandardOutput() will start seeing constructor output from logically-scoped data sources mixed in with session-level output. Consider guarding this on scope, or at minimum documenting the behaviour change.

2. Output duplication for the "winning" test

When Test_SharedDataSource_FirstTest happens to win the build race, its output now appears in both TestContext.Current.GetStandardOutput() (via SetBuildTimeOutput) AND TestSessionContext.Current.GetStandardOutput(). The PR correctly documents you can no longer rely on TestContext.Current for this, but a test reading both contexts would see the output twice. Worth noting in the test class summary.

3. Test_SharedDataSource_SecondTest has no assertion on the fix

SecondTest intentionally skips output assertions and just verifies data.Value. Now that session routing guarantees the output is always at TestSessionContext, it would be more valuable to also assert from SecondTest that the session output contains the constructor string. This gives a proper regression test proving that both tests, regardless of which won the build race, can access the shared output — which is the whole point of the fix.

Summary

The fix is correct and addresses the reported macOS CI flakiness. Concerns #1 and #2 are worth a conscious design decision but not blockers. Concern #3 (adding an assertion to SecondTest) is the most actionable improvement — it would give the fix a regression test that validates both sides of the race condition are covered.

[claude]

Follow-up Review

Checked this against the previous two review comments (both submitted simultaneously on the first commit).

Previous concerns — status

Concern #1 — Unconditional session routing for all data source types
Resolved by documentation. The comment now explicitly states "This intentionally covers all sharing types (None, PerClass, PerAssembly, etc.), making TestSessionContext a superset of every data source's build-time output." This makes the design decision visible and deliberate rather than accidental. Acceptable resolution.

Concern #2 — Output duplication for the "winning" test
Resolved by documentation. The added note — "the winning test batch also receives the output via SetBuildTimeOutput below, so its output appears in both TestContext and TestSessionContext" — makes the double-write explicit to future maintainers. Acceptable.

Concern #3 — SecondTest had no regression assertion
Fully addressed. SecondTest now asserts sessionOutput.Contains("SharedDataSource constructor:"), which is the key regression test validating that both sides of the build-order race are covered — regardless of which test's Lazy<T> factory wins.

New observation

The second commit expanded the TestBuilder.cs comment from 4 lines to 9 lines. The expanded comment is genuinely useful and adds important context, but one phrasing is slightly imprecise:

"making TestSessionContext a superset of every data source's build-time output"

TestSessionContext output is more accurately a union/accumulation (every data source that emits output writes into it) rather than a true set-superset relationship. Minor nit — not a blocker.

Summary

All three previous concerns have been addressed in the second commit — two by clear documentation, one by adding a proper regression assertion to SecondTest. The fix is correct, the scope is minimal, and the test coverage now validates both sides of the race condition. Ready to merge.

[claude]

Follow-up Review (3rd commit: 2e8a8aa2a1a7)

Reviewing against the previous three comments and the new changes in the third commit.

Previous concerns — all resolved

All three concerns from prior reviews have been addressed:

• Concern #1 (unconditional session routing): Documented as intentional in the expanded comment.
• Concern #2 (output duplication for the winning test): Documented explicitly in the comment.
• Concern #3 (SecondTest had no regression assertion): Fully fixed — SecondTest now asserts sessionOutput.Contains("SharedDataSource constructor:").

---

Third commit: guard _3882 assembly/session hooks against unrelated subprocess runs

Root cause diagnosis is accurate. Assembly and session hooks fire per subprocess, not per test target. When concurrent engine subprocesses all load the same assembly, each one runs the hooks — causing simultaneous writes to the same temp file and IOException.

Strengths:

• The guard pattern (context.TestClasses.Any(c => c.ClassType == typeof(...))) is the right mechanism: it checks whether the specific class participated in this subprocess's run before writing the marker file.
• Consistent: the guard is applied symmetrically to both [Before] and [After] hooks across all four affected classes.
• Removing the try-catch is safe here. The original try-catch re-threw the exception anyway (it only added a Console.WriteLine before the throw), so the simplified code is functionally equivalent in terms of exception propagation. The guard now makes the exceptional path (concurrent write collision) unreachable, so the logging no longer adds value.

Minor observations (not blockers):

1. Guard duplication: The context.TestClasses.Any(c => c.ClassType == typeof(X)) pattern appears 8 times across 2 files. For test project code this is fine, but if this pattern recurs, a small helper like IsClassInContext(context, typeof(X)) or an extension method would reduce noise. Not worth changing now.

2. [Before(TestSession)] guard timing: TestSessionContext.TestClasses must be fully populated before the Before hook runs for this guard to be reliable. That should be the case in TUnit (the session context is built before hooks execute), but worth noting as a subtle assumption.

3. Removed console logging on After-hook failure: The old code logged [AfterAssembly] Failed to write marker file: {ex.Message} before re-throwing. The new code skips the write entirely for non-participating subprocesses, so there's nothing to log. For the participating subprocess, failures will now surface as raw exceptions without the context message. Acceptable tradeoff for the simplification.

---

Summary

All prior concerns addressed. The third commit correctly fixes a secondary race condition: concurrent subprocess marker-file collisions in the _3882 test classes. The approach (guard on class membership) is sound, the simplification of try-catch is safe, and the change is scoped appropriately. Ready to merge.