feat: add TestContext.MakeCurrent() for console output correlation

[thomhurst]

Summary

• Adds TestContext.MakeCurrent() as the public API for correlating background-thread output (console, ILogger) to a specific test — returns a zero-allocation ContextScope (readonly struct) that restores the previous context on dispose
• Adds TestContext.GetById(string id) for looking up test contexts by their unique ID (used by middleware/handlers after extracting the test ID from a request)
• Updates TUnitTestContextMiddleware to call MakeCurrent() with scoped cleanup, so all output within a request is attributed to the correct test
• Simplifies CorrelatedTUnitLoggerProvider (no longer needs IHttpContextAccessor) and CorrelatedTUnitLogger (resolves via TestContext.Current only)

Design

MakeCurrent() sets the AsyncLocal directly, making it safe for any number of concurrent tests. An earlier iteration included a pluggable ITestContextResolver registry, but that mechanism is fundamentally unsafe under concurrent execution — with N tests registering resolvers simultaneously, first-non-null-wins resolution cannot determine which resolver belongs to which test. MakeCurrent() is simpler, safer, and sufficient for all use cases (gRPC, MCP, message queues, ASP.NET Core middleware, etc.).

Test plan

• 5 ASP.NET Core integration tests with shared WebApplicationFactory (PerTestSession) exercising both AsyncLocal-inherited path (TestServer) and MakeCurrent middleware path (simulated Kestrel via ExecutionContext.SuppressFlow)
• Public API snapshot tests updated and passing on all 4 target frameworks (net472, net8.0, net9.0, net10.0)
• Builds cleanly on all targets (netstandard2.0, net8.0, net9.0, net10.0)

Closes #5500

[claude]

Code Review:

This is a well-structured feature that solves a genuine problem (correlating console output on non-async-local threads). The design is clean and the test coverage is solid. A few issues worth addressing before merging.

---

Issue 1: Unhandled resolver exceptions will crash the hot path

File: TUnit.Core/TestContextResolverRegistry.cs, Resolve()

TestContextResolverRegistry.Resolve() is called on every Context.Current access, which happens on every Console.Write. If any registered resolver throws (e.g., due to a disposed IHttpContextAccessor), the exception propagates into the console interceptor and crashes the test unexpectedly.

Better approach — fail-safe per resolver:

foreach (var resolver in resolvers)
{
    try
    {
        var context = resolver.ResolveCurrentTestContext();
        if (context is not null)
            return context;
    }
    catch
    {
        // Resolver misbehaved; skip and continue to next resolver.
        // Don't let a bad resolver break console output for the whole test.
    }
}

Why: The resolver contract says "must be very cheap" but users will write resolvers that touch DI objects (like IHttpContextAccessor) that can be in an indeterminate state during teardown. Swallowing exceptions here is correct because the fallback (TestContext.Current) still works.

---

Issue 2: Resolver takes priority over ClassHookContext and TestBuildContext — silent semantic change

File: TUnit.Core/Context.cs

public static Context Current =>
    TestContextResolverRegistry.Resolve() as Context   // NEW: checked first
    ?? TestContext.Current as Context
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

A resolver that returns any non-null TestContext will prevent TestBuildContext.Current, ClassHookContext.Current, and AssemblyHookContext.Current from ever being returned by Context.Current. This is a silent, hard-to-debug regression for users who rely on Context.Current inside class/assembly hooks while a resolver is registered.

Better approach — insert after TestContext.Current, not before it:

public static Context Current =>
    TestContext.Current as Context                      // AsyncLocal first (cheapest, most specific)
    ?? TestContextResolverRegistry.Resolve() as Context // fallback for cross-thread scenarios
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

Why: The resolvers exist to handle the case where AsyncLocal is absent (e.g., on a thread-pool thread). When AsyncLocal is present, it should win — it carries the most specific, correct context. The current ordering makes the resolver override even valid AsyncLocal values, which contradicts the stated design goal of "fall through to the next resolver or the built-in AsyncLocal chain."

---

Issue 3: No guard against duplicate resolver registration

File: TUnit.Core/TestContextResolverRegistry.cs

Register allows the same resolver instance to be added multiple times. If CorrelatedTUnitLoggerProvider is created twice with the same IHttpContextAccessor, two HttpContextTestContextResolver instances are registered and each must be unregistered separately. Forgetting one causes a resolver leak.

Better approach:

public static void Register(ITestContextResolver resolver)
{
    ArgumentNullException.ThrowIfNull(resolver);
    lock (_lock)
    {
        if (Array.IndexOf(_resolvers, resolver) >= 0)
            return; // already registered
        // ... existing copy logic
    }
}

Why: Reference-equality idempotence prevents accidental double-registration (which multiplies work on the hot path) without complicating the unregister logic.

---

Minor: TestWebAppFactory workaround comment exposes a base class gap

File: TUnit.AspNetCore.Tests/TestWebAppFactory.cs

The comment acknowledges that TestWebApplicationFactory.CreateHostBuilder() isn't called for minimal-API apps, so AddCorrelatedTUnitLogging() must be re-added via ConfigureWebHost. This is a legitimate workaround, but it means every minimal-API test factory needs to know about this gotcha. Worth documenting in the base class XML docs or CLAUDE.md, or fixing in TestWebApplicationFactory directly so it doesn't rely on CreateHostBuilder for the registration.

---

What's good

• The volatile-array copy-on-write pattern for lock-free reads is exactly right for this hot path.
• CorrelatedTUnitLoggerProvider.Dispose() correctly unregisters the resolver — the lifecycle is properly managed.
• The [NotInParallel] guards on both test classes correctly isolate global-registry mutations.
• The ThreadStaticResolver cross-thread test is a great demonstration of the intended use case.
• The XML documentation is thorough and accurate.
• Extracting HttpContextTestContextResolver from CorrelatedTUnitLogger is the right separation of concerns.

The main actionable items are Issue 1 (exception safety) and Issue 2 (ordering semantics). Issue 3 is a nice-to-have.

[codacy-production]

Not up to standards ⛔

🔴 Issues 2 medium

Alerts:
⚠ 2 issues (≤ 0 issues of at least minor severity)

Results:
2 new issues

Category	Results
BestPractice	1 medium
Performance	1 medium

View in Codacy

🟢 Metrics 6 complexity

Metric	Results
Complexity	6

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review: feat/pluggable-test-context-resolver

This is a well-structured feature that solves a genuine problem (correlating console output on non-async-local threads). The design is clean and the test coverage is solid. A few issues worth addressing before merging.

---

Issue 1: Unhandled resolver exceptions will crash the hot path

File: TUnit.Core/TestContextResolverRegistry.cs, Resolve()

TestContextResolverRegistry.Resolve() is called on every Context.Current access, which happens on every Console.Write. If any registered resolver throws (e.g., due to a disposed IHttpContextAccessor), the exception propagates into the console interceptor and crashes the test unexpectedly.

Better approach — fail-safe per resolver:

foreach (var resolver in resolvers)
{
    try
    {
        var context = resolver.ResolveCurrentTestContext();
        if (context is not null)
            return context;
    }
    catch
    {
        // Resolver misbehaved; skip and continue to next resolver.
        // Don't let a bad resolver break console output for the whole test.
    }
}

Why: The resolver contract says "must be very cheap" but users will write resolvers that touch DI objects (like IHttpContextAccessor) that can be in an indeterminate state during teardown. Swallowing exceptions here is correct because the fallback (TestContext.Current) still works.

---

Issue 2: Resolver takes priority over ClassHookContext and TestBuildContext — silent semantic change

File: TUnit.Core/Context.cs

public static Context Current =>
    TestContextResolverRegistry.Resolve() as Context   // NEW: checked first
    ?? TestContext.Current as Context
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

A resolver that returns any non-null TestContext will prevent TestBuildContext.Current, ClassHookContext.Current, and AssemblyHookContext.Current from ever being returned by Context.Current. This is a silent, hard-to-debug regression for users who rely on Context.Current inside class/assembly hooks while a resolver is registered.

Better approach — insert after TestContext.Current, not before it:

public static Context Current =>
    TestContext.Current as Context                      // AsyncLocal first (cheapest, most specific)
    ?? TestContextResolverRegistry.Resolve() as Context // fallback for cross-thread scenarios
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

Why: The resolvers exist to handle the case where AsyncLocal is absent (e.g., on a thread-pool thread). When AsyncLocal is present, it should win — it carries the most specific, correct context. The current ordering makes the resolver override even valid AsyncLocal values, which contradicts the stated design goal of "fall through to the next resolver or the built-in AsyncLocal chain."

---

Issue 3: No guard against duplicate resolver registration

File: TUnit.Core/TestContextResolverRegistry.cs

Register allows the same resolver instance to be added multiple times. If CorrelatedTUnitLoggerProvider is created twice with the same IHttpContextAccessor, two HttpContextTestContextResolver instances are registered and each must be unregistered separately. Forgetting one causes a resolver leak.

Better approach:

public static void Register(ITestContextResolver resolver)
{
    ArgumentNullException.ThrowIfNull(resolver);
    lock (_lock)
    {
        if (Array.IndexOf(_resolvers, resolver) >= 0)
            return; // already registered
        // ... existing copy logic
    }
}

Why: Reference-equality idempotence prevents accidental double-registration (which multiplies work on the hot path) without complicating the unregister logic.

---

Minor: TestWebAppFactory workaround comment exposes a base class gap

File: TUnit.AspNetCore.Tests/TestWebAppFactory.cs

The comment acknowledges that TestWebApplicationFactory.CreateHostBuilder() is not called for minimal-API apps, so AddCorrelatedTUnitLogging() must be re-added via ConfigureWebHost. This is a legitimate workaround, but it means every minimal-API test factory needs to know about this gotcha. Worth documenting in the base class XML docs or fixing in TestWebApplicationFactory directly so the registration does not rely solely on CreateHostBuilder.

---

What's good

• The volatile-array copy-on-write pattern for lock-free reads is exactly right for this hot path.
• CorrelatedTUnitLoggerProvider.Dispose() correctly unregisters the resolver — the lifecycle is properly managed.
• The [NotInParallel] guards on both test classes correctly isolate global-registry mutations.
• The ThreadStaticResolver cross-thread test is a great demonstration of the intended use case.
• The XML documentation is thorough and accurate.
• Extracting HttpContextTestContextResolver from CorrelatedTUnitLogger is the right separation of concerns.

The main actionable items are Issue 1 (exception safety in hot path) and Issue 2 (resolver ordering semantics). Issue 3 is a nice-to-have.

[claude]

Code Review

This is a well-designed PR that adds a clean, pluggable context-resolution mechanism. The core design is solid: the volatile-array copy-on-write pattern in gives genuinely lock-free reads on the hot path, and extracting from is the right move for separation of concerns. The test coverage is thorough and the integration tests accurately model the real-world scenario they're guarding.

That said, there are several issues worth addressing:

---

1. Inconsistent resolution order between and

**** (AsyncLocal → Registry → rest of chain):

public static Context Current =>
    TestContext.Current as Context      // AsyncLocal first
    ?? TestContextResolverRegistry.Resolve() as Context   // registry second
    ?? TestBuildContext.Current as Context

**** (Registry → AsyncLocal):

return TestContextResolverRegistry.Resolve() ?? TestContext.Current;

These have opposite priorities. 's own XML doc says resolvers act as a fallback, consulted only when the AsyncLocal chain yields nothing — but checks the registry before . In the ASP.NET Core scenario this doesn't matter (the request thread won't have an AsyncLocal value), but it's an internal contradiction that could confuse anyone extending the system and will produce subtle ordering bugs if users ever hit both paths from the same thread.

Suggest making consistent with :

private static TestContext? ResolveTestContext()
    => TestContext.Current ?? TestContextResolverRegistry.Resolve();

---

2. Constructor side-effect on global state in

public CorrelatedTUnitLoggerProvider(IHttpContextAccessor httpContextAccessor, ...)
{
    _resolver = new HttpContextTestContextResolver(httpContextAccessor);
    TestContextResolverRegistry.Register(_resolver);   // <-- global mutation in ctor
}

Constructors with side effects on global static state are a well-known anti-pattern. The duplicate-instance guard in uses reference equality, so each — even if DI creates it more than once (e.g. in multi-factory test scenarios) — produces a fresh instance and registers it again. Those extra instances linger until each provider is d.

A cleaner alternative is to expose an explicit / hook that the host calls once, or to have register the resolver directly against the DI lifetime rather than through the provider's constructor. This keeps the global mutation visible and intentional, rather than a side-effect of object construction.

---

3. Silent exception swallowing with no diagnostics path

catch
{
    // Swallow exceptions from user-provided resolvers on the hot path.
}

The rationale is sound — faulty resolvers must not crash . However, a bare with no observability means resolver authors have no way to debug why their implementation isn't working. The fix doesn't need to be expensive: an -avoidance is already guaranteed by only swallowing non-fatal exceptions. Consider at minimum:

catch (Exception ex)
{
    // Best-effort: write to the true stdout so it doesn't recurse, but don't crash.
    // Users can attach a debugger or check for this prefix in output.
    System.Console.Error.WriteLine($"[TUnit] Resolver {resolver.GetType().Name} threw: {ex.Message}");
}

Or, if Debug-only tracing is acceptable, . Either way, completely silent failures make resolver implementations very difficult to develop and diagnose.

---

4. Registry consulted between test-phase contexts in

public static Context Current =>
    TestContext.Current as Context
    ?? TestContextResolverRegistry.Resolve() as Context  // returns TestContext, inserted here
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

The resolver returns , cast to . Inserting it between and means that during build/class/assembly hook phases, if a resolver returns a stale or incorrectly-scoped , it will shadow the hook context. Resolvers that don't carefully guard their state (e.g., a where the thread-static hasn't been cleared yet) will produce incorrect results in hook methods.

Consider placing resolver fallback after all the framework-provided contexts — only firing it when every built-in mechanism returns null:

public static Context Current =>
    TestContext.Current as Context
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context
    ?? GlobalContext.Current as Context
    ?? TestContextResolverRegistry.Resolve() as Context;  // genuine last resort

If the explicit goal is to allow resolvers to override hook contexts, that should be documented explicitly in 's remarks — right now the docs say nothing about this interaction.

---

5. workaround indicates a base-class gap

protected override void ConfigureWebHost(IWebHostBuilder builder)
{
    // For minimal API apps, CreateHostBuilder() returns null so the base class's
    // AddCorrelatedTUnitLogging() in CreateHostBuilder is never called.
    builder.ConfigureServices(services => services.AddCorrelatedTUnitLogging());
}

This is a workaround for the base class not calling for minimal-API-style programs. Rather than leaving this as an undiscovered trap for users, consider either:

• Making call via (which is called regardless of host style), or
• Adding a clear note to 's docs that users with minimal API programs must override .

Currently a user following the happy path with a minimal API will get no correlated logging and no error — just mysteriously missing output.

---

Minor: + in is confusing

<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="10.0.5" VersionOverride="8.0.0" />

Both attributes present is unusual. If this is intentional CPM usage (central version = 10.0.5, framework-specific override = 8.0.0) a comment saying so would prevent future maintainers from thinking it's a mistake and removing one of them.

---

Overall this is solid work — the pluggable resolver idea is well-motivated, the copy-on-write hot path is correctly implemented, and the test suite (especially the cross-thread correlation test) directly validates the core scenario. The issues above are worth addressing before merge, primarily the inconsistent resolution order (#1) and the constructor side-effect (#2), as those are the most likely to cause subtle bugs down the road.

[claude]

Code Review

This is a well-designed PR that adds a clean, pluggable context-resolution mechanism. The core design is solid: the volatile-array copy-on-write pattern in TestContextResolverRegistry gives genuinely lock-free reads on the hot path, and extracting HttpContextTestContextResolver from CorrelatedTUnitLogger is the right move for separation of concerns. The test coverage is thorough and the integration tests accurately model the real-world scenario they're guarding.

That said, there are several issues worth addressing:

---

1. Inconsistent resolution order between Context.Current and CorrelatedTUnitLogger

Context.cs (AsyncLocal first, Registry second):

public static Context Current =>
    TestContext.Current as Context      // AsyncLocal first
    ?? TestContextResolverRegistry.Resolve() as Context   // registry second
    ?? TestBuildContext.Current as Context

CorrelatedTUnitLogger.ResolveTestContext() (Registry first, AsyncLocal second):

return TestContextResolverRegistry.Resolve() ?? TestContext.Current;

These have opposite priorities. ITestContextResolver's own XML doc says resolvers act as a fallback, consulted only when the AsyncLocal chain yields nothing — but ResolveTestContext checks the registry before TestContext.Current. In the ASP.NET Core scenario this doesn't matter (the request thread won't have an AsyncLocal value), but it's an internal contradiction that could cause subtle ordering bugs if users hit both paths from the same thread.

Suggest making ResolveTestContext consistent with Context.Current:

private static TestContext? ResolveTestContext()
    => TestContext.Current ?? TestContextResolverRegistry.Resolve();

---

2. Constructor side-effect on global state in CorrelatedTUnitLoggerProvider

public CorrelatedTUnitLoggerProvider(IHttpContextAccessor httpContextAccessor, ...)
{
    _resolver = new HttpContextTestContextResolver(httpContextAccessor);
    TestContextResolverRegistry.Register(_resolver);   // <-- global mutation in ctor
}

Constructors with side effects on global static state are a well-known anti-pattern. The duplicate-instance guard in Register uses reference equality, so each new CorrelatedTUnitLoggerProvider(...) — even if DI creates multiple instances (e.g. in multi-factory test scenarios) — produces a fresh HttpContextTestContextResolver instance and registers it again. Those extra instances linger until each provider is Dispose()d.

A cleaner alternative is to have AddCorrelatedTUnitLogging register the resolver directly as a singleton tied to the DI lifetime, rather than burying global mutation inside the constructor. This makes the side-effect visible and intentional.

---

3. Silent exception swallowing with no diagnostics path

catch
{
    // Swallow exceptions from user-provided resolvers on the hot path.
}

The rationale is sound — faulty resolvers must not crash Console.Write. However, a bare catch {} with no observability means resolver authors have no way to debug why their implementation isn't working. Consider at minimum writing once to Console.Error or Debug.WriteLine to surface failures:

catch (Exception ex)
{
    // Best-effort: avoid infinite recursion by writing directly to stderr.
    System.Console.Error.WriteLine($"[TUnit] Resolver {resolver.GetType().Name} threw: {ex.Message}");
}

Completely silent failures make resolver implementations very difficult to develop and diagnose.

---

4. Registry position in Context.Current can shadow hook contexts

public static Context Current =>
    TestContext.Current as Context
    ?? TestContextResolverRegistry.Resolve() as Context  // inserted here
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

Inserting the registry between TestContext.Current and TestBuildContext.Current means that during build/class/assembly hook phases, if a resolver returns a stale or incorrectly-scoped TestContext (e.g. a ThreadStaticResolver whose thread-static hasn't been cleared yet), it will shadow the intended hook context.

Consider placing the resolver fallback after all framework-provided contexts, as a genuine last resort:

public static Context Current =>
    TestContext.Current as Context
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context
    ?? GlobalContext.Current as Context
    ?? TestContextResolverRegistry.Resolve() as Context;

If the intent is to allow resolvers to override hook contexts, that should be documented explicitly in ITestContextResolver's remarks — right now the docs say nothing about this interaction.

---

5. TestWebAppFactory workaround indicates a base-class gap

protected override void ConfigureWebHost(IWebHostBuilder builder)
{
    // For minimal API apps, CreateHostBuilder() returns null so the base class's
    // AddCorrelatedTUnitLogging() in CreateHostBuilder is never called.
    builder.ConfigureServices(services => services.AddCorrelatedTUnitLogging());
}

This workaround for minimal-API apps is a trap waiting to catch users. Consider making TestWebApplicationFactory call AddCorrelatedTUnitLogging from ConfigureWebHost instead (which is called regardless of host style), or add a clear note to the docs. Currently a user following the happy path with a minimal API will get no correlated logging and no error — just mysteriously missing output.

---

Minor: Version + VersionOverride in .csproj is confusing

<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="10.0.5" VersionOverride="8.0.0" />

Both attributes being present is unusual. If this is intentional CPM usage (central version = 10.0.5, framework-specific override = 8.0.0) a short comment would prevent future maintainers from thinking it's a typo.

---

Overall this is solid work. The pluggable resolver idea is well-motivated, the copy-on-write hot path is correctly implemented, and the test suite (especially the cross-thread correlation test) directly validates the core scenario. The most important issues before merge are the inconsistent resolution order (#1) and the constructor side-effect (#2).

[claude]

Code Review (follow-up)

Good progress since the previous reviews. Here's the status of previously raised issues and any new findings.

---

Previously raised issues — all addressed

• Exception safety in Resolve() — try/catch now wraps each resolver. Done.
• Resolver ordering in Context.cs — AsyncLocal (TestContext.Current) checked before the registry. Done.
• Duplicate registration guard — Array.IndexOf idempotency check added to Register(). Done.
• Inconsistent ordering between Context.cs and CorrelatedTUnitLogger.ResolveTestContext() — both now check AsyncLocal first. Done.
• [NotInParallel] removal — correctly removed only from integration tests (which are execution-context-scoped); kept on unit tests that mutate global registry state. Done.

---

Remaining issues from the previous round

1. Constructor side-effect on global state — still present

// CorrelatedTUnitLoggerProvider.cs
public CorrelatedTUnitLoggerProvider(IHttpContextAccessor httpContextAccessor, ...)
{
    _resolver = new HttpContextTestContextResolver(httpContextAccessor);
    TestContextResolverRegistry.Register(_resolver);   // global mutation in ctor
}

The duplicate-instance guard uses reference equality. Each new CorrelatedTUnitLoggerProvider(...) creates a fresh HttpContextTestContextResolver instance that bypasses the idempotency check — multiple providers register multiple resolvers. Those extra instances persist until Dispose() is called on each one. In multi-factory scenarios (e.g. two test classes each with their own TestWebApplicationFactory), this silently multiplies the resolver chain.

Better approach: Register in AddCorrelatedTUnitLogging as a singleton tied to DI lifetime, not in the provider constructor. This makes the global side-effect visible and intentional rather than buried in object construction.

2. Silent exception swallowing — still a bare catch {}

catch
{
    // Swallow exceptions from user-provided resolvers on the hot path.
    // A faulty resolver must not crash Console.Write/WriteLine.
}

The rationale for swallowing is correct. But a completely silent failure makes resolver implementations impossible to debug. A minimal fix avoids recursion by writing directly to stderr (bypassing the console interceptor):

catch (Exception ex)
{
    // Write directly to stderr to avoid re-entering the console interceptor.
    System.Console.Error.WriteLine($"[TUnit] {resolver.GetType().Name} threw during context resolution: {ex.Message}");
}

Or, if test output pollution is a concern, System.Diagnostics.Debug.WriteLine works without touching the console at all.

3. Registry position in Context.Current can still shadow hook contexts

public static Context Current =>
    TestContext.Current as Context
    ?? TestContextResolverRegistry.Resolve() as Context  // still here
    ?? TestBuildContext.Current as Context
    ?? ClassHookContext.Current as Context
    ?? AssemblyHookContext.Current as Context

A custom resolver that uses thread-static or activity-based storage (not just IHttpContextAccessor) may not clear state between test phases. If such a resolver returns a stale TestContext on a hook-phase thread, it shadows TestBuildContext.Current and ClassHookContext.Current, producing hard-to-diagnose incorrect context in hooks.

The HttpContextTestContextResolver is safe here (no HTTP context on hook threads), but the public ITestContextResolver interface has no such constraint. The docs currently don't mention this interaction.

At minimum, document this ordering explicitly in ITestContextResolver's remarks — resolver authors need to know their implementation can shadow hook contexts. Alternatively, move the registry to the end of the chain as the genuine last resort.

4. TestWebAppFactory workaround / base-class gap — still present

The comment in TestWebAppFactory.ConfigureWebHost still describes the minimal-API trap:

// For minimal API apps, CreateHostBuilder() returns null so the base class's
// AddCorrelatedTUnitLogging() in CreateHostBuilder is never called.

Users following the happy path with a minimal API app will get no correlated logging and no error message. This should either be fixed in TestWebApplicationFactory (register via ConfigureWebHost instead of CreateHostBuilder) or clearly documented as a known limitation in the class XML docs.

5. Version + VersionOverride without a comment — still present

<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="10.0.5" VersionOverride="8.0.0" />

Two version attributes without explanation looks like a typo. A short comment ("CPM central version, overridden per TFM") would prevent future maintainers from removing one and breaking the build.

---

Summary

The core mechanism is well-implemented and the most critical issues from earlier reviews have been fixed. The remaining items are:

#	Issue	Severity
1	Constructor side-effect may create multiple resolver instances in multi-factory scenarios	Medium
2	Bare catch {} with no diagnostic output	Low–Medium
3	Registry ordering can shadow hook contexts; undocumented	Low (with current resolvers) / Medium (for future resolver authors)
4	TestWebAppFactory minimal-API trap undocumented in base class	Low
5	Version/VersionOverride comment	Minor

Items 1 and 3 are worth addressing before merge; 2, 4, and 5 are lower priority but improve reliability and discoverability.

[claude]

Code Review: Follow-up

Good progress on this PR — all three issues raised in the first review have been addressed. Here's the status check and remaining items.

---

Previously Raised Issues — Status

#	Issue	Status
1	Unhandled resolver exceptions crash the hot path	✅ Fixed — try/catch added in Resolve()
2	Resolver checked before TestContext.Current (async-local) in Context.Current	✅ Fixed — TestContext.Current now comes first
3	No guard against duplicate resolver registration	✅ Fixed — Array.IndexOf idempotency check added

---

Remaining Issues from Previous Review (still outstanding)

1. Constructor side-effect creates multiple resolver instances in multi-factory scenarios — Medium

CorrelatedTUnitLoggerProvider registers _resolver in its constructor and unregisters in Dispose(). The idempotency guard checks reference equality, so each new provider instance creates a new HttpContextTestContextResolver and registers it as a separate entry.

In multi-factory scenarios (e.g. two test classes each with [ClassDataSource(Shared = SharedType.PerTestSession)] pointing to different TestWebApplicationFactory subtypes), N providers → N resolver instances in the global chain. Each extra resolver queries IHttpContextAccessor unnecessarily on every Console.Write.

Suggested fix: Register the resolver via the DI container as part of AddCorrelatedTUnitLogging() rather than in the provider constructor, so the side-effect is tied to DI lifetime and is explicit:

// In AddCorrelatedTUnitLogging():
services.AddSingleton<HttpContextTestContextResolver>();
services.AddHostedService<ResolverRegistrationService>(); // registers on start, unregisters on stop

Or at minimum, document the one-resolver-per-provider behavior in the XML docs.

2. Silent catch {} — no diagnostic output — Low–Medium

catch
{
    // Swallow exceptions from user-provided resolvers on the hot path.
    // A faulty resolver must not crash Console.Write/WriteLine.
}

The rationale is correct, but completely silent failures make resolver bugs invisible. A faulty resolver that always throws will just silently return null — the developer will see incorrect test output but have no indication why.

Suggested fix — write directly to stderr to avoid re-entering the console interceptor:

catch (Exception ex)
{
    // Write to stderr directly — avoids re-entering the TUnit console interceptor.
    System.Console.Error.WriteLine(
        $"[TUnit] Resolver {resolver.GetType().Name} threw during context resolution: {ex.Message}");
}

Or use System.Diagnostics.Debug.WriteLine if console pollution is a concern.

3. Registry ordering can shadow hook contexts; undocumented — Low (with current resolvers), Medium (for future authors)

The current Context.Current chain:

TestContext.Current as Context
?? TestContextResolverRegistry.Resolve() as Context   // ← sits between AsyncLocal and hook contexts
?? TestBuildContext.Current as Context
?? ClassHookContext.Current as Context
?? AssemblyHookContext.Current as Context

A custom resolver using thread-static or activity-based storage that doesn't clear state between test phases could return a stale TestContext on a hook-phase thread, silently shadowing TestBuildContext.Current and ClassHookContext.Current. The HttpContextTestContextResolver is safe (no HTTP context on hook threads), but the public ITestContextResolver interface docs don't mention this.

Suggested fix: Add a note to the ITestContextResolver XML docs:

Important: If your resolver returns a non-null value on threads that execute class- or assembly-level hooks, it will shadow ClassHookContext.Current and AssemblyHookContext.Current. Ensure your resolver returns null when no HTTP/protocol request is in-flight.

4. TestWebAppFactory minimal-API workaround still unexplained in base class — Low

TestWebAppFactory.cs carries a comment:

// For minimal API apps, CreateHostBuilder() returns null so the base class's
// AddCorrelatedTUnitLogging() in CreateHostBuilder is never called.

Users following the standard TestWebApplicationFactory<TProgram> docs won't know they need this override. The base class TestWebApplicationFactory should either register via ConfigureWebHost (which is called for both patterns) or its XML docs should warn about the minimal-API gap. As-is, users will write minimal API apps, skip the workaround, and get silently incorrect log routing.

5. Version/VersionOverride without explanation — Minor

<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="10.0.5" VersionOverride="8.0.0" />

Two version attributes with different values looks like a copy-paste mistake to any maintainer unfamiliar with CPM. A one-line comment ("central version from Directory.Packages.props, overridden per TFM") would prevent accidental removal.

---

New Observation

The CorrelatedTUnitLogger.ResolveTestContext() comment in the XML docs of CorrelatedTUnitLogger.cs now says:

"Resolution checks TestContext.Current (AsyncLocal) first, then falls back to the TestContextResolverRegistry"

But the actual code does the opposite — ResolveTestContext() calls TestContext.Current ?? TestContextResolverRegistry.Resolve(), which is correct. The docs are accurate. No change needed — just confirming the code and docs are consistent.

---

Summary

The core mechanism is solid and the critical issues from the first review have all been fixed. Items 1 and 3 above are worth addressing before merge to prevent multi-factory leaks and undocumented sharp edges for future resolver authors. Items 2, 4, and 5 are lower priority but improve debuggability and discoverability.

[claude]

Code Review: Final Pass

All issues raised across both previous reviews have been addressed in the latest commit (). Here's the status summary and my overall assessment.

---

Previously Raised Issues — Final Status

#	Issue	Status
1	Unhandled resolver exceptions crash the hot path	✅ Fixed — try/catch with re-entrancy explanation
2	Resolver checked before AsyncLocal in Context.Current	✅ Fixed — AsyncLocal comes first
3	No guard against duplicate resolver registration	✅ Fixed — idempotency via Array.IndexOf
4	One-resolver-per-provider behavior undocumented	✅ Addressed — XML docs on CorrelatedTUnitLoggerProvider explain multi-factory behavior
5	Silent catch {} gives no diagnostic signal	✅ Addressed — comment explains the re-entrancy constraint (logging from inside Console.Write would recurse back into the interceptor, so stderr is not safe either)
6	Resolver ordering shadows hook contexts; undocumented	✅ Addressed — ITestContextResolver XML docs now explicitly warn about shadowing ClassHookContext / AssemblyHookContext
7	Minimal-API CreateHostBuilder gap undocumented	✅ Addressed — TestWebAppFactory XML docs explain the pattern and the workaround
8	VersionOverride looks like a copy-paste error	✅ Addressed — comment explains CPM intent

---

Overall Assessment

The implementation is solid. The design choices are well-justified:

• Lock-free hot path: volatile array read + length check = minimal overhead when no resolvers registered. The copy-on-write pattern under lock is the right approach for a read-heavy, write-rare registry.
• Exception swallowing: Correct given the re-entrancy constraint. The comment now explains why, which is the important part.
• Registration lifecycle: Constructor-time registration + Dispose()-time unregistration is simple and correct. The documented multi-factory behavior (one resolver per factory) is actually a feature, not a bug — each resolver correctly queries its own IHttpContextAccessor.
• Test coverage: The ExecutionContext.SuppressFlow() technique to simulate real Kestrel thread isolation is a clever and accurate way to test the resolver fallback path without spin-up overhead.

---

One Minor Observation

In TestContextResolverRegistry.Register:

if (resolver is null)
{
    throw new ArgumentNullException(nameof(resolver));
}

The project targets .NET 8+ (and LangVersion is preview), so ArgumentNullException.ThrowIfNull(resolver) is available and more idiomatic. Same in Unregister. This is a trivial nit — not blocking.

---

This PR is ready to merge. The feature is well-scoped, the implementation is correct, and the documentation is thorough. Good work iterating on the review feedback.

[claude]

Code Review: Final Pass

All issues raised across both previous reviews have been addressed in the latest commit. Here's the status summary and my overall assessment.

---

Previously Raised Issues — Final Status

#	Issue	Status
1	Unhandled resolver exceptions crash the hot path	Fixed - try/catch with re-entrancy explanation
2	Resolver checked before AsyncLocal in Context.Current	Fixed - AsyncLocal comes first
3	No guard against duplicate resolver registration	Fixed - idempotency via Array.IndexOf
4	One-resolver-per-provider behavior undocumented	Addressed - XML docs on CorrelatedTUnitLoggerProvider explain multi-factory behavior
5	Silent catch {} gives no diagnostic signal	Addressed - comment explains the re-entrancy constraint (logging from inside Console.Write would recurse back into the interceptor, so stderr is not safe either)
6	Resolver ordering shadows hook contexts; undocumented	Addressed - ITestContextResolver XML docs now explicitly warn about shadowing ClassHookContext / AssemblyHookContext
7	Minimal-API CreateHostBuilder gap undocumented	Addressed - TestWebAppFactory XML docs explain the pattern and the workaround
8	VersionOverride looks like a copy-paste error	Addressed - comment explains CPM intent

---

Overall Assessment

The implementation is solid. The design choices are well-justified:

• Lock-free hot path: volatile array read + length check = minimal overhead when no resolvers are registered. The copy-on-write pattern under lock is the right approach for a read-heavy, write-rare registry.
• Exception swallowing: Correct given the re-entrancy constraint. The comment now explains why, which is the important part.
• Registration lifecycle: Constructor-time registration + Dispose()-time unregistration is simple and correct. The documented multi-factory behavior (one resolver per factory) is a feature, not a bug -- each resolver correctly queries its own IHttpContextAccessor.
• Test coverage: The ExecutionContext.SuppressFlow() technique to simulate real Kestrel thread isolation is a clever and accurate way to test the resolver fallback path without needing a full Kestrel spin-up.

---

One Minor Observation

In TestContextResolverRegistry.Register and Unregister:

if (resolver is null)
{
    throw new ArgumentNullException(nameof(resolver));
}

The project targets .NET 8+ with LangVersion preview, so ArgumentNullException.ThrowIfNull(resolver) is available and more idiomatic. This is a trivial nit -- not blocking.

---

This PR is ready to merge. The feature is well-scoped, the implementation is correct, and the documentation is thorough. Good work iterating on the review feedback.

[claude]

Code Review: Follow-up

Status of Previously Raised Issues

All issues from the 23:17 review have been addressed in the final commit (34509ee):

Issue 1 — Hardcoded `LogLevel.Information`: Fixed. `_minLogLevel` is now correctly stored and threaded through `CorrelatedTUnitLoggerProvider` → `CorrelatedTUnitLogger` → `IsEnabled()`. The `AddCorrelatedTUnitLogging(LogLevel minLogLevel = LogLevel.Information)` parameter is preserved. No silent capability regression.

Issue 2 — `BeginScope` returning `null`: Fixed. `BeginScope` now returns `TUnitLoggerScope.Push(state)`, restoring parity with the original implementation. Scope state from ASP.NET Core middleware (request IDs, correlation IDs) will flow correctly.

---

One Remaining Actionable Issue

New integration test projects are not in `TUnit.CI.slnx` — the CI build won't compile them

The CI workflow (dotnet.yml) runs:

1. dotnet build TUnit.CI.slnx -c Release — compiles everything in the CI solution
2. The pipeline module (RunAspNetCoreTestsModule) runs the tests with NoBuild = true

TUnit.AspNetCore.Tests and TUnit.AspNetCore.Tests.WebApp are in TUnit.slnx (the IDE solution) but not in TUnit.CI.slnx. Because the pipeline uses NoBuild = true, the test run will fail with a "project has not been built" error when CI hits RunAspNetCoreTestsModule.

This was flagged in the 22:36 review and is still not addressed in the current state.

Fix: Add both projects to TUnit.CI.slnx:

<Project Path="TUnit.AspNetCore.Tests/TUnit.AspNetCore.Tests.csproj" />
<Project Path="TUnit.AspNetCore.Tests.WebApp/TUnit.AspNetCore.Tests.WebApp.csproj" />

---

Minor Note

RunAspNetCoreTestsModule.TestableFrameworks overrides to net8.0 and net10.0 only, but the .csproj targets net8.0;net9.0;net10.0. The net9.0 build is compiled (if the slnx gap above is fixed) but never executed in CI. This is consistent with the rest of the codebase's pipeline pattern (the base TestBaseModule also omits net9.0), so it's not a new issue introduced here — just worth noting for awareness.

---

Overall Assessment

The core design is clean and correct. MakeCurrent() is the right API — simpler and safer than the pluggable registry it replaced. The `ContextScope` struct is zero-allocation, the middleware integration is straightforward, and the test coverage accurately exercises both the inherited-AsyncLocal path and the suppressed-flow / middleware path. The `TUnit.CI.slnx` gap is the one remaining blocker before this is fully wired into CI.

[claude]

Code Review: feat/add-TestContext.MakeCurrent

All issues from the previous review have been fully addressed — the entire ITestContextResolver registry (the root cause of the exception-in-hot-path, resolver-priority, memory-leak, and thread-safety issues) has been removed entirely. The pivot to MakeCurrent() is the right architectural call, well explained in the PR description.

The implementation is clean. A few observations:

---

Issue 1: _testContextsById is never cleared (potential memory growth)

File: TUnit.Core/TestContext.cs lines 34, 49

private static readonly ConcurrentDictionary<string, TestContext> _testContextsById = new();
// ...
_testContextsById[Id] = this;  // added on every TestContext construction, never removed

Test contexts are added on construction but never removed. For a test suite with thousands of parameterized tests (or one with retries), this holds TestContext objects — including their StringWriter output buffers, CancellationToken, and all metadata — in memory for the entire process lifetime.

Better approach: Remove completed test contexts from the dictionary after they can no longer be looked up by middleware. The OnDispose lifecycle event or the engine's existing CleanupTestAsync path (ObjectLifecycleService.cs:551) would be natural places:

// In the engine cleanup path, after test disposal:
_testContextsById.TryRemove(testContext.Id, out _);

Why this matters: GetById() is used to find a running test. Once a test has finished, any middleware receiving a stale ID will just get a null (which is the right safe-failure behavior). There is no value in keeping the entry after the test ends, and removing it prevents unbounded dictionary growth in long-running sessions.

---

Issue 2: RunAspNetCoreTestsModule skips net9.0

File: TUnit.Pipeline/Modules/RunAspNetCoreTestsModule.cs lines 549–555

protected override IEnumerable<string> TestableFrameworks
{
    get
    {
        yield return "net10.0";
        yield return "net8.0";
        // net9.0 missing
    }
}

The test project targets net8.0;net9.0;net10.0 but CI only runs two of the three. A net9.0-specific regression in AsyncLocal or middleware behavior would be invisible. Unless there's a deliberate cost/time tradeoff, adding net9.0 here is low effort and completes the coverage.

---

Minor: Redundant return; in TUnitTestContextMiddleware

File: TUnit.AspNetCore.Core/Logging/TUnitTestContextMiddleware.cs line 36

using (testContext.MakeCurrent())
{
    await _next(httpContext);
}

return;  // prevents the duplicate _next call below

The return; is technically necessary to prevent the fallback await _next(httpContext) from executing again, but the two-call structure takes a moment to parse. A minor readability improvement would be a comment on the return; line (e.g., // context found; _next already called above), or restructuring the method to have a single _next call. The current code is correct as-is — this is a style note only.

---

What's good

• Architectural simplification is the right call. The PR description nails why ITestContextResolver was fundamentally broken for concurrent tests: first-non-null-wins resolution under N concurrent tests is non-deterministic. MakeCurrent() is deterministic, zero-allocation (readonly struct scope), and composable.
• ContextScope as a readonly struct avoids heap allocation for the common case (using var scope = ctx.MakeCurrent()). The Dispose() restoring ClassHookContext.Current transitively (via TestContext.Current's setter) is correct.
• Test coverage is excellent: both AsyncLocal-inherited and suppressed-flow paths are tested; the ExecutionContext.SuppressFlow() technique in SendWithSuppressedFlow accurately simulates real Kestrel behavior.
• Removing IHttpContextAccessor dependency from the logger makes CorrelatedTUnitLoggerProvider trivially constructable — services.AddSingleton(new CorrelatedTUnitLoggerProvider(...)) is cleaner than the previous factory lambda.
• Documentation in logging.md with protocol-specific examples (gRPC, message queue) is thorough and immediately usable.

---

Verdict: Approve pending the memory-growth concern in Issue 1, which is the only non-trivial item. Issue 2 is a CI coverage gap worth addressing; the rest are minor.

[claude]

Code Review: Follow-up

Status of Previously Raised Issues

From the last review (the 17th review in this cycle), two items were flagged:

Issue 1 — _testContextsById is never cleared (memory growth): ❌ Still not addressed.

The dictionary at TUnit.Core/TestContext.cs:34 still adds entries on construction and never removes them:

private static readonly ConcurrentDictionary<string, TestContext> _testContextsById = new();
// ...
_testContextsById[Id] = this;  // line 49 — added, never removed

GetById() (line 188) is intended for finding running tests. Once a test completes, keeping its TestContext alive (with its StringWriter output buffer, metadata, and cancel token) serves no purpose and prevents GC. For suites with thousands of parameterized tests, or runs with retries, this is unbounded growth.

Recommended fix — remove from the dictionary in the engine's existing cleanup path. TestContextRestorer or ObjectLifecycleService's cleanup are natural locations:

// After test disposal, e.g. in ObjectLifecycleService CleanupTestAsync:
_testContextsById.TryRemove(testContext.Id, out _);

Alternatively, expose an internal static void Unregister(string id) on TestContext and call it from the engine — this keeps the registration/unregistration symmetric and co-located.

Issue 2 — RunAspNetCoreTestsModule skips net9.0: ❌ Still not addressed.

yield return "net10.0";
yield return "net8.0";
// net9.0 still missing

The test project targets net8.0;net9.0;net10.0 but CI only exercises two of the three. A net9.0-specific regression would be invisible. Adding yield return "net9.0"; is one line.

Issue 3 — Redundant return; in TUnitTestContextMiddleware: This was flagged as a minor style note in the last review. The current code is correct; no objection to leaving it as-is.

---

New Observations

The core design change is right. This PR is a clean pivot: dropping the ITestContextResolver registry entirely removes the exception-in-hot-path, resolver-priority, and thread-safety issues all at once. MakeCurrent() is deterministic, zero-allocation, and composable across every protocol (gRPC, message queue, MCP, ASP.NET Core).

ContextScope as a readonly struct is correct for the zero-allocation goal. The "do not dispose more than once" warning in the doc comment is the right nudge — double-dispose would silently restore a stale context. This is an acceptable tradeoff for a value type in a hot path; the warning is sufficient.

Removal of IHttpContextAccessor from CorrelatedTUnitLoggerProvider / CorrelatedTUnitLogger is a genuine simplification — the provider is now trivially constructable without a DI container.

Documentation in logging.md is thorough and immediately actionable — the gRPC interceptor and message queue consumer examples correctly show the full pattern.

---

Verdict: Two items from the previous review remain open. Issue 1 (memory growth) is the only non-trivial one and should be addressed before merging. Issue 2 (missing net9.0 in CI) is low-effort and worth fixing. Everything else looks good.