-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Make WebApplicationFactory worked with other IServers #34702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Hosting; | ||
using Microsoft.AspNetCore.Hosting.Server; | ||
using Microsoft.AspNetCore.Hosting.Server.Features; | ||
using Microsoft.AspNetCore.TestHost; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.DependencyModel; | ||
|
@@ -28,7 +29,7 @@ public class WebApplicationFactory<TEntryPoint> : IDisposable, IAsyncDisposable | |
{ | ||
private bool _disposed; | ||
private bool _disposedAsync; | ||
private TestServer? _server; | ||
private IServer? _server; | ||
private IHost? _host; | ||
private Action<IWebHostBuilder> _configuration; | ||
private readonly List<HttpClient> _clients = new(); | ||
|
@@ -78,7 +79,7 @@ public TestServer Server | |
get | ||
{ | ||
EnsureServer(); | ||
return _server; | ||
return _server as TestServer ?? throw new InvalidOperationException("Server is not a TestServer"); | ||
} | ||
} | ||
|
||
|
@@ -90,7 +91,7 @@ public virtual IServiceProvider Services | |
get | ||
{ | ||
EnsureServer(); | ||
return _host?.Services ?? _server.Host.Services; | ||
return _host?.Services ?? (_server is TestServer testServer ? testServer.Host.Services : throw new InvalidOperationException("Unable to resolve IServiceProvider")); | ||
} | ||
} | ||
|
||
|
@@ -198,11 +199,11 @@ private void ConfigureHostBuilder(IHostBuilder hostBuilder) | |
hostBuilder.ConfigureWebHost(webHostBuilder => | ||
{ | ||
SetContentRoot(webHostBuilder); | ||
webHostBuilder.UseTestServer(); // Set the server first so it can be modified by the configuration callback | ||
_configuration(webHostBuilder); | ||
webHostBuilder.UseTestServer(); | ||
}); | ||
_host = CreateHost(hostBuilder); | ||
_server = (TestServer)_host.Services.GetRequiredService<IServer>(); | ||
_server = _host.Services.GetRequiredService<IServer>(); | ||
} | ||
|
||
private void SetContentRoot(IWebHostBuilder builder) | ||
|
@@ -466,10 +467,34 @@ public HttpClient CreateDefaultClient(params DelegatingHandler[] handlers) | |
{ | ||
EnsureServer(); | ||
|
||
(HttpMessageHandler, Uri?) CreateHandler() | ||
{ | ||
if (_server is null) | ||
{ | ||
throw new InvalidOperationException("Server not available"); | ||
} | ||
|
||
if (_server is TestServer testServer) | ||
{ | ||
return (testServer.CreateHandler(), null); | ||
} | ||
|
||
var httpAddress = _server.Features.Get<IServerAddressesFeature>()?.Addresses.FirstOrDefault(a => a.StartsWith("http://")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there isn’t an HTTP address, try looking for an HTTPS one before throwing? For browser UI testing I usually configure Kestrel with a self-signed test cert so that I don’t need to disable RequireHttps for MVC or worry about behaviours in auth that require HTTPS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I should do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I will do that. Are you calling EnsureServer manually btw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't use the built-in one (at least not explicitly), instead I manually construct my own server as part of the xunit fixture init: https://github.com/martincostello/dotnet-minimal-api-integration-testing/blob/1493d95e9bb4458ac8d9b89f414e1396a55c8804/tests/TodoApp.Tests/HttpServerFixture.cs#L25 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make this cleaner you need to:
PS: WebApplicationFactory feels super messy because it has so many virtual methods which make it hard to modify 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it would be much nicer to be able to just drop down into the code here and remove all the extra stuff in my derived class beyond the customisations to the services and configuration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the ideal code you would write? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll have a think this morning and come up with some pseudo-code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without trying to think about how it works internally today too much and what I do to get this working in .NET 5 I came up with something like this for what's been changed here to not assume With the changes in this PR it would make the existing methods to get an HttpClient pointing at the server "just work" removing the need for a custom one that knows to point to the real server not the test server, and also remove the need to create and maintain its own host/server. using System.Security.Cryptography.X509Certificates;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Server;
using Microsoft.AspNetCore.Hosting.Server.Features;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
namespace TodoApp;
public sealed class HttpServerFixture : WebApplicationFactory<MyEntryPoint>
{
public string ServerAddress => ClientOptions.BaseAddress.ToString();
protected override void ConfigureWebHost(IWebHostBuilder builder)
{
base.ConfigureWebHost(builder);
builder.ConfigureKestrel(
serverOptions => serverOptions.ConfigureHttpsDefaults(
httpsOptions => httpsOptions.ServerCertificate = new X509Certificate2("localhost-dev.pfx", "Pa55w0rd!")));
// Configure the server address for the server to
// listen on for HTTPS requests on a dynamic port.
builder.UseUrls("https://127.0.0.1:0");
// Do something here (if needed?) that would add another IServer implementation to the service collection for a
// real server so that the call to UseTestServer() here effectively gets no-op'd. The tests would then just not
// use the ITestServer Server property in the base class hierarchy.
// https://github.com/dotnet/aspnetcore/blob/aa6c3d27eabfd781bae8d5157d50e1b8eaaf96f0/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs#L202
}
protected override IHost CreateHost(IHostBuilder builder)
{
// This assumes the base class has done something that means
// the host has ended up being a real server, rather than the test server.
var host = base.CreateHost(builder);
// Extra the selected dynamic port out of the server and assign it
// onto the client options for convenience so it "just works" as otherwise
// it'll be the default http://localhost URL, which won't route to the server.
var server = host.Services.GetRequiredService<IServer>();
var addresses = server.Features.Get<IServerAddressesFeature>();
ClientOptions.BaseAddress = addresses!.Addresses
.Select((p) => new Uri(p))
.Last();
return host;
}
} I might have a hack around in my test app to see how close to that I could get today without anything being changed here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be the minimal code needed for my use case with the changes from this PR. Without it fails in using System.Security.Cryptography.X509Certificates;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Server;
using Microsoft.AspNetCore.Hosting.Server.Features;
using Microsoft.AspNetCore.Mvc.Testing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
namespace MyApp;
public sealed class HttpServerFixture : WebApplicationFactory<MyEntrypoint>, IAsyncLifetime
{
public string ServerAddress => ClientOptions.BaseAddress.ToString();
Task IAsyncLifetime.InitializeAsync()
{
using (CreateDefaultClient()) // Force the server to start now
return Task.CompletedTask;
}
Task IAsyncLifetime.DisposeAsync() => Task.CompletedTask;
protected override void ConfigureWebHost(IWebHostBuilder builder)
{
base.ConfigureWebHost(builder);
builder.ConfigureKestrel(
serverOptions => serverOptions.ConfigureHttpsDefaults(
httpsOptions => httpsOptions.ServerCertificate = new X509Certificate2("localhost-dev.pfx", "Pa55w0rd!")));
// Configure the server address for the server to
// listen on for HTTPS requests on a dynamic port.
builder.UseUrls("https://127.0.0.1:0");
}
protected override IHost CreateHost(IHostBuilder builder)
{
// Use Kestrel instead of TestServer
builder.ConfigureWebHost((p) => p.UseKestrel());
var host = base.CreateHost(builder);
// Extract the selected dynamic port out of the server and assign it
// onto the client options for convenience so it "just works" as otherwise
// it'll be the default http://localhost URL, which won't route to the server.
var server = host.Services.GetRequiredService<IServer>();
var addresses = server.Features.Get<IServerAddressesFeature>();
ClientOptions.BaseAddress = addresses!.Addresses
.Select((p) => new Uri(p))
.Last();
return host;
}
} |
||
|
||
if (httpAddress is null) | ||
{ | ||
throw new InvalidOperationException("Unable to find HTTP address"); | ||
} | ||
|
||
return (new HttpClientHandler(), new Uri(httpAddress)); | ||
} | ||
|
||
var (handler, uri) = CreateHandler(); | ||
|
||
HttpClient client; | ||
if (handlers == null || handlers.Length == 0) | ||
{ | ||
client = _server.CreateClient(); | ||
client = new HttpClient(handler) { BaseAddress = uri }; | ||
} | ||
else | ||
{ | ||
|
@@ -478,10 +503,9 @@ public HttpClient CreateDefaultClient(params DelegatingHandler[] handlers) | |
handlers[i - 1].InnerHandler = handlers[i]; | ||
} | ||
|
||
var serverHandler = _server.CreateHandler(); | ||
handlers[^1].InnerHandler = serverHandler; | ||
handlers[^1].InnerHandler = handler; | ||
|
||
client = new HttpClient(handlers[0]); | ||
client = new HttpClient(handlers[0]) { BaseAddress = uri }; | ||
} | ||
|
||
_clients.Add(client); | ||
|
@@ -502,7 +526,7 @@ protected virtual void ConfigureClient(HttpClient client) | |
throw new ArgumentNullException(nameof(client)); | ||
} | ||
|
||
client.BaseAddress = new Uri("http://localhost"); | ||
client.BaseAddress ??= new Uri("http://localhost"); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -516,7 +540,7 @@ protected virtual void ConfigureClient(HttpClient client) | |
public HttpClient CreateDefaultClient(Uri baseAddress, params DelegatingHandler[] handlers) | ||
{ | ||
var client = CreateDefaultClient(handlers); | ||
client.BaseAddress = baseAddress; | ||
client.BaseAddress ??= baseAddress; | ||
|
||
return client; | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part seems the most likely to break people.
Javier, is _configuration here ever discovered from someone's Program.CreateHostBuilder? E.g. were they running CreateHostBuilder which calls UseKestrel, and then relying on UseTestServer to override Kestrel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely possible.