fix: resolve race condition in HookExecutionOrderTests

[thomhurst]

Summary

• GlobalHookExecutionOrderSetup.GlobalSetup has [BeforeEvery(Test)] which fires for every test in the assembly, not just HookExecutionOrderTests
• When tests run concurrently, another test's BeforeEvery invocation calls Clear() on the shared _executionOrder list between the Before hook and the test body, causing the assertion to see count=2 instead of 3
• Fix: filter the BeforeEvery hook to only act when the current test belongs to HookExecutionOrderTests

Test plan

• HookExecutionOrderTests.VerifyExecutionOrder passes in isolation (both source-gen and reflection modes)
• Full test suite passes with no flaky failures on this test

[claude]

Code Review

Fix is correct and the root cause diagnosis is accurate.

The hook firing for all tests in the assembly while clearing a shared static list is indeed the source of the race condition. The guard on properly contains the effect to the intended class.

---

Concern: Fragile static shared state

The fix resolves the cross-class interference, but _executionOrder is still a static List<string> — not thread-safe and still shared across any future tests added to HookExecutionOrderTests. The Clear() + Add("BeforeEvery") sequence is non-atomic, so if a second test method is ever added to this class and both run concurrently, the same race will resurface.

Suggested improvement — option A: Mark the class as non-parallel so it's always sequential:

[NotInParallel]
public class HookExecutionOrderTests

Suggested improvement — option B: Use per-test context storage instead of static state:

// In the global hook:
context.ObjectBag["executionOrder"] = new List<string>();
((List<string>)context.ObjectBag["executionOrder"]).Add("BeforeEvery");

// In Before(Test) and test body, read from context.ObjectBag

Option B is the more principled fix — it eliminates shared mutable state entirely and doesn't rely on filtering or parallelism attributes to prevent interference. It also scales naturally if more tests are added to the class.

---

That said, for a test-only file verifying hook ordering with a single test method, this PR is a pragmatic and correct fix for the immediate issue. The ClassType guard is idiomatic and readable. Happy to approve as-is or with the [NotInParallel] addition as a minimal safety net.

[claude]

Code Review

Fix is correct and the root cause diagnosis is accurate.

The [BeforeEvery(Test)] hook firing for all tests in the assembly while clearing a shared static list is indeed the source of the race condition. The guard on ClassType properly contains the effect to the intended class.

---

Concern: Fragile static shared state

The fix resolves the cross-class interference, but _executionOrder is still a static List<string> — not thread-safe and still shared across any future tests added to HookExecutionOrderTests. The Clear() + Add("BeforeEvery") sequence is non-atomic, so if a second test method is ever added to this class and both run concurrently, the same race will resurface.

Suggested improvement — option A: Mark the class as non-parallel so it's always sequential:

[NotInParallel]
public class HookExecutionOrderTests

Suggested improvement — option B: Use per-test context storage instead of static state:

// In the global hook:
context.ObjectBag["executionOrder"] = new List<string>();
((List<string>)context.ObjectBag["executionOrder"]).Add("BeforeEvery");

// In Before(Test) and test body, read from context.ObjectBag

Option B is the more principled fix — it eliminates shared mutable state entirely and doesn't rely on filtering or parallelism attributes to prevent interference. It also scales naturally if more tests are added to the class.

---

That said, for a test-only file verifying hook ordering with a single test method, this PR is a pragmatic and correct fix for the immediate issue. The ClassType guard is idiomatic and readable. Happy to approve as-is or with [NotInParallel] as a minimal safety net.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Updated Code Review

The PR has been updated since the previous review and now incorporates both suggested improvements:

1. The ClassType guard in GlobalHookExecutionOrderSetup.GlobalSetup — prevents cross-class interference from the [BeforeEvery(Test)] hook.
2. [NotInParallel] on HookExecutionOrderTests — prevents intra-class races between multiple test methods.

Both layers of protection are in place. The fix is correct, minimal, and addresses the root cause accurately.

Remaining minor observation

The static _executionOrder list is still shared mutable state, which means if HookExecutionOrderTests ever gains a second test method, [NotInParallel] will serialize execution and prevent races — but the Clear() call in the global hook still mutates state owned by another class, which is an unusual coupling. This is not a blocker for this PR; it is a design smell worth being aware of if the test class grows.

Verdict

The fix is pragmatic, correct, and the added [NotInParallel] provides the safety net called out in the previous review. No blocking issues. Ready to merge.

[claude]

Code Review — PR #5355: fix: resolve race condition in HookExecutionOrderTests

Summary of changes

The PR fixes a genuine flakiness bug in HookExecutionOrderTests. The original code used a static readonly List<string> _executionOrder field shared across all tests in the assembly. A [BeforeEvery(Test)] hook called Clear() on that list before every test — not just tests belonging to HookExecutionOrderTests. Under parallel execution, another test's BeforeEvery invocation could clear the list between the Before(Test) hook and the test body of VerifyExecutionOrder, causing an incorrect count.

The final state of the PR (commit cfb2fa80) takes the principled fix: it replaces the shared static list entirely with per-test StateBag storage, and adds a class-type guard in the global hook so it only acts on HookExecutionOrderTests tests.

---

What works well

Root cause is correctly diagnosed. The race was real and the described failure mode is accurate. [BeforeEvery(Test)] is assembly-scoped; calling Clear() on a static collection inside it is inherently unsafe when other tests run concurrently.

StateBag is the right abstraction here. TestContext.StateBag is per-test-context, backed by a ConcurrentDictionary<string, object?>. Using it eliminates all shared mutable state. Each test invocation gets its own List<string> initialized by GetOrAdd, so no cross-test interference is possible regardless of parallelism. This is the idiomatic TUnit approach for passing data between lifecycle hooks and test bodies.

The class-type guard is a correct secondary defence. Even with StateBag, having the global BeforeEvery hook silently add entries to the StateBag of every test in the assembly would be unexpected and wasteful. Filtering on ClassType is the right call.

Dropping [NotInParallel] (added in an intermediate commit, removed in the final one) is also the right call. The StateBag approach makes serialization unnecessary since there is no shared state left.

---

Suggestions

1. GetOrAdd in all hooks obscures ownership semantics

var order = context.StateBag.GetOrAdd<List<string>>("executionOrder", _ => []);
order.Add("BeforeEvery");

GetOrAdd is "get if present, create if absent." In GlobalSetup (the [BeforeEvery(Test)] hook), the list will never already exist — the StateBag is fresh per test context when the global hook fires first. Using it in all three sites masks which hook owns initialization vs. which hooks only read.

A more explicit pattern makes the ownership clear and turns a missing initialization into an immediate failure rather than a subtly wrong count:

// GlobalSetup: explicitly owns initialization
context.StateBag["executionOrder"] = new List<string> { "BeforeEvery" };

// InstanceSetup and test body: retrieval only
var order = (List<string>)TestContext.Current!.StateBag["executionOrder"]!;
order.Add("Before");

This is a readability improvement, not a correctness fix — the existing count assertion would catch a missing initialization either way.

2. Cross-class coupling: GlobalHookExecutionOrderSetup now depends on HookExecutionOrderTests

if (context.Metadata.TestDetails.ClassType != typeof(HookExecutionOrderTests))
    return;

GlobalHookExecutionOrderSetup has a compile-time reference to a sibling test class. For a test-only file this is perfectly acceptable, but it means renaming or moving HookExecutionOrderTests silently breaks the guard (renamed type → ClassType never matches → list is never populated → order.Count.ShouldBe(3) fails). A comment explaining the guard and the coupling would help future contributors.

An alternative is to move the [BeforeEvery(Test)] hook inside HookExecutionOrderTests as a nested setup class, collocating the concern:

public class HookExecutionOrderTests
{
    public sealed class Setup
    {
        [BeforeEvery(Test)]
        public static void GlobalSetup(TestContext context)
        {
            if (context.Metadata.TestDetails.ClassType != typeof(HookExecutionOrderTests)) return;
            // ...
        }
    }
}

This would need a quick check that TUnit's source generator picks up [BeforeEvery(Test)] on nested classes, but it would remove the cross-file coupling entirely.

---

Verdict

The fix is correct and well-reasoned. The StateBag approach is strictly better than the original static list design. No blocking issues — the two points above are readability/maintainability suggestions, not correctness concerns.

Ready to merge.