fix(tests): snapshot ExecutionLog under lock to fix parallel race

[thomhurst]

Problem

ExecutionOrderReproductionTests (TUnit.Example.Asp.Net.TestProject) intermittently fails on CI with:

InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNext()

Observed on modularpipeline (ubuntu-latest) (e.g. PR #6188, where it is unrelated to the change under test).

Root cause

ExecutionLog is a shared static List<string>. Log() takes a lock when writing, but the final foreach (var entry in ExecutionLog) enumerated the list without the lock. Tests in the class run in parallel, and their lifecycle hooks (ConfigureTestOptions, SetupAsync, ConfigureTestConfiguration, …) all call Log() → Add(). One instance mutating the list while another enumerates it throws.

Fix

Snapshot the list under the lock before enumerating. Test-only change.

[claude]

Code Review

Summary

Clean, minimal fix for a real flaky-test race condition. The diagnosis is accurate and the solution is correct.

---

What the PR does

ExecutionLog is a static List<string> shared across all parallel test instances in the class. Log() correctly acquires Lock before calling Add(), but the foreach at the end of Api_Should_Start_After_ConfigureTestConfiguration enumerated the list without holding the lock. Parallel test instances calling Log() from their lifecycle hooks (ConfigureTestOptions, SetupAsync, etc.) could mutate the list mid-enumeration, throwing the observed InvalidOperationException.

The fix snapshots the list inside the lock before enumerating:

List<string> executionLogSnapshot;
lock (Lock)
{
    executionLogSnapshot = ExecutionLog.ToList();
}
foreach (var entry in executionLogSnapshot) { ... }

This is the idiomatic, correct pattern for this scenario.

---

Findings

No issues found. A few observations for completeness:

1. The fix is sufficient and well-scoped. Only the enumeration site needed the lock; the snapshot approach avoids holding the lock during I/O (Console.WriteLine), which is the right call.

2. The inline comment is justified. CLAUDE.md allows comments for non-obvious invariants/workarounds — a shared static list with parallel test mutation is exactly that kind of subtle constraint.

3. No other enumeration sites were missed. Connection_String_Should_Be_Overridden and other tests don't enumerate ExecutionLog, so this is the only callsite that needed fixing.

4. Alternative considered (not needed here): Replacing List<string> + lock with ConcurrentBag<string> would avoid needing a separate lock object entirely, but since enumeration order matters for the diagnostic log and the existing pattern is already established, this would be a separate cleanup — not required for the correctness fix.

LGTM. This is a correct, test-only fix that eliminates a real source of CI flakiness.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

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.}