tests: migrate integration tests to TUnit.AspNetCore WebApplicationTest#1100
Merged
BenjaminMichaelis merged 18 commits intoMay 17, 2026
Merged
Conversation
- Add TUnit.AspNetCore 1.40.5 package reference - Create IntegrationTestBase : WebApplicationTest<WebApplicationFactory, Program> for per-test factory isolation, InServiceScope helpers, and trace correlation - Rewrite WebApplicationFactory to extend TestWebApplicationFactory<Program> (removes IAsyncInitializer, moves to ConcurrentBag for multi-connection tracking) - Migrate all test classes to inherit from IntegrationTestBase: - Remove [NotInParallel] and [ClassDataSource<WebApplicationFactory>] attributes - Collapse 6 McpRateLimitingTests classes into 1 (fresh DI per test = fresh rate limiter) - Update McpTestHelper to accept TracedWebApplicationFactory<Program> - Use Factory.Inner.CreateClient(options) where AllowAutoRedirect=false is needed - Remove TUnit.Core.Interfaces import (IAsyncInitializer no longer used)
… following TracedWebApplicationFactory.CreateClient() uses CreateDefaultClient() which sets AllowAutoRedirect=false. Tests expecting 200/404 after app-level redirects (e.g. / -> first chapter) need Factory.Inner.CreateClient() which defaults to AllowAutoRedirect=true.
…pe helpers - McpTestHelper.CreateClient: factory.Inner.CreateClient(options) -> factory.CreateClient() TracedWebApplicationFactory.CreateClient() already uses AllowAutoRedirect=false; going through Inner was silently bypassing activity propagation for all MCP tests. - ContentRateLimitingTests: same fix - Factory.Inner -> Factory.CreateClient() - FunctionalTests: introduce CreateRedirectFollowingClient() helper on IntegrationTestBase to document the explicit AllowAutoRedirect=true trade-off; all three test methods use it since the app issues redirects even for 404 responses. - IntegrationTestBase: add InServiceScopeAsync<T> and InServiceScopeAsync overloads so callers no longer need to manually manage IServiceScope for async DB/service work. - WebApplicationFactory: add Interlocked disposal guard to prevent double-dispose of SqliteConnection instances when both DisposeAsync and Dispose(bool) are called.
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the integration test project from a hand-rolled WebApplicationFactory<Program> + IAsyncInitializer setup to TUnit.AspNetCore's first-class WebApplicationTest<TFactory, TEntryPoint> pattern, giving per-test factory isolation, trace correlation, and removing the need for [NotInParallel] and [ClassDataSource] attributes.
Changes:
- Adds
TUnit.AspNetCore1.40.5; subclassesTestWebApplicationFactory<Program>and tracks SQLite connections in aConcurrentBagwithInterlockeddisposal guard. - Introduces
IntegrationTestBaseprovidingInServiceScope[Async]helpers and aCreateRedirectFollowingClient()escape hatch for redirect-following clients. - Refactors all integration test classes to inherit
IntegrationTestBase; merges six MCP rate-limiting classes into one (each method now gets a fresh factory/limiter).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Directory.Packages.props | Add TUnit.AspNetCore 1.40.5 package version. |
| EssentialCSharp.Web.Tests/EssentialCSharp.Web.Tests.csproj | Reference new TUnit.AspNetCore package. |
| EssentialCSharp.Web.Tests/WebApplicationFactory.cs | Switch base type, track connections in ConcurrentBag, add disposal guard. |
| EssentialCSharp.Web.Tests/IntegrationTestBase.cs | New base class with service-scope helpers and redirect-following client helper. |
| EssentialCSharp.Web.Tests/McpTestHelper.cs | Accept TracedWebApplicationFactory<Program>; drop AllowAutoRedirect=false. |
| EssentialCSharp.Web.Tests/McpRateLimitingTests.cs | Collapse six classes into one with six tests; remove [NotInParallel]. |
| EssentialCSharp.Web.Tests/McpTests.cs | Use new base; replace factory with Factory. |
| EssentialCSharp.Web.Tests/McpToolContractTests.cs | Use new base; replace factory with Factory. |
| EssentialCSharp.Web.Tests/McpApiTokenServiceTests.cs | Use new base; replace factory with Factory. |
| EssentialCSharp.Web.Tests/ContentRateLimitingTests.cs | Use new base; inline AllowAutoRedirect=false no longer needed (default). |
| EssentialCSharp.Web.Tests/FunctionalTests.cs | Use new base and CreateRedirectFollowingClient() to preserve redirect-following behavior. |
| EssentialCSharp.Web.Tests/ListingSourceCodeControllerTests.cs | Use new base; replace factory with Factory. |
| EssentialCSharp.Web.Tests/RouteConfigurationServiceTests.cs | Use new base; call InServiceScope on the base. |
| EssentialCSharp.Web.Tests/SitemapXmlHelpersTests.cs | Use new base; replace _Factory with Factory. |
The shared _disposed flag was blocking base.Dispose(bool) when DisposeAsync ran first — the base's async path may call Dispose(true), which our guard intercepted, skipping base resource cleanup. Fix: use a _connectionsDisposed flag that only gates the SQLite connection cleanup (which we own). Base disposal calls are always forwarded. Also removes the redundant GC.SuppressFinalize — the base class handles it. Addresses Copilot PR review comments.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
EssentialCSharp.Web.Tests/WebApplicationFactory.cs:69
- If
EnsureCreated()(or any other call between line 52 and 75) throws, theSqliteConnectioncreated on line 52 is not disposed — the local variable goes out of scope while the connection is still open. Consider wrapping the connection in ausing(or try/catch) and only handing ownership over after schema creation succeeds, or hooking it up to the factory's disposal so failures during configuration don't leak the connection and in-memory database.
SqliteConnection connection = new(SqlConnectionString);
connection.Open();
services.AddSingleton<DbConnection>(connection);
services.AddDbContext<EssentialCSharpWebContext>((serviceProvider, options) =>
{
DbConnection dbConnection = serviceProvider.GetRequiredService<DbConnection>();
options.UseSqlite(dbConnection);
});
DbContextOptions<EssentialCSharpWebContext> dbContextOptions =
new DbContextOptionsBuilder<EssentialCSharpWebContext>()
.UseSqlite(connection)
.Options;
using EssentialCSharpWebContext db = new(dbContextOptions);
db.Database.EnsureCreated();
… DbConnection - Rename CreateClientWithoutRedirectFollowing -> CreateClientWithoutAutoRedirect (clearer name, avoids double-negative) - Fix XML doc comment on the helper (was copy-pasted from GetFollowingGetRedirectsAsync) - Change DbConnection from singleton to scoped with a keep-alive connection, preventing SQLite 'database is locked' errors under concurrent requests - Add Dispose(bool) override to clean up _keepAliveConnection - Update PR description: correct method name and hosted-service ordering description
Remove prior matching registrations for DbContext options, DbConnection, DatabaseMigrationService, and EnsureCreatedHostedService before re-adding test overrides. Also keep the SQLite keep-alive connection creation guarded with ??= across repeated ConfigureServices invocations. This avoids duplicate EnsureCreated hosted services racing each other and failing CI with SQLite Error 1: table AspNetRoles already exists.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
EssentialCSharp.Web.Tests/WebApplicationFactory.cs:126
- The keep-alive connection is disposed before
base.Dispose(disposing)is called.base.Disposeis what tears down the test host and its DI container, which may still hold scopedSqliteConnectioninstances and/or trigger graceful shutdown logic that touches the database (e.g. hosted services' StopAsync, EF interceptors). Once the keep-alive connection is closed first, the shared-cache in-memory database can be dropped while host shutdown is still running, which can produce spurious "no such table" or "database is locked" errors during disposal. Consider callingbase.Dispose(disposing)first and disposing_keepAliveConnectionafterwards.
protected override void Dispose(bool disposing)
{
if (disposing)
{
_keepAliveConnection?.Dispose();
}
base.Dispose(disposing);
}
EssentialCSharp.Web.Tests/WebApplicationFactory.cs:136
EnsureCreatedHostedServicecreates a service scope synchronously withusing IServiceScope scope = ...but the scope resolves a scopedDbConnection(an async-disposableSqliteConnection) and an EFDbContext. Synchronous disposal of a scope containing async-disposable resources can run their fallback synchronous Dispose path, which is not ideal for SQLite connections opened against an in-memory shared cache. Preferawait using IServiceScope scope = ...(orAsyncServiceScope) so the scope is disposed asynchronously, consistent with the asyncStartAsyncmethod.
public async Task StartAsync(CancellationToken cancellationToken)
{
using IServiceScope scope = serviceProvider.CreateScope();
EssentialCSharpWebContext dbContext =
scope.ServiceProvider.GetRequiredService<EssentialCSharpWebContext>();
await dbContext.Database.EnsureCreatedAsync(cancellationToken);
}
EssentialCSharp.Web.Tests/WebApplicationFactory.cs:110
EnsureCreatedHostedServiceis inserted before the first descriptor whoseServiceType == typeof(IHostedService)to "guarantee it runs first". This relies onGenericWebHostService(the descriptor that actually starts the Kestrel/test server) being the first registered hosted service. If any other hosted service was registered earlier inProgram.cs(or gets added later via a library),EnsureCreatedHostedServicewill instead run just before that service but still after any preceding hosted services, which would then start without a schema. A more robust approach is to clear/preserve allIHostedServicedescriptors and re-insert withEnsureCreatedHostedServiceexplicitly first, or to seed the schema synchronously insideConfigureWebHost(as the previous code did) before the host starts.
ServiceDescriptor ensureCreatedDescriptor =
ServiceDescriptor.Singleton<IHostedService, EnsureCreatedHostedService>();
int firstHostedServiceIndex = -1;
for (int i = 0; i < services.Count; i++)
{
if (services[i].ServiceType == typeof(IHostedService))
{
firstHostedServiceIndex = i;
break;
}
}
if (firstHostedServiceIndex >= 0)
{
services.Insert(firstHostedServiceIndex, ensureCreatedDescriptor);
}
else
{
services.Add(ensureCreatedDescriptor);
}
…in functional tests Add a static SemaphoreSlim gate around EnsureCreatedHostedService.StartAsync so parallel host startup cannot race while creating schema in shared in-memory SQLite. Also use CreateClientWithoutAutoRedirect in FunctionalTests so redirect handling stays explicit in GetFollowingGetRedirectsAsync.
… explicit Use explicit single-or-none descriptor removal (throws if duplicates are present) for DbContext options, DbConnection, DatabaseMigrationService, and EnsureCreatedHostedService. Keep schema creation serialized via semaphore. Update FunctionalTests to use CreateClientWithoutAutoRedirect so manual redirect handling remains intentional.
…tracts - Use traced client creation paths in IntegrationTestBase and McpTestHelper - Make InServiceScope helper methods protected - Clarify GET-only redirect helper intent - Remove unnecessary explicit System.Threading using - Use pooled SQLite connection string and clarify scoped DbConnection ownership - Insert EnsureCreatedHostedService at index 0 for deterministic startup ordering - Remove global schema semaphore after hardening descriptor replacement checks
Use a per-factory SemaphoreSlim for EnsureCreatedHostedService so concurrent host startup for the same WebApplicationFactory cannot race while creating the in-memory SQLite schema. Remove Pooling=True from the in-memory SQLite connection string to keep database lifetime governed by the explicit keep-alive connection.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
EssentialCSharp.Web.Tests/WebApplicationFactory.cs:88
EnsureCreatedHostedServiceis registered asSingleton<IHostedService>capturing the rootIServiceProvider. Inserting it at index 0 only guarantees it runs first among services already in the collection at the time ofInsert(0, …); anyIHostedServiceregistered later in the pipeline (e.g. by framework code or otherConfigureServicescalls run after this one) will still execute before it isn’t affected, but more importantly the framework's existing schema-dependent hosted services (which this is intended to precede) may already be earlier in the list or get re-ordered. Consider sorting/asserting position by re-checking before host build, or using a more deterministic mechanism (e.g. anIHostStartupFilteror runningEnsureCreatedexplicitly beforeServeris materialized).
ServiceDescriptor ensureCreatedDescriptor =
ServiceDescriptor.Singleton<IHostedService>(
serviceProvider => new EnsureCreatedHostedService(
serviceProvider,
_schemaInitializationGate));
services.Insert(0, ensureCreatedDescriptor);
EssentialCSharp.Web.Tests/WebApplicationFactory.cs:104
_schemaInitializationGateis aSemaphoreSlimthat is never disposed. Although schema initialization runs only once per factory, the semaphore is held for the factory's lifetime andDispose(bool disposing)only disposes_keepAliveConnection. Add_schemaInitializationGate.Dispose()in thedisposingbranch.
protected override void Dispose(bool disposing)
{
if (disposing)
{
_keepAliveConnection?.Dispose();
}
base.Dispose(disposing);
}
- Restore AllowAutoRedirect=false clients via Factory.Inner.CreateClient(...) in IntegrationTestBase and McpTestHelper - Initialize keep-alive SQLite connection in WebApplicationFactory constructor - Keep per-factory schema initialization gate to prevent EnsureCreated races in CI
Comment on lines
+133
to
151
| private sealed class EnsureCreatedHostedService( | ||
| IServiceProvider serviceProvider, | ||
| SemaphoreSlim schemaInitializationGate) : IHostedService | ||
| { | ||
| base.Dispose(disposing); | ||
| if (disposing) | ||
| public async Task StartAsync(CancellationToken cancellationToken) | ||
| { | ||
| _Connection?.Dispose(); | ||
| _Connection = null; | ||
| await schemaInitializationGate.WaitAsync(cancellationToken); | ||
| try | ||
| { | ||
| using IServiceScope scope = serviceProvider.CreateScope(); | ||
| EssentialCSharpWebContext dbContext = | ||
| scope.ServiceProvider.GetRequiredService<EssentialCSharpWebContext>(); | ||
| await dbContext.Database.EnsureCreatedAsync(cancellationToken); | ||
| } | ||
| finally | ||
| { | ||
| schemaInitializationGate.Release(); | ||
| } | ||
| } |
Dispose the last HttpResponseMessage in GetFollowingGetRedirectsAsync before throwing when max redirects is exceeded.
…open Clarify that EnsureCreatedHostedService is prepended via Insert(0), and guard per-scope SqliteConnection.Open() with dispose-on-failure semantics.
Comment on lines
+106
to
+111
| if (disposing) | ||
| { | ||
| _keepAliveConnection.Dispose(); | ||
| _schemaInitializationGate.Dispose(); | ||
| } | ||
| base.Dispose(disposing); |
| private readonly string _sqlConnectionString = | ||
| $"DataSource=file:{Guid.NewGuid():N}?mode=memory&cache=shared"; | ||
|
|
||
| private readonly SemaphoreSlim _schemaInitializationGate = new(1, 1); |
Comment on lines
+18
to
+19
| public static HttpClient CreateClient(TracedWebApplicationFactory<Program> factory) => | ||
| factory.Inner.CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }); |
Comment on lines
+22
to
+52
| protected static async Task<HttpResponseMessage> GetFollowingGetRedirectsAsync( | ||
| HttpClient client, | ||
| string relativeUrl, | ||
| int maxRedirects = 10) | ||
| { | ||
| HttpResponseMessage response = await client.GetAsync(relativeUrl); | ||
|
|
||
| for (int redirectCount = 0; | ||
| redirectCount < maxRedirects && IsRedirectStatusCode(response.StatusCode); | ||
| redirectCount++) | ||
| { | ||
| Uri? location = response.Headers.Location; | ||
| if (location is null) | ||
| { | ||
| return response; | ||
| } | ||
|
|
||
| response.Dispose(); | ||
|
|
||
| response = await client.GetAsync(location); | ||
| } | ||
|
|
||
| if (IsRedirectStatusCode(response.StatusCode)) | ||
| { | ||
| response.Dispose(); | ||
| throw new InvalidOperationException( | ||
| $"Exceeded redirect limit ({maxRedirects}) for '{relativeUrl}'. Last status: {(int)response.StatusCode} {response.StatusCode}."); | ||
| } | ||
|
|
||
| return response; | ||
| } |
Comment on lines
+46
to
+48
| response.Dispose(); | ||
| throw new InvalidOperationException( | ||
| $"Exceeded redirect limit ({maxRedirects}) for '{relativeUrl}'. Last status: {(int)response.StatusCode} {response.StatusCode}."); |
Comment on lines
+10
to
12
| /// Each test method gets its own per-test factory (fresh IHost + rate limiter state) | ||
| /// via TUnit.AspNetCore's WebApplicationTest, so [NotInParallel] is no longer needed. | ||
| /// </summary> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates
EssentialCSharp.Web.TeststoTUnit.AspNetCoreusingWebApplicationTest<WebApplicationFactory, Program>andTestWebApplicationFactory<Program>.What changed
TUnit.AspNetCorepackage.IntegrationTestBasewith sharedInServiceScopehelpers.[ClassDataSource]/ constructor factory injection toIntegrationTestBaseinheritance.McpTestHelpernow acceptsTracedWebApplicationFactory<Program>and creates clients with explicitAllowAutoRedirect = falsewhere redirect assertions require it.Current implementation details
WebApplicationFactorystores a per-instance GUID-based SQLite connection string. A_keepAliveConnectionis held open for the factory lifetime to keep the shared-cache in-memory database alive.DbConnectionis registered as scoped so each request scope gets its ownSqliteConnection, avoiding concurrent-access locking from sharing a single connection instance.EnsureCreatedHostedService, registered at index0in the service collection so hosted-service startup runs schema creation first.EnsureCreatedAsync()races in CI.GetFollowingGetRedirectsAsync(GET-only redirect helper) and non-auto-redirect clients where status/location assertions depend on raw redirect responses.Notes
ConcurrentBag<SqliteConnection>,Interlockeddisposal guards, orCreateRedirectFollowingClient().