Skip to content

fix: auto-register correlated logging for minimal API hosts (#5503)#5511

Merged
thomhurst merged 1 commit intomainfrom
fix/5503-minimal-api-correlated-logging
Apr 11, 2026
Merged

fix: auto-register correlated logging for minimal API hosts (#5503)#5511
thomhurst merged 1 commit intomainfrom
fix/5503-minimal-api-correlated-logging

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Fixes TestWebApplicationFactory: correlated logging not auto-registered for minimal API apps #5503: TestWebApplicationFactory<T> was registering AddCorrelatedTUnitLogging() inside CreateHostBuilder(), which returns null for minimal API hosts (WebApplication.CreateBuilder / top-level statements). The null-propagating hostBuilder?.ConfigureServices(...) silently skipped registration, leaving server-side ILogger output uncorrelated to the owning TestContext.
  • Move the registration to a ConfigureWebHost(IWebHostBuilder) override. WebApplicationFactory<T> invokes this hook for both IHostBuilder-based and minimal API hosts, so correlated logging is now wired up for both hosting models with no subclass cooperation required.
  • Remove the pre-existing workaround in TUnit.AspNetCore.Tests/TestWebAppFactory.cs that manually registered logging via ConfigureWebHost.
  • Add a new dedicated regression target TUnit.AspNetCore.Tests.MinimalApi (minimal API webapp with namespaced Program so it can coexist with the existing TUnit.AspNetCore.Tests.WebApp's global Program in the same test assembly) plus MinimalApiAutoRegistrationTests with a zero-override MinimalApiTestFactory — the existence of the no-override factory is the regression test.

Test plan

  • dotnet build clean across TUnit.AspNetCore.Core, TUnit.AspNetCore.Tests.MinimalApi, and TUnit.AspNetCore.Tests
  • TUnit.AspNetCore.Tests 6/6 passing on net10.0 (5 existing CorrelatedLoggingResolverTests + 1 new ServerLog_AutoCorrelated_OnMinimalApiHost_WithoutSubclassConfig)
  • Negative-test verification: temporarily reverted the base-class fix (git stash), reran the new test, confirmed it fails with the marker missing from TestContext.Current.GetStandardOutput(). Restored the fix and the test passes again — proves the test catches the exact bug
  • CI green on net8.0, net9.0, net10.0

Notes for reviewers

  • Subclasses overriding ConfigureWebHost now must call base.ConfigureWebHost(builder) to inherit logging registration. This is documented in the XML doc on the override. The new no-override regression test catches accidental drift in the base behavior.
  • The existing CorrelatedLoggingResolverTests (which use TestWebAppFactory against the existing minimal API TUnit.AspNetCore.Tests.WebApp) would have started failing once the workaround in TestWebAppFactory was removed if the base class fix were absent, so they also serve as additional regression coverage.

TestWebApplicationFactory<T> registered AddCorrelatedTUnitLogging() inside
CreateHostBuilder(), which returns null for minimal API apps using
WebApplication.CreateBuilder — silently skipping registration via the
null-propagating ConfigureServices call. Move the registration to a
ConfigureWebHost(IWebHostBuilder) override, which is invoked by
WebApplicationFactory<T> for both IHostBuilder-based and minimal API hosts.

Add TUnit.AspNetCore.Tests.MinimalApi as a clean regression target with
zero-override factory and test that asserts correlated logging is wired
up by the base class alone. Verified by reverting the fix and confirming
the new test fails.

Removes a workaround in the existing TestWebAppFactory that registered
logging via ConfigureWebHost manually.
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The fix is correct and well-motivated. Moving from to is exactly the right layer — guarantees is called for all host models, so this closes the silent-skip path for minimal API apps.

The regression test design is particularly clean: having zero overrides is self-documenting evidence that the base class is doing the work. Well done there.


Issue 1: Potential double-registration for callers who applied the workaround themselves

Any user who read the old XML doc and applied the workaround (calling AddCorrelatedTUnitLogging() in their own ConfigureWebHost override without calling base) will now get the service registered twice. Both:

  • IStartupFilter → TUnitTestContextStartupFilter (causes TUnitTestContextMiddleware to run twice in the pipeline)
  • ILoggerProvider → CorrelatedTUnitLoggerProvider (duplicates every log write to stdout)

ASP.NET Core allows multiple registrations of interface-typed singletons, so neither will be rejected — they'll just silently double-up. The first is worse because middleware duplication can cause subtle context-overwrite bugs under concurrency.

Suggested improvement: Guard against double-registration in AddCorrelatedTUnitLogging:

This makes the extension idempotent (like AddLogging() itself) and removes the migration burden from users.


Issue 2: Silent behavioral change for subclasses overriding ConfigureWebHost without calling base

Before this PR, for IHostBuilder-based apps, logging was registered in CreateHostBuilder() — independent of whether the subclass overrode ConfigureWebHost. Now the registration depends entirely on base.ConfigureWebHost(builder) being called.

Any existing subclass like:

…will silently lose correlated logging on all host types (not just minimal API). The XML doc warns about it, but this is a semantic change to a protected override that previously had no base to call.

Suggested improvement: Since this affects the public API surface, consider adding a note in release notes / changelog and/or bumping the minor version if SemVer is observed. Alternatively, extract the registration into a separate non-overridable helper and call it from both ConfigureWebHost and ConfigureHostBuilder to provide belt-and-suspenders coverage — though that adds complexity.


Minor observation: ConfigureStartupConfiguration is still skipped for minimal API hosts in the shared (non-isolated) path

In CreateHostBuilder():

This is a pre-existing gap (not introduced here) — ConfigureStartupConfiguration is not wired into the minimal API host path outside of GetIsolatedFactory. Worth a follow-up issue if users rely on it.


Summary

Verdict
Fix correctness ✅ Correct approach
Regression test ✅ Solid zero-override pattern
Workaround removal ✅ Appropriate cleanup
Double-registration risk ⚠️ Recommend making extension idempotent
Subclass breaking change ⚠️ Consider changelog note or extra safety net

The core fix is solid. The double-registration guard (Issue 1) is the highest-priority follow-up as it can cause subtle runtime bugs for users who migrated the workaround.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The fix is correct and well-motivated. Moving AddCorrelatedTUnitLogging() from CreateHostBuilder() to ConfigureWebHost(IWebHostBuilder) is exactly the right layer — WebApplicationFactory<T> guarantees ConfigureWebHost is called for all host models, so this closes the silent-skip path for minimal API apps.

The regression test design is particularly clean: MinimalApiTestFactory having zero overrides is self-documenting evidence that the base class is doing the work. Well done there.


Issue 1: Potential double-registration for callers who applied the workaround themselves

Any user who read the old XML doc and applied the workaround (calling AddCorrelatedTUnitLogging() in their own ConfigureWebHost override without calling base) will now get the service registered twice. Both:

  • IStartupFilter -> TUnitTestContextStartupFilter (causes TUnitTestContextMiddleware to run twice in the pipeline)
  • ILoggerProvider -> CorrelatedTUnitLoggerProvider (duplicates every log write to stdout)

ASP.NET Core allows multiple registrations of interface-typed singletons, so neither will be rejected — they will just silently double-up. The first is worse because middleware duplication can cause subtle context-overwrite bugs under concurrency.

Suggested improvement: Guard against double-registration in AddCorrelatedTUnitLogging:

public static IServiceCollection AddCorrelatedTUnitLogging(
    this IServiceCollection services,
    LogLevel minLogLevel = LogLevel.Information)
{
    if (services.Any(d => d.ImplementationType == typeof(TUnitTestContextStartupFilter)
                       || d.ImplementationInstance is TUnitTestContextStartupFilter))
        return services;

    services.AddSingleton<IStartupFilter>(new TUnitTestContextStartupFilter());
    services.AddSingleton<ILoggerProvider>(new CorrelatedTUnitLoggerProvider(minLogLevel));
    return services;
}

This makes the extension idempotent (like AddLogging() itself) and removes the migration burden from users.


Issue 2: Silent behavioral change for subclasses overriding ConfigureWebHost without calling base

Before this PR, for IHostBuilder-based apps, logging was registered in CreateHostBuilder() — independent of whether the subclass overrode ConfigureWebHost. Now the registration depends entirely on base.ConfigureWebHost(builder) being called.

Any existing subclass like:

protected override void ConfigureWebHost(IWebHostBuilder builder)
{
    // forgot to call base — they did not need to before this PR
    builder.ConfigureTestServices(services => services.AddSingleton<IMyService, MyFakeService>());
}

...will silently lose correlated logging on all host types (not just minimal API). The XML doc warns about it, but this is a semantic change to a protected override that previously had no base to call.

Suggested improvement: If the idempotency guard from Issue 1 is applied, you could keep a belt-and-suspenders registration in CreateHostBuilder as well. The double call becomes harmless, and existing subclasses that override ConfigureWebHost without calling base will still get logging via the CreateHostBuilder path for IHostBuilder-based apps. A changelog note for this behavioral change would also help.


Minor observation: ConfigureStartupConfiguration is still skipped for minimal API hosts in the shared (non-isolated) path

In CreateHostBuilder(), hostBuilder?.ConfigureHostConfiguration(ConfigureStartupConfiguration) is a null-conditional call, so for minimal API hosts where CreateHostBuilder returns null, ConfigureStartupConfiguration is never invoked on the shared factory path. This is a pre-existing gap (not introduced by this PR), but worth a follow-up issue if users rely on it.


Summary

Verdict
Fix correctness Correct approach
Regression test Solid zero-override pattern
Workaround removal Appropriate cleanup
Double-registration risk Recommend making extension idempotent
Subclass breaking change Consider changelog note or belt-and-suspenders approach

The core fix is solid. The double-registration guard (Issue 1) is the highest-priority follow-up as it can cause subtle runtime bugs for users who previously applied the workaround themselves.

@codacy-production
Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 medium

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

Results:
1 new issue

Category Results
BestPractice 1 medium

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

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


app.MapGet("/log/{marker}", (string marker) =>
{
logger.LogInformation("MINIMAL_API_LOG:{Marker}", marker);
@thomhurst thomhurst enabled auto-merge (squash) April 11, 2026 11:38
@thomhurst thomhurst disabled auto-merge April 11, 2026 12:36
@thomhurst thomhurst merged commit e0cc712 into main Apr 11, 2026
16 of 18 checks passed
@thomhurst thomhurst deleted the fix/5503-minimal-api-correlated-logging branch April 11, 2026 12:36
@claude claude bot mentioned this pull request Apr 11, 2026
1 task
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.

TestWebApplicationFactory: correlated logging not auto-registered for minimal API apps

2 participants