Skip to content

feat: test execution metrics collection#4954

Closed
thomhurst wants to merge 2 commits into
mainfrom
feat/test-metrics
Closed

feat: test execution metrics collection#4954
thomhurst wants to merge 2 commits into
mainfrom
feat/test-metrics

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Closes #4896. Adds --metrics flag with TestMetricsCollector.

@claude

claude Bot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Code Review

Bug: Asymmetric concurrency counter for dependency-skipped tests

The --metrics flag feature is well-designed overall — the use of Interlocked operations for thread safety, ValueTask for async methods, and the zero-overhead opt-in via nullable _metricsCollector are all solid choices.

However, there is a logic bug in how OnTestCompleted is called for dependency-skipped tests in TestRunner.cs.

The problem:

In TestRunner.cs lines 85–90, when a test is skipped due to a failed dependency, OnTestCompleted is called without a prior call to OnTestStarted:

if (dependency.Test.State != TestState.Passed && !dependency.ProceedOnFailure)
{
_testStateManager.MarkSkipped(test, "Skipped due to failed dependencies");
_metricsCollector?.OnTestCompleted(passed: false, skipped: true, duration: null);
await _tunitMessageBus.Skipped(test.Context, "Skipped due to failed dependencies").ConfigureAwait(false);
return;
}
}

OnTestStarted (which increments _currentConcurrentTests) is only called at line 97, which is after the early return on line 90:

test.StartTime = DateTimeOffset.UtcNow;
_metricsCollector?.OnTestStarted();
try
{

Meanwhile, OnTestCompleted unconditionally decrements _currentConcurrentTests as its very first operation:

public void OnTestCompleted(bool passed, bool skipped, TimeSpan? duration)
{
Interlocked.Decrement(ref _currentConcurrentTests);
if (skipped)

The consequence:

Every dependency-skipped test decrements _currentConcurrentTests without a matching increment, driving it negative. In a test suite with N dependency-skipped tests, the counter starts at -N before any real tests run. When 8 tests then run concurrently, _currentConcurrentTests only reaches 8 - N, so _peakConcurrentTests is reported as 8 - N instead of 8. The "Peak concurrent tests" metric in the summary is definitively wrong.

Suggested fix:

The cleanest fix is to add a dedicated OnTestSkippedBeforeStart method to TestMetricsCollector that only increments the skip counter without touching _currentConcurrentTests:

// In TestMetricsCollector.cs
public void OnTestSkippedBeforeStart()
{
    Interlocked.Increment(ref _totalTests);
    Interlocked.Increment(ref _skippedTests);
}

And in TestRunner.cs, replace line 88:

// Before:
_metricsCollector?.OnTestCompleted(passed: false, skipped: true, duration: null);

// After:
_metricsCollector?.OnTestSkippedBeforeStart();

This separates the "test lifecycle" tracking (_currentConcurrentTests) from the "test result" tracking (skip/pass/fail counts), which is a cleaner abstraction since a dependency-skipped test never actually started from the concurrency perspective.

Add a TestMetricsCollector service that tracks test execution statistics
and outputs a summary when enabled via the --metrics command-line flag.
Tracks total/pass/fail/skip counts, average duration, peak concurrency,
wall-clock time, and memory usage (start/end/delta).

Closes #4896
When a test is skipped due to a failed dependency, OnTestCompleted was
called without a prior OnTestStarted, causing the concurrent test
counter to go below zero. Add a dedicated OnTestSkipped method that
only increments the skip counter without touching concurrency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: test execution metrics collection and adaptive scheduling

1 participant