Skip to content
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

Decouple WebApplicationFactory and TestServer #33846

Closed
martincostello opened this issue Jun 25, 2021 · 32 comments
Closed

Decouple WebApplicationFactory and TestServer #33846

martincostello opened this issue Jun 25, 2021 · 32 comments
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-mvc-testing MVC testing package Priority:3 Work that is nice to have
Milestone

Comments

@martincostello
Copy link
Member

Describe the bug

As part of looking into the new features in ASP.NET Core 6 (top-level statements, minimal APIs, etc.) I've been looking at how to refactor the integration test approach I've been using with previous versions of .NET Core where WebApplicationFactory is available so that it works with the new approaches.

For integration tests where a UI is required, such as for browser automation tests, I've been tackling this by creating a derived WebApplicationFactory class to piggy-back its features to bootstrap an HTTP server using Kestrel so that there's a real HTTP port being listened to so that tools like Playwright and Selenium can interact with the application to test it.

These tests work by using the CreateHostBuilder()/CreateWebHostBuilder() methods to access the build for the application and then manually creating it (rather than using the TestServer the class usually provides) (example). The reason for re-using the WebApplicationFactory code is that there's a logic embedded within it for finding the default content root, ensuring the .deps.json files are there etc., which is a fair chunk of additional code to copy and maintain to otherwise replicate the approach with only minor tweaks on top (i.e. a real server). It also gives good code coverage of the same code that runs in production, rather than having to use an alternate code path just for the purpose of tests.

Trying this out with the changes from #33462 using a preview 6 daily build however doesn't work for this scenario. This is because in the top-level statements scenario both methods return null and the deferred implementation is private to the EnsureServer() method:

[MemberNotNull(nameof(_server))]
private void EnsureServer()
{
if (_server != null)
{
return;
}
EnsureDepsFile();
var hostBuilder = CreateHostBuilder();
if (hostBuilder is not null)
{
ConfigureHostBuilder(hostBuilder);
return;
}
var builder = CreateWebHostBuilder();
if (builder is null)
{
var deferredHostBuilder = new DeferredHostBuilder();
// This helper call does the hard work to determine if we can fallback to diagnostic source events to get the host instance
var factory = HostFactoryResolver.ResolveHostFactory(
typeof(TEntryPoint).Assembly,
stopApplication: false,
configureHostBuilder: deferredHostBuilder.ConfigureHostBuilder,
entrypointCompleted: deferredHostBuilder.EntryPointCompleted);
if (factory is not null)
{
// If we have a valid factory it means the specified entry point's assembly can potentially resolve the IHost
// so we set the factory on the DeferredHostBuilder so we can invoke it on the call to IHostBuilder.Build.
deferredHostBuilder.SetHostFactory(factory);
ConfigureHostBuilder(deferredHostBuilder);
return;
}

If the implementation were to be refactored in a way that supported the existing scenarios by allowing the consuming class to get access to the DeferredHostBuilder as an IHostBuilder, then I presume that the use case I have today would work if that builder was used to bootstrap an application with it instead.

Off the top of my head, maybe something like this could be a possible approach:

protected virtual IHostBuilder? CreateHostBuilder()
{
    var hostBuilder = HostFactoryResolver.ResolveHostBuilderFactory<IHostBuilder>(typeof(TEntryPoint).Assembly)?.Invoke(Array.Empty<string>());

    if (hostBuilder is null)
    {
        var deferredHostBuilder = new DeferredHostBuilder();
        var factory = HostFactoryResolver.ResolveHostFactory(
            typeof(TEntryPoint).Assembly,
            stopApplication: false,
            configureHostBuilder: deferredHostBuilder.ConfigureHostBuilder,
            entrypointCompleted: deferredHostBuilder.EntryPointCompleted);

        if (factory is not null)
        {
            deferredHostBuilder.SetHostFactory(factory);
            hostBuilder = deferredHostBuilder;
        }
    }

    hostBuilder?.UseEnvironment(Environments.Development);
    return hostBuilder;
}

The EnsureServer() method would then just consume the deferred implementation without actually having any knowledge of it, and derived classes would be able to use the deferred implementation without being aware of the actual implementation details.

I've got a GitHub repo here with a sample TodoApp using this test approach using ASP.NET Core 6 preview 5 here (it doesn't use minimal APIs yet, mainly due to this issue), and there's a branch using a preview 6 daily build with this approach that fails to run the tests due to the lack of access to a host builder.

While maybe this isn't an intended use case of WebApplicationFactory, it's been working well since ASP.NET Core 2.1 and would require a fair bit of work to move away from to leverage the new functionalities in various application codebases in order to adopt ASP.NET Core 6.

If some minimal refactoring could be done that doesn't break the intended design, which I would be happy to contribute to, that could get this sort of use case working again with ASP.NET Core 6 using top-level statements that would be appreciated.

/cc @davidfowl

To Reproduce

To reproduce, clone the preview-6 branch of my work-in-progress sample application.

Further technical details

  • .NET SDK 6.0.100-preview.6.21324.1 (a daily build)
  • Microsoft.AspNetCore.Mvc.Testing version 6.0.0-preview.6.21323.4
  • Visual Studio 2022 17.0.0 Preview 1.1
@davidfowl
Copy link
Member

I originally did this but it breaks applications that were using the CreateWebHost pattern, that's why it got moved to be a fallback. I can think of a sneaky way to solve this if we what you suggest above and only use the IHostBuilder if it's not the DeferredHostBuilder. That is, we would keep the precedence as it is today but CreateHostBuilder wouldn't return null.

But stepping back a bit here, it seems like we should just decouple the WebApplicationFactory from the TestServer.

@martincostello
Copy link
Member Author

But stepping back a bit here, it seems like we should just decouple the WebApplicationFactory from the TestServer.

Yeah, I definitely think there would be a benefit in exposing the building blocks that are in the implementation details out so you can compose them up in different ways. I'm not sure what form they'd take, but factoring the .deps, content root etc. bits out into some sort of ""helpers"" that could be re-used, and then having WebApplicationFactory be refactored to utilise those would remove the need to sort-of user TestServer but then ignore it completely.

I feel like maybe this was discussed in a different PR or issue for WebApplicationFactory I was involved in previously, but there was some concern about doing so. I'll have a search and see if I recall correctly or if I've just imagined it 😄

@martincostello
Copy link
Member Author

I feel like maybe this was discussed in a different PR or issue for WebApplicationFactory I was involved in previously, but there was some concern about doing so. I'll have a search and see if I recall correctly or if I've just imagined it 😄

It was this comment here I was thinking of: #7414 (comment)

@javiercn javiercn added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 25, 2021
@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label Jun 28, 2021
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Jun 28, 2021
@ghost
Copy link

ghost commented Jun 28, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl
Copy link
Member

I wonder if we can make a small change here to make the TestServer property be null if it a real server is in use. Instead typing the server here as IServer. Then change CreateClient to use the IServer to get the server addresses to create a real http client if it's not a test server. I can play with this tonight.

@martincostello
Copy link
Member Author

The proposed changes would still be better as I could remove yet more code, but I had a think about this a bit more, and managed to refactor my usage to get rid of the gnarly reflection to access the deferred host builder and move my "hook" into CreateHost(). The only bit I don't like is that the real server has to be started first, otherwise you can't get the server addresses. I guess that's a consequence of the deferred host(builder) and the callback/event that gets fired by that once the application is built so the WebApplicationFactory<T> callbacks can run.

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

public sealed class HttpServerFixture : WebApplicationFactory<MyEntrypoint>
{
    private bool _disposed;
    private IHost? _host;

    public string ServerAddress
    {
        get
        {
            EnsureServer();
            return ClientOptions.BaseAddress.ToString();
        }
    }

    public override IServiceProvider Services
    {
        get
        {
            EnsureServer();
            return _host!.Services!;
        }
    }

    protected override void ConfigureWebHost(IWebHostBuilder builder)
    {
        base.ConfigureWebHost(builder);

        builder.ConfigureKestrel(
            serverOptions => serverOptions.ConfigureHttpsDefaults(
                httpsOptions => httpsOptions.ServerCertificate = new X509Certificate2("localhost-dev.pfx", "Pa55w0rd!")));

        builder.UseUrls("https://127.0.0.1:0");
    }

    protected override IHost CreateHost(IHostBuilder builder)
    {
        // Create the host for TestServer now before we
        // modify the builder to use Kestrel instead.
        var testHost = builder.Build();

        // Modify the host builder to use Kestrel instead
        // of TestServer so we can listen on a real address.
        builder.ConfigureWebHost((p) => p.UseKestrel());

        // Create and start the Kestrel server before the test server,
        // otherwise due to the way the deferred host builder works
        // for minimal hosting, the server will not get "initialized
        // enough" for the address it is listening on to be available.
        _host = builder.Build();
        _host.Start();

        // Extract the selected dynamic port out of the Kestrel 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 Kestrel-hosted HTTP server.
        var server = _host.Services.GetRequiredService<IServer>();
        var addresses = server.Features.Get<IServerAddressesFeature>();

        ClientOptions.BaseAddress = addresses!.Addresses
            .Select((p) => new Uri(p))
            .Last();

        // Return the host that uses TestServer, rather than the real one.
        // Otherwise the internals will complain about the host's server
        // not being an instance of the concrete type TestServer.
        testHost.Start();
        return testHost;
    }

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);

        if (!_disposed)
        {
            if (disposing)
            {
                _host?.Dispose();
            }

            _disposed = true;
        }
    }

    private void EnsureServer()
    {
        // This forces WebApplicationFactory to bootstrap the server
        using var _ = CreateDefaultClient();
    }
}

@mkArtakMSFT mkArtakMSFT added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Aug 12, 2021
@rafikiassumani-msft rafikiassumani-msft added the Priority:0 Work that we can't release without label Aug 24, 2021
@rafikiassumani-msft rafikiassumani-msft added Priority:2 Work that is important, but not critical for the release Priority:3 Work that is nice to have and removed Priority:0 Work that we can't release without Priority:2 Work that is important, but not critical for the release labels Aug 31, 2021
@rafikiassumani-msft rafikiassumani-msft modified the milestones: 6.0-rc2, Backlog Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@halter73 halter73 changed the title Unable to get host builder from derived class when using top-level statements with WebApplicationFactory Decouple WebApplicationFactory and TestServer Oct 12, 2021
@rafikiassumani-msft rafikiassumani-msft added the feature-mvc-testing MVC testing package label Dec 28, 2021
@achselschweisz
Copy link

The proposed changes would still be better as I could remove yet more code, but I had a think about this a bit more, and managed to refactor my usage to get rid of the gnarly reflection to access the deferred host builder and move my "hook" into CreateHost(). The only bit I don't like is that the real server has to be started first, otherwise you can't get the server addresses. I guess that's a consequence of the deferred host(builder) and the callback/event that gets fired by that once the application is built so the WebApplicationFactory<T> callbacks can run.

Slightly off-topic but I need to mention this here anyway:
Thanks to this solution I finally managed to get integration tests running using Kestrel + uds (unix domain sockets). Cheers!

@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
@KSemenenko
Copy link

is there any updates? maybe .net 9?

@martincostello
Copy link
Member Author

It's too late for .NET 9 - this wouldn't be until .NET 10 at the earliest now.

@KSemenenko
Copy link

Then I push temporary solution https://github.com/managedcode/IntegrationTestBaseKit
I add http liner, signalR client and playwright.

@pacoteinnov
Copy link

When calling .Build() twice, I get: "System.InvalidOperationException : Build can only be called once." Any idea why? (.NET 6 and .NET 7)

Looks like it has been there for a long time: https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs#L150C1-L154

You probably have a public static IHostBuilder CreateHostBuilder(string[] args) somewhere that gets called.

@serefarikan
Copy link

serefarikan commented Jan 29, 2025

Just dropped by to say that this would help us a lot. We have multiple products being built on Asp.Net and we are planning for a whole heap of playwright tests. I'm currently scratching my head about how to get this done. It would be fantastic if we had this out of the box.

@martincostello
Copy link
Member Author

@serefarikan I have a sample project that shows how to build Playwright tests on top of the Mvc.Testing package if it's helpful to you: martincostello/dotnet-minimal-api-integration-testing. I use this approach for all my UI apps.

@serefarikan
Copy link

@martincostello wow! That's a nice gift at the end of a long work day! I'll study this. Many thanks

@mkArtakMSFT
Copy link
Member

I originally did this but it breaks applications that were using the CreateWebHost pattern, that's why it got moved to be a fallback. I can think of a sneaky way to solve this if we what you suggest above and only use the IHostBuilder if it's not the DeferredHostBuilder. That is, we would keep the precedence as it is today but CreateHostBuilder wouldn't return null.

@davidfowl I see how that would fail if the order we try to instantiate the host is as follows:

  1. CreateHostBuilder
  2. CreateWebHostBuilder

However, if we switch it, will it still break? Am I missing something? Something like this, I mean:

 var builder = CreateWebHostBuilder();
  if (builder is not null)
  {
    SetContentRoot(builder);
    _configuration(builder);
    _server = CreateTestServer(builder);
    return;
  }
  
  var hostBuilder = CreateHostBuilder (); // This will now contain the DeferredHostBuilder 
  if (hostBuilder is null)
  {
    throw ...
  }
  ConfigureHostBuilder(hostBuilder);

@mkArtakMSFT
Copy link
Member

@martincostello one more thought. Why don't you use a much simpler approach for testing against a real server? That's what we used for Blazor E2E tests, and here is a sample repo that demonstrates the basic approach: https://github.com/MackinnonBuck/blazor-playwright-example/tree/main/BlazorServerPlaywright/BlazorServerPlaywright.Test

As you can see, the WebApplicationFactory isn't used at all. In which areas this approach is not good enough for real browser testing?

@martincostello
Copy link
Member Author

From memory, I think the main reasons were:

  • Easier ability to reconfigure the server's services (e.g. replace a service implementation with a mock)
  • WAF's built-in helpers to do things like find the runtimedeps.json files and the content root location.

Essentially, using WAF to get the side-effects in provides without needing it itself. I think that's then why I suggested making those things usable separately from WAF itself.

@mkArtakMSFT
Copy link
Member

I thought a bit more about this proposal (specifically the one in my comment with the order change) and I feel like I we move more logic (the DeferredHostBuilder creation) into the CreateHostBuilder - that would be potentially a breaking change. Specifically, if someone have and override of CreateHostBuilder and that returns null, The EnsureServer() implementation will follow-up by creating the DeferredHostBuilder, and if we move that into the CreateHostBuilder, we basically break that flow. So, not so great approach.

And if I don't merge these two methods - seems that there is no change from @martincostello 's sample point of view - no benefit.

@mkArtakMSFT
Copy link
Member

What if we change the SetContentRoot to be protected, so that derived classes can call it on demand?

protected void SetContentRoot(IWebHostBuilder builder)

@martincostello
Copy link
Member Author

martincostello commented Feb 24, 2025

Wouldn't you still need to derive from WAF to use that then?

At a glance, all of this code appears to not really need the state of the class to work, it just depends on TEntrypoint, so what about if it was exposed publicly somewhere instead?

Thinking out loud:

public static class SomeClassNameForWebHostBuilderExtensions
{
    public static IWebHostBuilder SetContentRootForTests<TEntryPoint>(this IWebHostBuilder builder)
    {
        // Copy of the existing private logic in WebApplicationFactory<T>...

        return builder;
    }
}

Then WAF could just be refactored to call it:

private void SetContentRoot(IWebHostBuilder builder)
{
    builder.SetContentRootForTests<TEntryPoint>(builder);
}

Essentially just moving the code and exposing it for re-use without needing to derive from WebApplicationFactory<T> at all.

@mkArtakMSFT
Copy link
Member

Thanks for additional information, @martincostello. But I thought you still need to derive from WAF (WebApplicationFactory) because you wanted to alter some service registrations in DI for test / validation purposes.
And if you don't want to modify any services, can't you just start the server as in the sample I've pointed to where you don't even need to change anything (neither the content root)? Or do you have an example where starting the server would fail because the content root is not correct?

@martincostello
Copy link
Member Author

For apps where I do want to do that, yes. I meant for the case just where the content root and runtimedeps parts were wanted.

I might have an example for the content root somewhere, but I'll need to go hunting around for one as I set up most of my test approach on top of WAF years ago once it was all working and haven't had much need to change it since (other than Minimal APIs adding the deferred host builder) 😄. It's possible that since I needed to do that (I seem to remember I there were issues with MVC apps when rendering Razor views) it's become redundant and isn't needed anymore.

@martincostello
Copy link
Member Author

On reflection, I think I was confusing things with UseSolutionRelativeContentRoot() that I set in a bunch of projects.

But in pretty much every case I route the app's logging to xunit, so I need to mutate the service collection anyway.

@mkArtakMSFT
Copy link
Member

This is great, thanks @martincostello. Sounds like there is no real need for moving the content root related logic out then, and the changes as they are may be good enough for now.

@martincostello
Copy link
Member Author

Sure - I'll circle back to this issue once I've tried out the other changes once they land in a shipped preview.

@mkArtakMSFT
Copy link
Member

@martincostello we had a design-discussion around the approach and have decided to take a more straight-forward approach instead. Have a look at #60635

@KSemenenko
Copy link

I love idea to use Kasterl!

@mkArtakMSFT
Copy link
Member

Closing this as I believe the issue is now addressed.
I would appreciate if people would give the upcoming .NET 10 Preview 4 builds a try, in case there is something important I may have missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-mvc-testing MVC testing package Priority:3 Work that is nice to have
Projects
No open projects
Status: No status
Development

No branches or pull requests

13 participants