perf: bypass RetryHelper and TimeoutHelper when not needed

[thomhurst]

Summary

• Adds fast path that bypasses RetryHelper and TimeoutHelper wrappers when test has no retry limit and no timeout
• Eliminates ~10% per-test CPU overhead from async state machines and lambda allocations for standard tests
• Extracts ExecuteTestLifecycleAsync method for clean separation

Fixes #4288

Test plan

• Unit tests pass (94/94)
• Benchmarks show improved execution time for tests without retry/timeout

🤖 Generated with Claude Code

[thomhurst]

Summary

Performance optimization that bypasses RetryHelper and TimeoutHelper wrappers for tests without retry or timeout configuration.

Critical Issues

1. Logic Error: Fast path condition is incorrect ⚠️

The fast path condition at TestCoordinator.cs:110 checks:

if (retryLimit == 0 && !testTimeout.HasValue)

However, looking at RetryHelper.cs:11, the retry loop runs for maxRetries + 1 iterations:

for (var attempt = 0; attempt < maxRetries + 1; attempt++)

This means:

• retryLimit = 0 → runs 1 time (attempt 0)
• retryLimit = 1 → runs 2 times (attempts 0, 1)

The fast path is being taken when retryLimit == 0, which is correct behavior. However, this deserves a comment explaining that a retryLimit of 0 means "no retries, run once" to avoid future confusion.

2. Missing context update ⚠️

In the slow path (TestCoordinator.cs:108-133), testContext.CurrentRetryAttempt is set by RetryHelper.cs:13. But in the fast path (TestCoordinator.cs:112), this field is never set. This could cause tests to have an incorrect retry attempt value (likely 0 or whatever it was previously).

Fix needed:

if (retryLimit == 0 && !testTimeout.HasValue)
{
    test.Context.CurrentRetryAttempt = 0;  // Add this line
    await ExecuteTestLifecycleAsync(test, cancellationToken).ConfigureAwait(false);
}

Suggestions

1. Add clarifying comment
Consider adding a comment at TestCoordinator.cs:110 explaining:

// Fast path: retryLimit == 0 means "no retries" (run once), not "unlimited retries"
if (retryLimit == 0 && !testTimeout.HasValue)

2. Performance validation
The PR description mentions benchmarks but marks them as incomplete. Consider running benchmarks to validate the ~10% improvement claim before merging.

Verdict

⚠️ REQUEST CHANGES - Critical issue #2 must be fixed (missing CurrentRetryAttempt assignment in fast path)

[thomhurst]

Addressed review feedback:

• Added test.Context.CurrentRetryAttempt = 0 in the fast path for consistency
• Added clarifying comment that retryLimit == 0 means "no retries" (run once)

[thomhurst]

Summary

Adds fast-path optimization that bypasses RetryHelper and TimeoutHelper wrappers when tests have no retry limit and no timeout.

Critical Issues

None found ✅

Suggestions

The optimization looks solid. A few minor observations:

1. Consider documenting the semantics in the extracted method (TUnit.Engine/Services/TestExecution/TestCoordinator.cs:269): The new method ExecuteTestLifecycleAsync is well-documented, but it might be helpful to note that this method is called both directly (fast path) and via RetryHelper/TimeoutHelper wrappers (slow path).

2. The .gitignore change is unrelated (.gitignore:438-440): Adding .worktrees/ appears unrelated to the performance optimization. Consider splitting this into a separate commit or PR for cleaner history.

3. Fast path correctness verification: The fast path correctly sets CurrentRetryAttempt = 0 to match RetryHelper behavior for the first attempt. The logic properly identifies tests without retry/timeout needs.

Verdict

✅ APPROVE - No critical issues

The performance optimization follows TUnit Performance First principle (Rule 4) and eliminates unnecessary wrapper overhead for the common case. The extracted ExecuteTestLifecycleAsync method improves maintainability by eliminating code duplication between the fast and slow paths.