feat: enhance test configuration isolation and validation in WebApplicationFactory

[thomhurst]

No description provided.

[claude]

Code Review

Thank you for this PR! I've reviewed the changes to enhance test configuration isolation and validation in WebApplicationFactory. Here's my feedback:

✅ Positive Aspects

1. Improved Configuration Isolation: The addition of ConfigureStartupConfiguration allows for early configuration that runs before the web host is built, which is essential for validation scenarios like the one demonstrated in Program.cs:8-11.

2. Clear Intent: The parameter renaming (configureServices → configureIsolatedServices, etc.) makes it more explicit that these configurations are per-test isolated.

3. Proper Ordering: Moving configureWebHostBuilder to the end (line 51 in TestWebApplicationFactory.cs) is correct - it allows it to override the standard configuration when needed.

🔍 Issues & Concerns

1. Breaking Change (High Priority)

The signature of GetIsolatedFactory has changed:

• Before: 4 parameters
• After: 5 parameters (added configureIsolatedStartupConfiguration)

This is a breaking change that will affect any custom implementations or extensions. Since TUnit.AspNetCore is a public API, this should be handled carefully:

• Consider deprecating the old signature with [Obsolete] attribute
• Or provide an overload for backward compatibility
• Or ensure this is released as part of a major version bump

2. Potential Null Reference (Medium Priority)

In TestWebApplicationFactory.cs:61-63:

var hostBuilder = base.CreateHostBuilder();
hostBuilder?.ConfigureHostConfiguration(ConfigureStartupConfiguration);
return hostBuilder;

While the null-conditional operator is used, the pattern here is fragile. If base.CreateHostBuilder() returns null, the configuration silently fails to apply. Consider:

• Adding a comment explaining when/why this might be null
• Or logging when the configuration is skipped
• Or documenting this behavior in XML comments

3. Duplicated Configuration Logic (Medium Priority)

In GetIsolatedFactory (lines 29-36):

var configurationBuilder = new ConfigurationManager();
ConfigureStartupConfiguration(configurationBuilder);
configureIsolatedStartupConfiguration(configurationBuilder);

foreach (var keyValuePair in configurationBuilder.AsEnumerable())
{
    builder.UseSetting(keyValuePair.Key, keyValuePair.Value);
}

This creates a temporary ConfigurationManager, builds it, then transfers settings via UseSetting. A few concerns:

• Allocations: Creating a temporary ConfigurationManager adds allocations (though probably not in a hot path)
• Loss of Fidelity: Using UseSetting for key-value pairs may not preserve all configuration provider semantics (priorities, reload tokens, etc.)
• Consistency: The same configuration is applied via two different mechanisms:
  • Via UseSetting (lines 33-36)
  • Via CreateHostBuilder override (lines 59-66)

Recommendation: Document why both mechanisms are needed, or consolidate if possible.

4. Missing XML Documentation (Low Priority)

The new ConfigureStartupConfiguration method (lines 55-57) lacks XML documentation. Given this is a protected virtual method meant for override, it should have:

• <summary> explaining when it's called and its purpose
• <remarks> clarifying the difference from ConfigureTestConfiguration
• <example> showing typical usage

This is especially important since there are now multiple configuration extension points with similar names.

5. Test Coverage (Medium Priority)

Based on my review:

• The example in WebApplicationFactory.cs:57-63 demonstrates the feature
• The validation in Program.cs:8-11 tests that it works

However, I don't see explicit unit tests covering:

• What happens if ConfigureStartupConfiguration throws an exception?
• What happens if both ConfigureStartupConfiguration and configureIsolatedStartupConfiguration set the same key?
• The behavior when CreateHostBuilder() returns null

Recommendation: Add tests to TUnit.AspNetCore.Tests (if such a project exists) or document expected behavior.

6. Naming Clarity (Low Priority)

The term "startup configuration" may be confusing given ASP.NET Core's historical Startup.cs class. Consider:

• ConfigureHostConfiguration (aligns with IHostBuilder.ConfigureHostConfiguration)
• ConfigureEarlyConfiguration
• Add XML docs to clarify the execution order

📋 Pre-Commit Checklist (per CLAUDE.md)

Based on the TUnit development guidelines:

• ❌ Breaking Change: Signature change needs version planning or compatibility overload
• ⚠️ XML Documentation: Missing docs for new virtual method
• ⚠️ Test Coverage: No explicit unit tests for new functionality (only example usage)
• ✅ Code Style: Follows modern C# conventions
• ✅ No Performance Issues: Not in a hot path
• ✅ No Dual-Mode Impact: This is infrastructure, not test discovery/execution

🎯 Recommendations

1. Add backward compatibility: Provide an overload or obsolete the old signature
2. Add XML documentation to ConfigureStartupConfiguration
3. Add unit tests covering edge cases
4. Document the null handling in CreateHostBuilder override
5. Consider renaming to ConfigureHostConfiguration for clarity
6. Add comments explaining why configuration is applied via both UseSetting and CreateHostBuilder override

Verdict

The feature is valuable and the implementation is mostly sound, but needs attention to the breaking change and documentation before merging. I'd recommend addressing the breaking change concern (#1) as a blocker, and the others as nice-to-haves.

Great work on improving the configuration isolation! 🚀

[Copilot]

Pull request overview

This PR introduces a new ConfigureStartupConfiguration method to enable configuration setup before application startup in WebApplicationFactory, allowing tests to set configuration values that must be available when WebApplication.CreateBuilder(args) is called. The changes demonstrate this capability by adding validation in Program.cs that checks for a configuration value set via the new method.

Key changes:

• Adds ConfigureStartupConfiguration virtual method to TestWebApplicationFactory for early configuration setup
• Modifies GetIsolatedFactory to accept startup configuration as a separate parameter from app configuration
• Implements CreateHostBuilder override to apply startup configuration via ConfigureHostConfiguration

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
TUnit.AspNetCore/TestWebApplicationFactory.cs	Adds ConfigureStartupConfiguration method and integrates it into both CreateHostBuilder and GetIsolatedFactory for startup-time configuration
TUnit.AspNetCore/WebApplicationTest.cs	Updates GetIsolatedFactory call to pass the new startup configuration parameter
TUnit.Example.Asp.Net.TestProject/WebApplicationFactory.cs	Implements ConfigureStartupConfiguration to set "SomeKey" configuration value and adds trailing comma to configuration dictionary
TUnit.Example.Asp.Net/Program.cs	Adds validation to verify "SomeKey" configuration is set correctly during application startup

[Copilot]

This validation check will throw an exception during application startup if the configuration value is not set correctly. While this demonstrates that the startup configuration feature works, throwing an exception in production code for test-specific configuration is problematic.

Consider one of these alternatives:

1. Remove this validation after verifying the feature works, as it's test infrastructure code in production code
2. Guard it with an environment check: if (builder.Environment.IsDevelopment() && builder.Configuration["SomeKey"] != "SomeValue")
3. Move this validation to a dedicated test that verifies the configuration was set correctly, rather than embedding it in Program.cs

The current approach will cause the application to fail to start in any non-test environment where "SomeKey" isn't set to "SomeValue".

Suggested change

	if (builder.Configuration["SomeKey"] != "SomeValue")
	if (builder.Environment.IsDevelopment() &&
	builder.Configuration["SomeKey"] != "SomeValue")

[Copilot]

The configureWebHostBuilder escape hatch is now called AFTER the standard configuration (services, configuration, HTTP exchange capture). The PR description mentions "enhance test configuration isolation" but this changes the order compared to the comment on the removed line 29 which said "Apply user's escape hatch configuration first".

This is a subtle behavioral change: previously the escape hatch ran first and could be overridden by standard configuration; now it runs last and can override standard configuration. While the new order may be more useful (allowing tests to override framework defaults), this is a breaking change that should be:

1. Documented in the method's XML comments
2. Called out in the PR description or release notes
3. Verified against existing tests that may rely on the old order

Consider whether this ordering change is intentional and desired.

[Copilot]

Passing ConfigureTestConfiguration here causes it to be applied twice - once as startup configuration (this line) and once as app configuration (line 119). This was introduced when the new configureIsolatedStartupConfiguration parameter was added to GetIsolatedFactory.

The startup configuration parameter should either be an empty lambda or a separate dedicated method. Consider changing line 118 to _ => { } unless there's a specific need for different startup vs app configuration behavior in tests.

Suggested change

	ConfigureTestConfiguration,
	_ => { },

[Copilot]

The ConfigureStartupConfiguration method is invoked in two different code paths:

1. In CreateHostBuilder (line 63) for the global factory instance
2. In GetIsolatedFactory (line 30) for isolated per-test factories

This means that for isolated factories created via GetIsolatedFactory, the startup configuration from ConfigureStartupConfiguration will be applied twice - once when the base factory's host is created, and once when building the isolated factory. This could lead to configuration being duplicated or overridden unexpectedly.

Consider whether ConfigureStartupConfiguration should only be applied in CreateHostBuilder (for the global factory), or if the method in GetIsolatedFactory should skip calling the base ConfigureStartupConfiguration when it's already been applied by the base factory.

Suggested change

ConfigureStartupConfiguration(configurationBuilder);

[Copilot]

The new ConfigureStartupConfiguration method should have XML documentation comments to explain:

• Its purpose and when it's called
• How it differs from ConfigureTestConfiguration (in WebApplicationTest)
• That it's called during host builder creation, before the application starts
• An example of when to use it versus app configuration

This is especially important since this is part of the public API surface that test authors will extend.

Suggested change

	/// <summary>
	/// Configures host-level startup configuration for tests that use this factory.
	/// </summary>
	/// <remarks>
	/// <para>
	/// This method is invoked during creation of the generic host builder, before the
	/// application startup code runs. Values added to <paramref name="configurationBuilder"/>
	/// become part of the host configuration and can be overridden by later configuration
	/// sources in the application configuration pipeline.
	/// </para>
	/// <para>
	/// Unlike <c>ConfigureTestConfiguration</c> on <c>WebApplicationTest</c>, which configures
	/// the application configuration (for example via
	/// <see cref="IWebHostBuilder.ConfigureAppConfiguration(System.Action{WebHostBuilderContext, IConfigurationBuilder})"/>) for a
	/// specific test run, this method is intended for host-level or global defaults that
	/// apply to all tests using this <see cref="TestWebApplicationFactory{TEntryPoint}"/>.
	/// It is called as part of <see cref="IHostBuilder.ConfigureHostConfiguration(System.Action{IConfigurationBuilder})"/>.
	/// </para>
	/// <para>
	/// Use this method when you need to establish baseline configuration for the test host,
	/// such as default environment variables, in-memory key/value pairs, or connection strings
	/// that should be present for every test. For per-test or scenario-specific configuration,
	/// prefer using the <c>configureIsolatedStartupConfiguration</c> or
	/// <c>configureIsolatedAppConfiguration</c> delegates passed to
	/// <see cref="GetIsolatedFactory(TestContext, WebApplicationTestOptions, System.Action{IServiceCollection}, System.Action{IConfigurationBuilder}, System.Action{WebHostBuilderContext, IConfigurationBuilder}, System.Action{IWebHostBuilder}?)"/>
	/// or application-level hooks such as <c>ConfigureTestConfiguration</c>.
	/// </para>
	/// </remarks>
	/// <param name="configurationBuilder">
	/// The <see cref="IConfigurationBuilder"/> used to construct the host configuration
	/// before the application starts.
	/// </param>

[Copilot]

Disposable 'ConfigurationManager' is created but not disposed.

Suggested change

	var configurationBuilder = new ConfigurationManager();
	using var configurationBuilder = new ConfigurationManager();