fix: parent data source initialization spans under session span

[thomhurst]

Problem

When IAsyncInitializer.InitializeAsync runs for ClassDataSource objects, the test case Activity span had already been started in TestExecutor. This meant every span produced during initialization — container startup, HTTP health checks, auth calls, connection pool warm-up — appeared nested inside the individual test's trace timeline in the HTML report.

This was especially visible when running a single test in isolation, where all shared fixture setup costs show up in that test's trace, making the timeline noisy and misleading.

Fix

Move the Activity.Current manipulation into TestInitializer.InitializeTestObjectsAsync, where it belongs. Before calling into ObjectLifecycleService, we briefly set Activity.Current to the session span so initialization spans are parented there instead. TestExecutor is unchanged in its external behaviour.

TestInitializer is the right owner of this concern — TestExecutor is an orchestrator and shouldn't need to know about activity context details.

Also included

Adds an [Explicit] CloudShop example test (CheckoutTraceDemoTests) that demonstrates the HTML report's trace timeline. The test exercises PostgreSQL (direct query), Redis (cache check), and two HTTP endpoints, with a deliberately failing assertion and an error recorded on the failing span — showing exactly how the trace timeline surfaces what happened and where things went wrong.

Test plan

• Existing tests pass
• Run CheckoutTraceDemoTests explicitly and verify initialization spans (container startup, auth calls) appear under the session span in the HTML report, not inside the test case's trace timeline
• Verify the test case trace shows only the test's own spans: db.read_product_stock, api.get_product, cache.check_product, api.create_order

[claude]

Code Review

Summary

The core fix is correct and well-motivated. Moving InitializeTestObjectsAsync before the test-case Activity span starts in TestExecutor.cs is the right architectural move — the executor should not have been responsible for ensuring the right tracing context was active during initialization.

---

TUnit.Engine/TestExecutor.cs

Clean improvement. The 4-line move is the minimal, correct fix for the parenting problem. TestExecutor is correctly left as a pure orchestrator with no knowledge of activity-context details.

---

TUnit.Engine/TestInitializer.cs

The two-layered approach — (1) call before test-case span, (2) also explicitly pin Activity.Current to the session span — is belt-and-suspenders, which is a defensible choice. However, it introduces a subtle concern worth calling out.

Concern: Activity.Current restoration after await in async contexts

Activity.Current is backed by AsyncLocal<Activity?>, so its value flows through ExecutionContext. Setting it before an await and restoring it in finally is generally correct, but there is a nuance:

var previousActivity = Activity.Current;
if (sessionActivity is not null)
{
    Activity.Current = sessionActivity;
}
try
{
    await _objectLifecycleService.InitializeTestObjectsAsync(test.Context, cancellationToken);
}
finally
{
    Activity.Current = previousActivity; // ← restores in current async scope
}

At the point RestoreExecutionContext() is called for the class context (in TestExecutor) just before InitializeTestObjectsAsync, Activity.Current is already the class-level activity — not the session activity. After this patch, TestInitializer explicitly walks up to the session activity. That's intentional, but means that if the InitializeTestObjectsAsync ever spawns fire-and-forget child tasks (even inadvertently), those would inherit the session activity and then not be cleaned up when finally runs on the parent. This is a pre-existing risk in the fire-and-forget pattern, not introduced by this PR, but it's worth a comment in the code acknowledging this assumption.

Simplification opportunity: the if (sessionActivity is not null) guard is good, but the try/finally still runs even when nothing was changed. Consider extracting to a guard-return pattern to make the intent clearer:

// If there's no session activity to parent under, just run normally.
if (sessionActivity is null)
{
    await _objectLifecycleService.InitializeTestObjectsAsync(test.Context, cancellationToken);
    return;
}

var previousActivity = Activity.Current;
Activity.Current = sessionActivity;
try
{
    await _objectLifecycleService.InitializeTestObjectsAsync(test.Context, cancellationToken);
}
finally
{
    Activity.Current = previousActivity;
}

This removes the path where previousActivity is needlessly restored to itself and makes the conditional logic more explicit.

---

examples/CloudShop/CloudShop.Tests/Tests/Traces/CheckoutTraceDemoTests.cs

The demo is well-written, with good XML docs and clear step-by-step comments. The [Explicit] attribute is the right call for a deliberately-failing showcase test.

One issue: Admin is injected but never used.

[ClassDataSource<AdminApiClient>(Shared = SharedType.PerTestSession)]
public required AdminApiClient Admin { get; init; }

AdminApiClient is never referenced in Checkout_FailsWhenRequestedQuantityExceedsStock. This causes unnecessary initialization of the admin fixture (including whatever auth/HTTP setup that entails) for every run of this test. Either remove the property, or add a scenario that actually exercises it.

Minor: the first DB query has no span

The initial product lookup (finding a product ID) runs outside any using (var span = ...) block, which is inconsistent with how the rest of the test is structured. This could be confusing in the HTML trace timeline because the query runs but produces no visible span. Either wrap it in a span (e.g., "db.find_product") or add a brief comment explaining why it's intentionally untraced.

---

Overall

The fix is correct, focused, and the architectural reasoning in the PR description is sound — TestInitializer is the right owner of activity-context concerns during initialization. The two issues above (unused Admin fixture, nullable guard simplification) are worth addressing before merge but are not blockers. The core behavioral change is solid.

[claude]

Code Review

Summary

The fix is well-targeted and conceptually sound. Moving before in , combined with the explicit redirect in , correctly ensures that data source initialization spans (container startup, connection warmup, auth calls) are parented under the session span rather than the individual test case span.

---

Correctness: Manipulation

The borrow pattern in is correct:

var previousActivity = Activity.Current;
if (sessionActivity is not null)
    Activity.Current = sessionActivity;
try   { await ...; }
finally { Activity.Current = previousActivity; }

Activity.Current is backed by AsyncLocal<Activity?>, so:

• Parallel tests running concurrently are unaffected (each has its own async-local copy).
• The try/finally properly restores the value in the continuation after await, even if InitializeTestObjectsAsync internally awaits in parallel branches.

One observation: the guard if (sessionActivity is not null) means when there is no session span, Activity.Current is not changed but is still "restored" in finally. That's a harmless no-op and is intentional — leaving the existing activity in place is the correct fallback.

---

Issue: Unused Data Source in Demo Test

In CheckoutTraceDemoTests.cs:

[ClassDataSource<AdminApiClient>(Shared = SharedType.PerTestSession)]
public required AdminApiClient Admin { get; init; }

Admin is injected but never used anywhere in the test. This will:

1. Cause AdminApiClient (and any underlying infrastructure it wraps — HTTP client, auth flows, etc.) to be initialized as a shared data source, adding initialization overhead.
2. Produce initialization spans that will appear in the session timeline, potentially making the trace demo noisier than intended.
3. Mislead future readers about why it's there.

Recommendation: Remove the Admin property unless it's needed for a future test in the same class. If it's intentional scaffolding, add a comment explaining why.

---

Observation: #else Branch is Dead Code for Current Targets

All current TUnit targets (net8.0, net9.0, net10.0) define NET, so the #else path in TestInitializer.cs will never compile into any output artifact:

#else
        await _objectLifecycleService.InitializeTestObjectsAsync(test.Context, cancellationToken);
#endif

This is consistent with existing patterns throughout TestExecutor.cs and the rest of the engine, so it's not a new concern — just worth noting it's defensive code for hypothetical future targets.

---

Responsibility Assignment is Correct

Moving this concern from TestExecutor to TestInitializer is the right call. TestExecutor is an orchestrator; it shouldn't need to know about tracing context. TestInitializer owns the initialization lifecycle and is the natural home for "set context before initializing, restore after."

---

Overall

The core fix is clean and correct. The main actionable issue is the unused AdminApiClient dependency in the demo test. Everything else is in good shape.

[claude]

Code Review

Summary

The fix is well-targeted and conceptually sound. Moving InitializeTestObjectsAsync before StartActivity("test case") in TestExecutor.cs, combined with the explicit Activity.Current redirect in TestInitializer.cs, correctly ensures that data source initialization spans (container startup, connection warmup, auth calls) are parented under the session span rather than the individual test case span.

---

Correctness: Activity.Current Manipulation

The borrow pattern in TestInitializer.cs is correct:

var previousActivity = Activity.Current;
if (sessionActivity is not null)
    Activity.Current = sessionActivity;
try   { await ...; }
finally { Activity.Current = previousActivity; }

Activity.Current is backed by AsyncLocal<Activity?>, so parallel tests running concurrently are unaffected (each has its own async-local copy), and the try/finally properly restores the value in the continuation after await.

One observation: the guard if (sessionActivity is not null) means when there is no session span, Activity.Current is unchanged but still "restored" in finally. That is a harmless no-op and the correct fallback.

---

Issue: Unused Admin Data Source in Demo Test

In CheckoutTraceDemoTests.cs:

[ClassDataSource<AdminApiClient>(Shared = SharedType.PerTestSession)]
public required AdminApiClient Admin { get; init; }

Admin is injected but never used anywhere in the test. This causes:

1. AdminApiClient (and any underlying infrastructure it wraps) to be initialized as a shared data source, adding unnecessary overhead.
2. Initialization spans appearing in the session timeline, potentially making the trace demo noisier — the opposite of what the demo is trying to illustrate.
3. Confusion for future readers about why it is there.

Recommendation: Remove the Admin property unless it is needed for a future test in the same class. If it is intentional scaffolding, add a comment explaining why.

---

Observation: #else Branch is Dead Code for Current Targets

All current TUnit targets (net8.0, net9.0, net10.0) define NET, so the #else path in TestInitializer.cs will never compile. This is consistent with existing patterns in TestExecutor.cs and the rest of the engine, so it is not a new concern — just defensive code for hypothetical future targets.

---

Responsibility Assignment is Correct

Moving this concern from TestExecutor to TestInitializer is the right call. TestExecutor is an orchestrator and should not need to know about tracing context details. TestInitializer owns the initialization lifecycle and is the natural home for "set context before initializing, restore after."

---

Overall

The core fix is clean and correct. The main actionable item is the unused AdminApiClient in the demo test, which undermines the very thing the demo is supposed to illustrate. Everything else looks good.