Make WebApplicationFactory worked with other IServers

[davidfowl]

• Don't assume the TestServer is the only IServer implementation that works with the WebApplicationFactory. Fix CreateClient to handle HttpClient creation for different kinds of IServer implementations by getting the IServerAddressesFeature and populating the HttpClient with that URL (preferring http or https).

Contributes to #33846

cc @martincostello

[martincostello]

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.

Example: https://github.com/martincostello/dotnet-minimal-api-integration-testing/blob/1493d95e9bb4458ac8d9b89f414e1396a55c8804/tests/TodoApp.Tests/HttpServerFixture.cs#L70-L76

[davidfowl]

Yes, I should do that.

[davidfowl]

Yes I will do that. Are you calling EnsureServer manually btw?

[martincostello]

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

[davidfowl]

To make this cleaner you need to:

• Have the ability to start the server (I'd prefer if you didn't need to manually do it)
• Get a handle on the address so you can construct a client

PS: WebApplicationFactory feels super messy because it has so many virtual methods which make it hard to modify 😄

[martincostello]

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.

[davidfowl]

What's the ideal code you would write?

[martincostello]

I'll have a think this morning and come up with some pseudo-code.

[martincostello]

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 TestServer too much.

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.

[martincostello]

I think this would be the minimal code needed for my use case with the changes from this PR. Without it fails in EnsureStarted() due to the IServer not being of type TestServer (System.InvalidCastException : Unable to cast object of type 'Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServerImpl' to type 'Microsoft.AspNetCore.TestHost.TestServer'.).

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;
    }
}

[javiercn]

• Don't assume the TestServer is the only IServer implementation that works with the WebApplicationFactory. Fix CreateClient to handle HttpClient creation for different kinds of IServer implementations by getting the IServerAddressesFeature and populating the HttpClient with that URL (preferring http or https).

This is changing a fundamental aspect of the design, we should discuss this more before we move on with this. I worry that there are many things we can get away with WAF because it runs in-memory that you can't get away with an alternative server implementation.

[Tratcher]

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?

[davidfowl]

Definitely possible.

[davidfowl]

This is changing a fundamental aspect of the design, we should discuss this more before we move on with this. I worry that there are many things we can get away with WAF because it runs in-memory that you can't get away with an alternative server implementation.

It has the same caveats as using the test server. You can't set state on the client that will be observed by the server. Exceptions don't bubble client side etc.

[martincostello]

Any further thoughts on if this change is acceptable? Would be great to get this in for RC1 😃

[javiercn]

It has the same caveats as using the test server. You can't set state on the client that will be observed by the server. Exceptions don't bubble client side etc.

I think its more than that, you are not spinning real servers that will compete for resources.

I'm not sure how some of the other configs, service replacements, etc. you can do with the in memory version will work here (for the record, I believe they will).

Things like debugging will offer a poor experience because the real server will time out when you stop to debug things.

This is based on experience when working with tests that spin up real servers (like our E2E tests).

My recommendation would be that if you want to enable this, think about alternative options that produce a good E2E experience, I'm just not sure this is going to be the case. That said, its just my advice, if you want to move forward that's fine with me, I'm just a bit worried that this will open a can of worms and provide little value over testing thing in memory.

(All our MVC tests run in memory and I can't remember a time where we caught a bug that was specific to the fact that we were not running on an in memory server).

To be very clear, the technical aspect is not the issue here, it's the experience and the longer term maintenance cost (bugs, poor E2E experience, inconsistencies between using different servers, unmatched expectations from customers), that I'm worried about, against the value of running a real server over the in-memory implementation.

I think its better to have an experience that is explicitly designed to run real servers explicitly than add something to WAF with the risk of cornering yourself in the future.

@davidfowl This is just my advice and I'll trust you to decide on how to best move forward here.

[Tratcher]

I agree with @javiercn, WAF wasn't designed for this and there will be a lot of gatchas enabling it. If we want to go down this route we should wait until the start of .NET 7 so there's time to sort out the end-to-end.