feat: add test discovery Activity span for tracing

[thomhurst]

Summary

• Adds a "test discovery" Activity span as a child of the session span, giving users visibility into how long test discovery takes vs execution
• Extracts shared DiscoverAndResolveTestsAsync helper to deduplicate discovery logic between DiscoverTests and DiscoverTestsFullyStreamingAsync
• Removes dead AreAllDependenciesSatisfied method
• Fixes pre-existing double-registration of tests in the non-filter discovery path
• Updates OpenTelemetry docs with the new span hierarchy and attributes

Details

The discovery span wraps metadata collection, test building, and dependency resolution. It is created after [Before(TestDiscovery)] hooks run (which is after [Before(TestSession)] hooks where users register their TracerProvider), so HasListeners() correctly detects the listener.

Span hierarchy is now:

test session
  ├── test discovery
  ├── test assembly
  │     └── test suite
  │           └── test case

Zero cost when no listeners are attached — HasListeners() returns false and StartActivity returns null.

The RegisterTest call was previously invoked twice per test in the streaming (non-filter) path — once inside DiscoverTestsStreamAsync and again in the shared registration loop. This has been fixed by moving RegisterTest into the filter path only, where DiscoverTestsStreamAsync is not used.

Test plan

• Verify dotnet build TUnit.Engine succeeds on all TFMs
• Verify existing OpenTelemetry example project still works
• Confirm discovery span appears in trace output when a listener is configured
• Confirm no span is created when no listener is attached

---

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

[claude]

Code Review

Overall this is a well-structured PR. The refactoring is clean, the span lifecycle is handled correctly, and the zero-cost guarantee when no listeners are attached is properly implemented.

Observations

Double-registration in the non-filter path (pre-existing, not a regression)

calls for each test as it yields. Then loops over the collected and calls again (line 165-168). This double-registration existed before this PR, so the refactoring preserves the existing behaviour. Worth confirming that RegisterTest is idempotent (i.e., safe to call twice for the same test).

_ = ex; in the #else catch branch (minor)

catch (Exception ex)
{
#if NET
    TUnitActivitySource.RecordException(discoveryActivity, ex);
#else
    _ = ex;  // suppress unused-variable warning
#endif
    throw;
}

This works and suppresses the compiler warning, but an alternative that avoids the discard entirely is to use a regular catch with no variable in the #else case:

#if NET
        catch (Exception ex)
        {
            TUnitActivitySource.RecordException(discoveryActivity, ex);
            throw;
        }
#else
        catch
        {
            throw;
        }
#endif

This is more explicit about intent for the non-NET build, though the current approach is perfectly readable.

Parameter ordering (minor)

private async Task<List<AbstractExecutableTest>> DiscoverAndResolveTestsAsync(
    string testSessionId,
    ITestExecutionFilter? filter,
    CancellationToken cancellationToken,
    bool isForExecution)          // ← after CancellationToken

.NET convention is to put CancellationToken last. Not a correctness issue, but may confuse tooling/analyzers.

HasListeners() guard is justified

The explicit guard around the session activity lookup is the right call — it avoids a potentially non-trivial call to GetContextProvider().TestSessionContext.Activity on every test run, and ensures the explicit parent context is only set when a listener is actually present. The comment in TUnitActivitySource.StartActivity correctly notes the method itself returns null when no listeners are sampling, but the guard here also prevents the call to GetContextProvider().

What's good

• Clean deduplication of the discovery logic — the before/after symmetry between DiscoverTests and DiscoverTestsFullyStreamingAsync is now obvious.
• Exception recording on the activity + SetStatus(Error) follows OTel conventions correctly.
• tunit.test.count set in finally means it's always populated even when an exception occurs mid-discovery (count reflects what was collected up to the failure point).
• Removal of the dead AreAllDependenciesSatisfied method is a nice cleanup.
• Docs accurately reflect the new span hierarchy and attribute table.

The double-registration question is the only one worth validating before merge; everything else is stylistic.

[claude]

Code Review

Overall this is a well-structured PR. The refactoring is clean, the span lifecycle is handled correctly, and the zero-cost guarantee when no listeners are attached is properly implemented.

Observations

Double-registration in the non-filter path (pre-existing, not a regression)

DiscoverTestsStreamAsync calls _dependencyResolver.RegisterTest(test) for each test as it yields. Then DiscoverAndResolveTestsAsync loops over the collected allTests and calls RegisterTest again (lines 165-168). This double-registration existed before this PR, so the refactoring preserves existing behaviour — worth confirming RegisterTest is idempotent (safe to call twice for the same test).

_ = ex; in the #else catch branch (minor)

The current approach works and suppresses the compiler warning. A cleaner alternative avoids the discard by using a parameterless catch in the non-NET branch:

#if NET
        catch (Exception ex)
        {
            TUnitActivitySource.RecordException(discoveryActivity, ex);
            throw;
        }
#else
        catch
        {
            throw;
        }
#endif

Parameter ordering (minor)

bool isForExecution appears after CancellationToken in DiscoverAndResolveTestsAsync. .NET convention puts CancellationToken last. Not a correctness issue but may confuse analyzers.

HasListeners() guard is justified

The explicit guard around the session activity lookup is the right call — it avoids a call to GetContextProvider().TestSessionContext.Activity on every test run when no tracing is configured. While TUnitActivitySource.StartActivity already returns null with no listeners, the guard here also skips the GetContextProvider() call entirely, which is a valid optimization.

What's good

• Clean deduplication of the discovery logic — the before/after symmetry between DiscoverTests and DiscoverTestsFullyStreamingAsync is now obvious.
• Exception recording on the activity + SetStatus(Error) follows OTel conventions correctly.
• tunit.test.count set in finally means it is always populated even when an exception occurs mid-discovery (count reflects what was collected up to the failure point).
• Removal of the dead AreAllDependenciesSatisfied method is a nice cleanup.
• Docs accurately reflect the new span hierarchy and the updated attribute table.

The double-registration question is the only one worth a quick check before merge; everything else is stylistic.

[thomhurst]

@copilot address Claude's review on this pr

[Copilot]

@copilot address Claude's review on this pr

Addressed all three items from Claude's review in 2f239c5:

1. Parameter ordering — moved CancellationToken to last position in DiscoverAndResolveTestsAsync per .NET convention
2. Catch pattern — replaced _ = ex; discard with #if/#else around the entire catch block, using parameterless catch in non-NET builds
3. Double-registration — moved RegisterTest into the filter path only, since DiscoverTestsStreamAsync already registers tests in the streaming path (fixes pre-existing double-registration bug)

[claude]

Follow-up Review

All three points from the previous review have been addressed in commit 2f239c51:

Addressed ✅

Parameter ordering — CancellationToken cancellationToken is now correctly last in DiscoverAndResolveTestsAsync.

Catch pattern — Uses #else catch { throw; } instead of _ = ex;. Clean and explicit.

Double-registration — The previous review said this was "preserved", but looking at the updated code it was actually fixed: in the streaming path, RegisterTest is now only called once (inside DiscoverTestsStreamAsync). The old DiscoverTests had a second redundant foreach (var test in allTests) { _dependencyResolver.RegisterTest(test); } pass after the streaming loop — that's gone now. Good catch.

Remaining observations (informational, not blocking)

• The #else catch { throw; } block is syntactically required here to pair with the #if NET catch (Exception ex) block. On non-NET targets this is a no-op rethrow which the JIT should eliminate, so the overhead is negligible — just noting this for clarity.
• sessionActivity?.Context ?? default correctly produces a root span when no session activity exists (i.e., no listener configured at session level). This is the right behaviour.

Overall

The PR is well-structured. The refactoring cleanly deduplicates discovery logic, the span lifecycle is handled correctly (starts after Before hooks so HasListeners() fires correctly, stops in finally so tunit.test.count is always set even on failure), and the zero-cost guarantee when no listeners are attached is intact. The docs update accurately reflects the new hierarchy.

Approved.

[thomhurst]

@copilot do we need to update any docs to instruct users where and how they can set up their own trace listeners?

[Copilot]

@copilot do we need to update any docs to instruct users where and how they can set up their own trace listeners?

Yes — the existing docs already had the OpenTelemetry SDK setup, but were missing some important pieces. Updated in 329668b:

1. Added raw ActivityListener example (Option B) for users who don't want the OpenTelemetry SDK dependency — just System.Diagnostics
2. Added "Why [Before(TestSession)]?" section explaining the hook execution order and why the listener must be registered before discovery to capture the new discovery span
3. Fixed exporter section formatting — the Jaeger/Zipkin/OTLP section previously mixed dotnet add package bash commands inside a C# code block, now properly separated into distinct blocks

[thomhurst]

@copilot shouldn't the docs say [Before(TestDiscovery)] for registering trace listeners?

[Copilot]

@copilot shouldn't the docs say [Before(TestDiscovery)] for registering trace listeners?

You're right — [Before(TestDiscovery)] is the correct hook since it runs right before the discovery span starts. Updated all examples and the timing explanation in fd4914b. Both [Before(TestDiscovery)] and [Before(TestSession)] work, but the docs now recommend [Before(TestDiscovery)] as the primary and note [Before(TestSession)] as an alternative.