Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

TestServer IServer.Features is null and makes it overly difficult to test middleware relying on ServerFeatures being populated. #967

Closed
mattmazzola opened this issue Mar 18, 2017 · 1 comment

Comments

@mattmazzola
Copy link
Contributor

Use Case:
I have written a stateless middleware which uses the address the server is configured to listen on and I want to write test for this middleware.

Example of extension method trying to extract listening addresses from server features:

public static IApplicationBuilder UseServiceFabricRouting(this IApplicationBuilder builder)
{
    var serverAddressesFeature = builder.ServerFeatures.Get<IServerAddressesFeature>();
    return builder.UseStatelessHttpMiddleware(new ServiceFabricRoutingMiddleware(serverAddressesFeature.Addresses.ToList()));
}

Expected:
I should be able to write tests using the normal aspnetcore convention of using TestServer such as below:

// Arrange
var webHostBuilder = new WebHostBuilder()
    .UseUrls("http://localhost:8000/appName/serviceName")
    .Configure(b => b
        .UseServiceFabricRouting()
        .Run(c =>
        {
            c.Response.StatusCode = (int)HttpStatusCode.OK;
            return Task.FromResult(0);
        }));

var testServer = new TestServer(webHostBuilder);

// Act
var response = await testServer
    .CreateRequest(path: "someOtherService")
    .SendAsync("GET");

// Assert
response.StatusCode.Should().Be(HttpStatusCode.Gone);

Actual:
I had to copy the implementation of TestServer into my own ExtendedTestServer class which is burdensome. The whole point is for the tests to feel familiar and not be one off solution so I want my copy class to feel just like TestServer. This means I also have to duplicate the associated RequestBuilder class since it takes concrete dependency of TestServer. Also the TestServer has method which invokes internal constructor of WebSocket so I can't even implement that method.

I think all of the above could be avoided with some very simple tweaks.

More background on server setup and why coping implementation was only solution (extending class or wrapping class does not work)

Normal server setup:
1: UseKestrel() will automatically setup the FeatureCollection

Features = new FeatureCollection();
_serverAddresses = new ServerAddressesFeature();
Features.Set<IServerAddressesFeature>(_serverAddresses);

https://github.com/aspnet/KestrelHttpServer/blob/632780dd16729298933f96796e6a969f3470d810/src/Microsoft.AspNetCore.Server.Kestrel/KestrelServer.cs#L54

  1. WebHost EnsureServer method will copy all the urls from the settings to ServerAddressesFeature
                var addresses = Server.Features?.Get<IServerAddressesFeature>()?.Addresses;
                if (addresses != null && !addresses.IsReadOnly && addresses.Count == 0)
                {
                    var urls = _config[WebHostDefaults.ServerUrlsKey] ?? _config[DeprecatedServerUrlsKey];
                    if (!string.IsNullOrEmpty(urls))
                    {
                        foreach (var value in urls.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries))
                        {
                            addresses.Add(value);
                        }
                    }
                }

var addresses = Server.Features?.Get<IServerAddressesFeature>()?.Addresses;

  1. Middleware would be able to access builder.ServerFeatures.Get<IServerAddresssesFeature>() becuase builder.ServerFeatures would not be null.

TestServer setup:
Step 1 is not done, and then step 2 is no op because addresses is null.

  • Cannot extend TestServer because you don't have ability to set base classes IServer.Features property.
    It only has getter and also there's no way to execute the code before the base class invokes Build()

  • Cannot wrap TestServer because the internal instance of TestServer is immedately invoking Build()

var host = builder.UseServer(this).Build();
host.Start();

var host = builder.UseServer(this).Build();

Becuase of this, the internal instance of TestServer (this in line above) is the instance that is used for the Server property in the WebHost. Then it would still be checking the IServer.Features on the TestServer instance not the parent class attempting to wrap it and failing to achieve desired result.

Solutions:
I'm sure there are many ways to get the desired result but here are a few I could think of:

  1. Add another constructor to TestServer to allow taking FeatureCollection as parameter which assigns it before invoking Build / Start
    I think this would work but would only solve issue for FeatureCollection, perhaps there are more things people might want to do with TestServer before the web host is built.

  2. Add setter to FeatureCollection and another constructor which doesn't immediately invoke Build / Start. This would allow extending the TestServer class and settings FeatureCollection up in parent constructor and then later invoking build / start.

Would be interested to see what other solutions there are from people more familiar with C# features.

@mattmazzola
Copy link
Contributor Author

I added PR which implements the second solution proposed above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants