Decouple WebApplicationFactory from TestServer implementation

[mkArtakMSFT]

Background and Motivation

We have a long-standing popular issue tracking improvement of automated browser testing with real server. Part of the ask is to decouple the WebApplicationFactory from the TestServer implementation, as they're currently [tightly coupled] (

aspnetcore/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs

Line 29 in 2edcf3b

private TestServer? _server;

).

Unfortunately, the TestServer is also exposed in WebApplicationFactory via a public property Server. Hence, decoupling requires an API change proposed below.

The second change is exposing the initialization logic publicly. Without this change, customers had to call the CreateClient() API which would internally initialize the server but wouldn't use the returned HttpClient instance. This will make it more intuitive and avoid creating unnecessary objects. Here is a screenshot from a blogpost pointing out this odd usage pattern:

Another example of not-so-great solution that the community came up with can be found here:

Here, the developer calls the CreateDefaultClient() in a derived class to force initialization.

With the proposed changes, customers can now simply call server.Initialize() instead which is more intuitive.

Proposed API

New abstraction / interface to depend on instead of the TestServer implementation.

namespace Microsoft.AspNetCore.TestHost;

public interface ITestServer : IServer
{
    IWebHost Host { get; }
    HttpMessageHandler CreateHandler();
    HttpClient CreateClient();
}

- public class TestServer : IServer
+ public class TestServer : ITestServer

Usage / API change in WebApplicationFactory:

namespace Microsoft.AspNetCore.Mvc.Testing;

public partial class WebApplicationFactory<TEntryPoint> : IDisposable, IAsyncDisposable where TEntryPoint : class
{
    [Obsolete("This property is obsolete. Consider utilizing the TestServer property instead.")]
    public TestServer? Server { get; }
   +public ITestServer? TestServer { get; }

   - private void EnsureServer()
   + public void Initialize()


    [Obsolete("This method is obsolete. Consider utilizing the CreateTestServer method instead.")]
    protected virtual TestServer CreateServer(IWebHostBuilder builder) => new TestServer(builder);
    + protected virtual ITestServer CreateTestServer(IWebHostBuilder builder) => CreateServer(builder);
}

Usage Examples

With this change, developers will be able to avoid creating artificial server instances only to fulfil the contract. Instead, here is an example of how a developer will override the newly added CreateTestServer method, that will utilize the very same server created by the host and use an adapter to utilize it as ITestServer:

protected override ITestServer CreateTestServer(IWebHostBuilder builder)
{
    var webHost = builder.Build();
    webHost.Start();

    var server = webHost.Services.GetRequiredService<IServer>();
    RootUri = new Uri(server.Features.Get<IServerAddressesFeature>().Addresses.LastOrDefault());
    ClientOptions.BaseAddress = RootUri;
    return new KestrelTestServerAdapter(server, webHost);
}

Note, that the KestrelTestServerAdapter type is something the developers will need to come up with. Here is a sample implementation:

Sample IServer adapter

Below is the adapter class to encapsulate real IServer implementation and expose it as ITestServer.
 

internal class KestrelTestServerAdapter : ITestServer
{
    private readonly IServer _server;
    private readonly IWebHost _host;

    public KestrelTestServerAdapter(IServer server, IWebHost webHost)
    {
        _server = server ?? throw new ArgumentNullException(nameof(server));
        _host = webHost ?? throw new ArgumentNullException(nameof(webHost));
    }

    public IWebHost Host => _host;

    public IFeatureCollection Features => _server.Features;

    public HttpClient CreateClient()
    {
        return _host.Services.GetService<HttpClient>();
    }

    public HttpMessageHandler CreateHandler()
    {
        return new HttpClientHandler();
    }

    public void Dispose()
    {
        this._server.Dispose();
        this._host.Dispose();
    }

    public Task StartAsync<TContext>(IHttpApplication<TContext> application, CancellationToken cancellationToken) where TContext : notnull
    {
        return _server.StartAsync<TContext>(application, cancellationToken);
    }

    public Task StopAsync(CancellationToken cancellationToken)
    {
        return _server.StopAsync(cancellationToken);
    }
}

Alternative Designs

Risks

[halter73]

   - protected virtual TestServer CreateServer(IWebHostBuilder builder)
   + protected virtual ITestServer CreateServer(IWebHostBuilder builder)

This is a breaking change. Someone could have a call like the following in their code. TestServer realTestServer = base.CreateServer(builder). And beyond this somewhat unlikely source break, I think it would result in type load exceptions for any classes that override the method, but I haven't tested that part.

❌ DISALLOWED: Changing the type of a property, field, parameter, or return value

https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules

[mkArtakMSFT]

It is indeed a breaking change, @halter73. This is intentional. And there will be an announcement for this change. I can, technically, utilize a pattern similar to the one I've used with the Server / TestServer properties, where I obsolete one and add a new method instead. What do you think?

[halter73]

If possible, I would prefer a non-breaking version of this change, even if it's a bit less ergonomic. We can including the breaking change as an alternative design or vice-versa.

[mkArtakMSFT]

With the properties it was easier, as they were read-only. Whereas with the CreateServer method, the user has to provide an implementation, and it is being called from within the WebApplicationFactory. So if we choose to take this route, the approach would be something like this:

var server = CreateTestServer();    // existing method that we will obsolete
if (server is null)
  server = CreateServer(); // the new method to 

This way we would prefer the new method over the old one, and eventually in the future will remove the CreateServer with the if condition alltogether.

Thoughts?

[halter73]

What if we had something like this?

public virtual ITestServer CreateServerInterface(IWebHostBuilder builder) => CreateServer(builder);

var server = CreateServerInterface();

I don't love ending the method name in "Interface", but something like that seems to strike a balance between making the new functionality easy to use without breaking anyone. I'm not sure we ever would need to remove CreateServer. Things would be a bit cleaner without it, but it doesn't seem bad enough to warrant a breaking change.

[mkArtakMSFT]

Ok, I went with the following approach, which is consistent with the Server/TestServer properties I had:

    [Obsolete("This method is obsolete. Consider utilizing the CreateTestServer method instead.")]
    protected virtual TestServer CreateServer(IWebHostBuilder builder) => new TestServer(builder);

    protected virtual ITestServer CreateTestServer(IWebHostBuilder builder) => CreateServer(builder);

[mkArtakMSFT]

@martincostello, @shanselman I know that you've done some work related to WebApplicationFactory in the past. I would appreciate your feedback regarding the approach I've taken here. As you can see in the Usage Example section in the description, the CreateTestServer implementation will be the way to utilize the very same server & host without creating anything extra. What do you think?

[dotnet-policy-service]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[martincostello]

This looks reasonable to me. I'll be sure to try it out with my sample project (which is basically where the second motivating example comes from) once there's a version in a shipped preview to give further feedback on.

The obsolete way suggested is definitely better than the hard-breaking option, as I've written some internal libraries that derive from WAF to enapsulate some of these workarounds and it avoids breaking them at runtime if used in a .NET 10 application without recompiling for net10.0.

Thinking out loud, is there any value in changing Initialize() to virtual so people can tweak the behaviour in a derived class? Just a hypothetical question at this stage without trying to actually use it in something existing.

- public void Initialize()
+ public virtual void Initialize()

Would the proposal here also tackle #33846, or is that a separate ask?

[mkArtakMSFT]

Thank you, @martincostello.
I think the whole idea for introducing the ITestServer is to help with decoupling for sure.
I also see your point in the linked issue, and just dropped a comment there too, as the refactoring the DeferredHostBuilder part into the CreateHostBuilder resonates with me too: #33846 (comment)

[shanselman]

I like it

[mkArtakMSFT]

Closing this in favor of an alternative approach: #60758