perf: engine hot-path allocation wins (#5528 B)

[thomhurst]

Part B of the perf work catalogued in #5528. Tackles per-test allocation wins on the execution hot path.

Items

3. `TestDetails.TestClassInjectedPropertyArguments` → lazy

Most tests inject zero properties, so the per-test `ConcurrentDictionary` allocation (~200+ bytes) is pure waste. The property now returns a shared read-only empty singleton until `GetOrCreateInjectedPropertyArguments()` is called from `PropertyInjector` on first write.

5. Nullable `cleanupExceptions`

• `ObjectTracker.UntrackObjects` returns `List?` — null on the success path
• `TestExecutor.ExecuteAfterClassAssemblyHooks` returns `List?` instead of always allocating a fresh list
• `TestCoordinator`'s `finally` block now holds a nullable and materializes only when something fails

Saves ~56 bytes per passing test (the common case).

6. `Context.HasCapturedOutput` short-circuit

Passing tests with no output skip the `GetOutput` / `GetErrorOutput` calls — and their `ReaderWriterLockSlim` acquisition — entirely when neither writer nor build-time output was ever populated.

Item 4 — skipped deliberately

The audit in #5528 proposed dropping the unconditional `Console.Out.FlushAsync() / Console.Error.FlushAsync()` calls in `TestCoordinator`'s finally block on the claim that `ConcurrentStringWriter.Flush` is a no-op. In practice `Console.Out` is the `OptimizedConsoleInterceptor`, whose `FlushAsync` flushes partial-line buffers to the output sinks — skipping it loses any `Console.Write("…")` output that didn't end in a newline. Left as-is.

Test plan

• `TUnit.Engine.Tests` — 148/148 pass
• `TUnit.UnitTests` — 180/180 pass
• `TUnit.PublicAPI` — 4/4 pass (no public surface changes)

[codacy-production]

Not up to standards ⛔

🔴 Issues 5 minor

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

Results:
5 new issues

Category	Results
CodeStyle	4 minor
Performance	1 minor

View in Codacy

🟢 Metrics 16 complexity

Metric	Results
Complexity	16

View in Codacy

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

[claude]

Code Review — PR #5610 (Part B: Hot-path allocation wins)

This PR is a draft scaffolding commit with an empty diff — the commit message explicitly says "Will contain:" for all four items. Reviewing the plan against the current codebase rather than actual code changes.

---

Item #3 — TestDetails: lazy ConcurrentDictionary for injected properties

Current state (TestDetails.cs:52):

public IDictionary<string, object?> TestClassInjectedPropertyArguments { get; init; } = new ConcurrentDictionary<string, object?>();

Plan looks good. The allocation is unconditional. Lazy initialization here is straightforward. Suggested shape:

private ConcurrentDictionary<string, object?>? _injectedPropertyArguments;
public IDictionary<string, object?> TestClassInjectedPropertyArguments =>
    _injectedPropertyArguments ??= new ConcurrentDictionary<string, object?>();

init needs to be dropped in favour of a lazy backing field. Make sure PropertyInjector and any reflection-mode code that writes to this property can still do so post-construction (i.e., nothing assumes the dict is the same instance set at init time).

---

Item #4 — TestCoordinator: remove Console.Out/Error.FlushAsync ⚠️

Current state (TestCoordinator.cs:143-144):

await Console.Out.FlushAsync().ConfigureAwait(false);
await Console.Error.FlushAsync().ConfigureAwait(false);

Concern: OptimizedConsoleInterceptor.FlushAsync is NOT a no-op.

The issue tracking doc (#5528) states "TUnit's ConcurrentStringWriter.Flush is a no-op", but the actual Console.Out replacement is OptimizedConsoleInterceptor, and its FlushAsync (OptimizedConsoleInterceptor.cs:75-82) does real work:

public override async Task FlushAsync()
{
    var content = GetLineBuffer().FlushIfNonEmpty();
    if (content != null)
    {
        await RouteToSinksAsync(content).ConfigureAwait(false);
    }
}

This flushes any partial line (e.g., Console.Write("no newline")) to the output sinks. Removing these calls unconditionally would silently drop partial-line output from tests.

Recommended fix: Guard rather than remove:

// Only flush if the writer is our interceptor; other TextWriter impls may have real state
if (Console.Out is OptimizedConsoleInterceptor outInterceptor)
    await outInterceptor.FlushAsync().ConfigureAwait(false);
else
    await Console.Out.FlushAsync().ConfigureAwait(false);

Or even simpler — keep the calls but only skip them when it's confirmed there's no buffered content. The async state machine overhead cited in the issue is negligible compared to a silently lost test output bug.

---

Item #5 — TestCoordinator: keep cleanupExceptions null on success path

Current state:

List<Exception>? cleanupExceptions = null;          // line 137 — nullable ✓
// ...
await _objectTracker.UntrackObjects(test.Context, cleanupExceptions ??= [])  // line 151 — forces alloc ✗

Plan is correct. UntrackObjects signature (ObjectTracker.cs:102) takes List<Exception> (non-nullable), which is what forces the ??= []. The fix needs to thread nullable through:

1. Change UntrackObjects to accept List<Exception>? (or return exceptions rather than accepting a collector)
2. Keep cleanupExceptions null on the no-exception path

The return-exceptions pattern is cleaner long-term (no shared mutable list passed by ref), but changing the signature is the minimal fix.

---

Item #6 — AbstractExecutableTest: defer output string concat

Partially already done. AbstractExecutableTest.cs:75-83 already has the guard:

// Lazy output building - avoid string allocation when there's no output
var output = Context.GetOutput();
var errorOutput = Context.GetErrorOutput();
var combinedOutput = string.Empty;

if (output.Length > 0 || errorOutput.Length > 0)
{
    combinedOutput = string.Concat(output, ...);
}

But GetOutput() and GetErrorOutput() (TestContext.Output.cs:99-101) still allocate strings to check length. The string.Concat is deferred, but the intermediate strings are not.

True deferral would mean storing the writers in TestResult.Output as a Func<string> or a lazy wrapper, building the string only when a reporter reads it. That's a larger API change — worth flagging whether the partial win here is sufficient or if the full lazy approach is in scope for this PR.

---

Summary

Item	Plan Correctness	Key Risk
#3 Lazy ConcurrentDictionary	✅ Sound	Verify init → backing field migration
#4 Remove Console flushes	⚠️ Incorrect premise	OptimizedConsoleInterceptor.FlushAsync is NOT a no-op
#5 Nullable cleanupExceptions	✅ Sound	Requires UntrackObjects signature change
#6 Defer output concat	⚠️ Partially done	GetOutput()/GetErrorOutput() still allocate

Item #4 is the main blocker — please validate whether OptimizedConsoleInterceptor.FlushAsync needs to be called at test teardown before removing it. The other items look like good wins.

[claude]

Code Review — PR #5610 (Part B: Hot-path allocation wins — actual implementation)

This is the follow-up review on the real diff. Previous review was on an empty scaffold commit. Checking previous concerns against the implementation.

---

Previous feedback addressed ✅

Item #4 kept as-is — the PR description explicitly explains why Console.Out.FlushAsync() / Console.Error.FlushAsync() were preserved (OptimizedConsoleInterceptor.FlushAsync is not a no-op). This directly addresses my earlier concern. Good call.

Item #5 — Nullable cleanupExceptions — UntrackObjects now returns ValueTask<List<Exception>?> and only materializes the list on failure. The Interlocked.CompareExchange in GetOrCreateInjectedPropertyArguments() is correct for thread-safe lazy init.

Item #6 — Output short-circuit — The HasCapturedOutput approach is actually better than just deferring the string concat: it skips the ReaderWriterLockSlim acquisition entirely, not just the string.Concat. Cleaner than the Func<string> lazy wrapper I suggested.

---

Concern: source-gen write sites for TestClassInjectedPropertyArguments

The public property now returns a ReadOnlyDictionary singleton when no properties have been injected. Any code that holds a reference and calls ["key"] = value on it will get a NotSupportedException at runtime.

PropertyInjector.cs has been updated to use GetOrCreateInjectedPropertyArguments() in all three places. But the source-gen path also writes to this property — has the generated code (or any reflection-mode code) been audited for direct dictionary mutation through the property? If any source-generated test setup does:

testDetails.TestClassInjectedPropertyArguments[key] = value;

instead of going through GetOrCreateInjectedPropertyArguments(), it will throw NotSupportedException — and only when that test class actually has injected properties (making it hard to catch in general runs). This is dual-mode critical per CLAUDE.md.

Recommended check: grep -r "TestClassInjectedPropertyArguments\[" --include="*.cs" across both the engine and source-generator output to confirm all write paths are covered.

---

Minor: inconsistent pattern in ObjectTracker.UntrackObjectsAsync

The two catch blocks handle the list differently:

// First catch — one-liner:
(cleanupExceptions ??= []).Add(e);

// Second catch — two statements:
cleanupExceptions ??= [];
foreach (var e in whenAllTask.Exception!.InnerExceptions)
    cleanupExceptions.Add(e);

The second could use .AddRange() for symmetry:

(cleanupExceptions ??= []).AddRange(whenAllTask.Exception!.InnerExceptions);

Minor, but the inconsistency is mildly surprising.

---

TestDetails.cs — implementation quality

The lazy init with Interlocked.CompareExchange is correct:

return Interlocked.CompareExchange(ref _testClassInjectedPropertyArguments, created, null) ?? created;

• CAS succeeds (was null) → returns null → ?? created → returns created ✓
• CAS fails (non-null) → returns existing → ?? created skipped → returns existing ✓

The init setter handling all three IDictionary cases (null, ConcurrentDictionary<,>, other) is thorough and defensive.

---

Summary

Item	Status
Item #4 flush calls kept	✅ Correctly preserved
Item #3 lazy ConcurrentDictionary	✅ Thread-safe, well-structured
Item #5 nullable cleanup list	✅ Clean return-instead-of-collector pattern
Item #6 HasCapturedOutput short-circuit	✅ Better than suggested approach
Source-gen write sites audit	⚠️ Needs verification before merge
Minor catch inconsistency	ℹ️ Cosmetic

The implementation is solid. The one thing worth a quick grep before merge is confirming no source-gen or reflection-mode code bypasses GetOrCreateInjectedPropertyArguments() with a direct dictionary write.

[claude]

Code Review — PR #5610 (Part B: Hot-path allocation wins)

Previous review was on a scaffolding commit with an empty diff. This review covers the actual implementation.

---

Previous Concerns — Addressed ✅

Both items flagged in the earlier review have been resolved:

• **Write-site audit for ** — confirmed clean. All writes in PropertyInjector.cs now go through GetOrCreateInjectedPropertyArguments(). The public TestClassInjectedPropertyArguments property only exposes get + init, so post-construction writes via the property accessor are impossible at compile time.
• AddRange inconsistency in ObjectTracker.UntrackObjectsAsync — the second catch block now uses (cleanupExceptions ??= []).AddRange(...) consistently with the first. ✅

---

Implementation Quality

TestDetails.cs — lazy ConcurrentDictionary

The Interlocked.CompareExchange pattern in GetOrCreateInjectedPropertyArguments() is correct:

• CAS succeeds (was null) → returns null → ?? created → created ✓
• CAS fails (race) → returns existing → ?? created skipped ✓

The init setter handling all three IDictionary shapes (null, ConcurrentDictionary\<,\>, other) is thorough and defensive. The EmptyInjectedPropertyArguments.Instance as a ReadOnlyDictionary is a good choice — any accidental write through the interface (bypassing GetOrCreateInjectedPropertyArguments) would throw NotSupportedException loudly rather than silently creating a new dictionary.

Context.HasCapturedOutput short-circuit

The internal virtual / internal override pattern across Context and TestContext.Output is well-structured. The implementation correctly checks all four backing fields across the two levels.

Nullable exception list — return-instead-of-collector pattern

Cleaner than the previous parameter-passing approach. The propagation through ObjectTracker → ObjectLifecycleService → TestCoordinator is consistent.

---

One Minor Observation

In TestCoordinator.cs:

if (cleanupExceptions is null)
{
    cleanupExceptions = hookExceptions;  // list aliasing
}
else
{
    cleanupExceptions.AddRange(hookExceptions);
}

When cleanupExceptions is null, it's assigned the hookExceptions list directly (list reuse — saves an allocation). Then later in the same finally block, the catches at lines 192/205/218 can append to cleanupExceptions, which is now the same list object that ExecuteAfterClassAssemblyHooks returned. This is correct (the returned list is owned here), but it's a subtle aliased-mutation pattern. A comment noting the intentional reuse would help a future reader. Not a blocker.

---

Summary

Item	Status
Item #3 — lazy ConcurrentDictionary	✅ Thread-safe, well-structured
Item #4 — console flush calls preserved	✅ Correctly documented why
Item #5 — nullable cleanup list	✅ Clean, propagates consistently
Item #6 — HasCapturedOutput short-circuit	✅ Correct at both inheritance levels
Write-site audit (prev review concern)	✅ All paths confirmed safe
AddRange inconsistency (prev review)	✅ Fixed
List aliasing in TestCoordinator	ℹ️ Cosmetic — add a comment if desired

Implementation is solid and the optimizations are well-reasoned. LGTM.

[claude]

Code Review — PR #5610 (Part B: Hot-path allocation wins)

Previous review was on a scaffolding commit with an empty diff. This review covers the actual implementation.

---

Previous Concerns — Addressed ✅

Both items flagged in the earlier review have been resolved:

• Write-site audit for GetOrCreateInjectedPropertyArguments() — confirmed clean. All writes in PropertyInjector.cs now go through GetOrCreateInjectedPropertyArguments(). The public TestClassInjectedPropertyArguments property only exposes get + init, so post-construction writes via the property accessor are impossible at compile time.
• AddRange inconsistency in ObjectTracker.UntrackObjectsAsync — the second catch block now uses (cleanupExceptions ??= []).AddRange(...) consistently with the first. ✅

---

Implementation Quality

TestDetails.cs — lazy ConcurrentDictionary

The Interlocked.CompareExchange pattern in GetOrCreateInjectedPropertyArguments() is correct:

• CAS succeeds (was null) → returns null → ?? created → created ✓
• CAS fails (race) → returns existing → ?? created skipped ✓

The init setter handling all three IDictionary shapes (null, ConcurrentDictionary<,>, other) is thorough and defensive. The EmptyInjectedPropertyArguments.Instance as a ReadOnlyDictionary is a good choice — any accidental write through the interface (bypassing GetOrCreateInjectedPropertyArguments) would throw NotSupportedException loudly rather than silently creating a new dictionary.

Context.HasCapturedOutput short-circuit

The internal virtual / internal override pattern across Context and TestContext.Output is well-structured. The implementation correctly checks all four backing fields across the two levels.

Nullable exception list — return-instead-of-collector pattern

Cleaner than the previous parameter-passing approach. The propagation through ObjectTracker → ObjectLifecycleService → TestCoordinator is consistent.

---

One Minor Observation

In TestCoordinator.cs:

if (cleanupExceptions is null)
{
    cleanupExceptions = hookExceptions;  // list aliasing
}
else
{
    cleanupExceptions.AddRange(hookExceptions);
}

When cleanupExceptions is null, it's assigned the hookExceptions list directly (list reuse — saves an allocation). Then later in the same finally block, the catches at lines 192/205/218 can append to cleanupExceptions, which is now the same list object that ExecuteAfterClassAssemblyHooks returned. This is correct (the returned list is owned here), but it's a subtle aliased-mutation pattern. A comment noting the intentional reuse would help a future reader. Not a blocker.

---

Summary

Item	Status
Item #3 — lazy ConcurrentDictionary	✅ Thread-safe, well-structured
Item #4 — console flush calls preserved	✅ Correctly documented why
Item #5 — nullable cleanup list	✅ Clean, propagates consistently
Item #6 — HasCapturedOutput short-circuit	✅ Correct at both inheritance levels
Write-site audit (prev review concern)	✅ All paths confirmed safe
AddRange inconsistency (prev review)	✅ Fixed
List aliasing in TestCoordinator	ℹ️ Cosmetic — add a comment if desired

Implementation is solid and the optimizations are well-reasoned. LGTM.