Skip to content

Commit e0d59a0

Browse files
thomhurstclaude
thomhurst
and
claude
committed
perf: bypass RetryHelper and TimeoutHelper when not needed
Add fast path that bypasses wrapper overhead for tests without retry or timeout configuration: - Set CurrentRetryAttempt = 0 for consistency with slow path - Add clarifying comment about retryLimit semantics - Extract ExecuteTestLifecycleAsync for clean separation Fixes #4288 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 984b19c commit e0d59a0

1 file changed

Lines changed: 87 additions & 71 deletions

File tree

‎TUnit.Engine/Services/TestExecution/TestCoordinator.cs‎

Lines changed: 87 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -103,78 +103,33 @@ private async ValueTask ExecuteTestInternalAsync(AbstractExecutableTest test, Ca
103103
// Ensure TestSession hooks run before creating test instances
104104
await _testExecutor.EnsureTestSessionHooksExecutedAsync(cancellationToken).ConfigureAwait(false);
105105

106-
// Execute test with retry logic - each retry gets a fresh instance
107-
// Timeout is applied per retry attempt, not across all retries
108-
await RetryHelper.ExecuteWithRetry(test.Context, async () =>
106+
// Check if we can use the fast path (no retry, no timeout)
107+
// Note: retryLimit == 0 means "no retries" (run once), not "unlimited retries"
108+
var retryLimit = test.Context.Metadata.TestDetails.RetryLimit;
109+
var testTimeout = test.Context.Metadata.TestDetails.Timeout;
110+
111+
if (retryLimit == 0 && !testTimeout.HasValue)
109112
{
110-
// Get timeout configuration for this attempt
111-
var testTimeout = test.Context.Metadata.TestDetails.Timeout;
112-
var timeoutMessage = testTimeout.HasValue
113-
? $"Test '{test.Context.Metadata.TestDetails.TestName}' timed out after {testTimeout.Value}"
114-
: null;
115-
116-
// Wrap entire lifecycle (instance creation, initialization, execution) with timeout
117-
await TimeoutHelper.ExecuteWithTimeoutAsync(
118-
async ct =>
119-
{
120-
test.Context.Metadata.TestDetails.ClassInstance = await test.CreateInstanceAsync().ConfigureAwait(false);
121-
122-
// Invalidate cached eligible event objects since ClassInstance changed
123-
test.Context.CachedEligibleEventObjects = null;
124-
125-
// Check if this test should be skipped (after creating instance)
126-
if (test.Context.Metadata.TestDetails.ClassInstance is SkippedTestInstance ||
127-
!string.IsNullOrEmpty(test.Context.SkipReason))
128-
{
129-
await _stateManager.MarkSkippedAsync(test, test.Context.SkipReason ?? "Test was skipped").ConfigureAwait(false);
130-
131-
await _eventReceiverOrchestrator.InvokeTestSkippedEventReceiversAsync(test.Context, ct).ConfigureAwait(false);
132-
133-
await _eventReceiverOrchestrator.InvokeTestEndEventReceiversAsync(test.Context, ct).ConfigureAwait(false);
134-
135-
return;
136-
}
137-
138-
try
139-
{
140-
_testInitializer.PrepareTest(test, ct);
141-
test.Context.RestoreExecutionContext();
142-
await _testExecutor.ExecuteAsync(test, _testInitializer, ct).ConfigureAwait(false);
143-
}
144-
finally
145-
{
146-
// Dispose test instance and fire OnDispose after each attempt
147-
// This ensures each retry gets a fresh instance
148-
var onDispose = test.Context.InternalEvents.OnDispose;
149-
if (onDispose?.InvocationList != null)
150-
{
151-
foreach (var invocation in onDispose.InvocationList)
152-
{
153-
try
154-
{
155-
await invocation.InvokeAsync(test.Context, test.Context).ConfigureAwait(false);
156-
}
157-
catch (Exception disposeEx)
158-
{
159-
await _logger.LogErrorAsync($"Error during OnDispose for {test.TestId}: {disposeEx}").ConfigureAwait(false);
160-
}
161-
}
162-
}
163-
164-
try
165-
{
166-
await TestExecutor.DisposeTestInstance(test).ConfigureAwait(false);
167-
}
168-
catch (Exception disposeEx)
169-
{
170-
await _logger.LogErrorAsync($"Error disposing test instance for {test.TestId}: {disposeEx}").ConfigureAwait(false);
171-
}
172-
}
173-
},
174-
testTimeout,
175-
cancellationToken,
176-
timeoutMessage).ConfigureAwait(false);
177-
}).ConfigureAwait(false);
113+
// Fast path: direct execution without wrapper overhead
114+
test.Context.CurrentRetryAttempt = 0;
115+
await ExecuteTestLifecycleAsync(test, cancellationToken).ConfigureAwait(false);
116+
}
117+
else
118+
{
119+
// Slow path: use retry and timeout wrappers
120+
await RetryHelper.ExecuteWithRetry(test.Context, async () =>
121+
{
122+
var timeoutMessage = testTimeout.HasValue
123+
? $"Test '{test.Context.Metadata.TestDetails.TestName}' timed out after {testTimeout.Value}"
124+
: null;
125+
126+
await TimeoutHelper.ExecuteWithTimeoutAsync(
127+
ct => ExecuteTestLifecycleAsync(test, ct),
128+
testTimeout,
129+
cancellationToken,
130+
timeoutMessage).ConfigureAwait(false);
131+
}).ConfigureAwait(false);
132+
}
178133

179134
await _stateManager.MarkCompletedAsync(test).ConfigureAwait(false);
180135

@@ -306,4 +261,65 @@ private void CollectAllDependencies(AbstractExecutableTest test, HashSet<TestDet
306261
}
307262
}
308263
}
264+
265+
/// <summary>
266+
/// Core test lifecycle execution: instance creation, initialization, execution, and disposal.
267+
/// Extracted to allow bypassing retry/timeout wrappers when not needed.
268+
/// </summary>
269+
private async Task ExecuteTestLifecycleAsync(AbstractExecutableTest test, CancellationToken cancellationToken)
270+
{
271+
test.Context.Metadata.TestDetails.ClassInstance = await test.CreateInstanceAsync().ConfigureAwait(false);
272+
273+
// Invalidate cached eligible event objects since ClassInstance changed
274+
test.Context.CachedEligibleEventObjects = null;
275+
276+
// Check if this test should be skipped (after creating instance)
277+
if (test.Context.Metadata.TestDetails.ClassInstance is SkippedTestInstance ||
278+
!string.IsNullOrEmpty(test.Context.SkipReason))
279+
{
280+
await _stateManager.MarkSkippedAsync(test, test.Context.SkipReason ?? "Test was skipped").ConfigureAwait(false);
281+
282+
await _eventReceiverOrchestrator.InvokeTestSkippedEventReceiversAsync(test.Context, cancellationToken).ConfigureAwait(false);
283+
284+
await _eventReceiverOrchestrator.InvokeTestEndEventReceiversAsync(test.Context, cancellationToken).ConfigureAwait(false);
285+
286+
return;
287+
}
288+
289+
try
290+
{
291+
_testInitializer.PrepareTest(test, cancellationToken);
292+
test.Context.RestoreExecutionContext();
293+
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken).ConfigureAwait(false);
294+
}
295+
finally
296+
{
297+
// Dispose test instance and fire OnDispose after each attempt
298+
// This ensures each retry gets a fresh instance
299+
var onDispose = test.Context.InternalEvents.OnDispose;
300+
if (onDispose?.InvocationList != null)
301+
{
302+
foreach (var invocation in onDispose.InvocationList)
303+
{
304+
try
305+
{
306+
await invocation.InvokeAsync(test.Context, test.Context).ConfigureAwait(false);
307+
}
308+
catch (Exception disposeEx)
309+
{
310+
await _logger.LogErrorAsync($"Error during OnDispose for {test.TestId}: {disposeEx}").ConfigureAwait(false);
311+
}
312+
}
313+
}
314+
315+
try
316+
{
317+
await TestExecutor.DisposeTestInstance(test).ConfigureAwait(false);
318+
}
319+
catch (Exception disposeEx)
320+
{
321+
await _logger.LogErrorAsync($"Error disposing test instance for {test.TestId}: {disposeEx}").ConfigureAwait(false);
322+
}
323+
}
324+
}
309325
}

0 commit comments

Comments
 (0)