perf(engine): collapse async forwarding wrappers in test execution (#5714)

[thomhurst]

Summary

• Collapses the pure-forward TestCoordinator.ExecuteTestAsync → ExecuteTestInternalAsync wrapper into a single method, eliminating one async state machine per test.
• Inlines the no-retry ExecuteTestLifecycleAsync body directly into ExecuteTestAsync. The retry branch still dispatches through ExecuteTestLifecycleAsync because RetryHelper.ExecuteWithRetry requires a Func<ValueTask>.
• Net effect on the hot no-retry path: ~2 async state machines removed per test (consistent with the profiler evidence in perf: collapse 4-layer async forwarding chain in test execution #5714).

TestRunner.ExecuteTestAsync was also audited. It is not a pure forward — it owns the session-wide dedup ledger (ConcurrentDictionary<string, TCS<bool>>) and already uses a minimal WrapAsync helper that only spins a state machine on the non-synchronous path, so it is already optimized and left alone.

TestExecutor.ExecuteAsync was audited and performs substantial real work (hook orchestration, activity lifecycle, timeout wiring, exception aggregation) — not a forwarder; left alone.

Risks

• Intentional duplication: the no-retry inlined body is a verbatim copy of ExecuteTestLifecycleAsync. Both locations carry cross-reference comments so future edits stay in sync. If the lifecycle gets any more complex, consider reverting to a single method and accepting the state-machine cost, or refactoring into AOT-safe non-async helpers.
• Retry, cancellation, and exception semantics are preserved exactly (skip checks run in the same order as before; dispose-with-span still fires in finally).

Test plan

• dotnet build TUnit.slnx -c Release — succeeds (0 errors).
• TUnit.Core.SourceGenerator.Tests snapshot tests on net10.0 — 116 passed, 1 skipped (pre-existing).
• TUnit.TestProject BasicTests smoke — 3/3 passed.
• TUnit.TestProject RetryTests — retry counts exactly match expectations (AssertCounts passes; 3 intentional failure tests fail after the expected attempt counts: 2, 3, 4 — confirming retry wrapper path still works).
• TUnit.TestProject DependsOnTests — 2/2 passed (dependency-chain path via TestRunner).

Closes #5714

[claude]

Review: perf(engine) collapse async forwarding wrappers in test execution

Summary

The change is well-motivated, correctly scoped, and includes solid profiler evidence from the related issue. The execution semantics are preserved, the test plan is thorough, and the PR description is unusually detailed and honest about trade-offs. No correctness issues found.

---

Duplication Style: if/else vs. early-return

The only structural difference between the inlined no-retry body (lines 120–161) and the original ExecuteTestLifecycleAsync (lines 337–378) is that the inlined version uses nested if/else instead of early return. Both are functionally identical, but the if/else nesting adds one level of indentation for every skip path, which can grow if future skip conditions are added.

Consider whether the inlined version should use the same guard-clause / early-return style as ExecuteTestLifecycleAsync to make future divergence easier to spot:

// No-retry fast path (inlined)
test.Context.CurrentRetryAttempt = 0;

if (!string.IsNullOrEmpty(test.Context.SkipReason))
{
    _stateManager.MarkSkipped(test, test.Context.SkipReason!);
    await _eventReceiverOrchestrator.InvokeTestSkippedEventReceiversAsync(test.Context, cancellationToken).ConfigureAwait(false);
    await _eventReceiverOrchestrator.InvokeTestEndEventReceiversAsync(test.Context, cancellationToken).ConfigureAwait(false);
    return; // ← early return instead of wrapping the else branch
}
// … rest of lifecycle

This mirrors ExecuteTestLifecycleAsync exactly and makes the "these two must stay in sync" requirement much easier to verify visually. Right now you need to mentally flatten the nested if/else to confirm they are equivalent.

---

The real maintainability concern: comment-enforced synchronisation

The PR is clear-eyed about the risk:

"Both locations carry cross-reference comments so future edits stay in sync."

This works, but comment-enforced duplication is fragile under code review pressure — the comment is easy to miss when editing one site. A few approaches worth weighing against each other, in order of complexity:

1. Keep the current approach (what the PR does) — acceptable for a small, stable lifecycle with low churn. The cross-reference comments and PR notes are better than average.

2. Extract a non-async synchronous helper for the skip-before-instance-creation guard and the instance-creation-then-skip check:

   // Returns true and marks/fires skipped if the test should be skipped; false otherwise.
   private bool TryHandlePreExecutionSkip(AbstractExecutableTest test, CancellationToken ct) { ... }

   The actual hot-path allocation saving comes from eliminating the async wrapper, not from keeping the logic monolithic. A sync helper called by both paths does not add a state machine.

3. Lift to a [MethodImpl(MethodImplOptions.AggressiveInlining)] non-async helper — the compiler will often inline small helpers anyway, but the attribute makes the intent explicit.

4. AOT-safe iterator / pipeline approach — likely over-engineering for this file; not recommended here.

Option 2 is the most pragmatic upgrade: the skip-check logic is already clear enough that extracting TryHandlePreExecutionSkip would be a net reduction in duplication with no runtime cost, while keeping both paths readable without scrolling between them. This would eliminate the "must stay in sync" risk entirely.

---

Minor observations

• The retryLimit == 0 fast-path skip check (lines 120–127) is now the third place in the same method that calls MarkSkipped + InvokeTestSkippedEventReceiversAsync + InvokeTestEndEventReceiversAsync. The first is the early-exit at lines 72–78 (pre-registration skip), the second is lines 120–127, and the third is lines 141–145. Extracting a HandleSkip(test, reason, ct) helper for that triple would reduce the total line count and make each use-site one line rather than four.

• The RetryHelper.ExecuteWithRetry method re-reads testContext.Metadata.TestDetails.RetryLimit inside the loop (line 14 of RetryHelper.cs), while TestCoordinator already holds retryLimit in a local. This is a pre-existing minor issue unrelated to this PR, but worth noting if anyone is profiling allocations in that class.

---

Bottom line

The optimisation is sound, the implementation is correct, and the PR description does an excellent job of documenting the risk. The main suggestion is to adopt early-return style in the inlined block to match ExecuteTestLifecycleAsync, making future sync checks trivial. Extracting a HandleSkip helper is a worthwhile follow-up to reduce the triple-repetition of the skip-event firing sequence.

Approving with the above non-blocking suggestions.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 4 complexity

Metric	Results
Complexity	4

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

[thomhurst]

Applied both non-blocking review suggestions in ea534fa:

• Extracted HandleSkipAsync private helper (mark state + skipped receivers + end receivers) and routed all three skip sites through it (pre-registration, fast-path pre-instance, fast-path post-instance + the two matching sites in ExecuteTestLifecycleAsync).
• Converted the inlined no-retry fast path to early-return style so its body now mirrors ExecuteTestLifecycleAsync shape for shape, making the "must stay in sync" maintenance contract trivial to verify by diff.

Semantics preserved: MarkCompleted is idempotent on an already-skipped test (MarkSkipped set test.Result, so Result ??= is a no-op), so the early return in the fast-path skip branches is observationally equivalent to the previous fall-through via MarkCompleted. Retry, cancellation, and exception handling paths are unchanged.

Validation:

• dotnet build TUnit.slnx -c Release - 0 errors
• TUnit.Core.SourceGenerator.Tests snapshots - 116/117 passed, 1 skipped (pre-existing flaky)
• BasicTests/* smoke - 3/3 passed
• Skip-related suites (SkipTests, SimpleSkipTest, SkipConstructorTest, SkipExceptionFixTest, DynamicSkipReasonTests, RunOnSkipTests, SkippedEventReceiverTests, RuntimeSkipEventReceiverTests) - 24 passed, 1 intentionally skipped
• Retry suites (CustomRetryTests, ScopedRetryAttributePriorityTests, SimpleRetryPriorityTest) - failures match baseline on the pre-refactor commit (designed-to-fail tests that throw intentionally; retry-count AssertCounts passes)