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

Aspnetcore 7 WebApplicationFactory doesn't get default minimum configuration correctly #332

Closed
wzchua opened this issue Oct 12, 2022 · 11 comments · Fixed by #413
Closed

Comments

@wzchua
Copy link

wzchua commented Oct 12, 2022

Checking for the minimum section returns "" instead of null, so the logic doesn't get the value from Default

@nblumhardt
Copy link
Member

Hi! Thanks for the report - any chance of a code sample/failing test that demonstrates the issue?

@wzchua wzchua changed the title NET 7 doesn't get default minimum configuration correctly Aspnetcore 7 WebApplicationFactory doesn't get default minimum configuration correctly Oct 12, 2022
@wzchua
Copy link
Author

wzchua commented Oct 12, 2022

Hmm, I made the incorrect diagnosis .

I hit this using https://learn.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-7.0 with net7.0.

What happens is that the WebApplicationFactory forwards a bunch of configuration as arguments for the program.

for an appsetting of

{
    "Serilog": {
      "MinimumLevel": {
          "Default": "Information",
      }
    }
  }

It sets the below as arguments

var configuration = new ConfigurationBuilder()
    .AddCommandLine(new []
    {
        "--Serilog=",
        "--Serilog:MinimumLevel=",
        "--Serilog:MinimumLevel:Default=Information"
    })
    .Build();

var logger = new LoggerConfiguration()
    .ReadFrom.Configuration(configuration)
    .CreateLogger();

which the logger configuration cannot handle.

Something changed in how they set the configuration resulted in this new behavior.

@wzchua
Copy link
Author

wzchua commented Oct 12, 2022

repro https://github.com/wzchua/WebApplicationFactoryBugWithSerilogConfiguration

public class UnitTest1  :
    IClassFixture<CustomWebApplicationFactory<Program>>
{
    private readonly HttpClient _client;
    private readonly CustomWebApplicationFactory<Program>
        _factory;

    public UnitTest1(CustomWebApplicationFactory<Program> factory)
    {
        _factory = factory;
        _client = factory.CreateClient(new WebApplicationFactoryClientOptions
        {
            AllowAutoRedirect = false
        });
    }

    [Fact]
    public async Task Test1()
    {
        var message = await _client.GetAsync("/");
    }
}

public class CustomWebApplicationFactory<TProgram>
    : WebApplicationFactory<TProgram> where TProgram : class
{
    protected override IHost CreateHost(IHostBuilder builder)
    {
        builder.ConfigureHostConfiguration(configurationBuilder =>
        {
            configurationBuilder.AddInMemoryCollection(new Dictionary<string, string?>
            {
                {"Serilog:MinimumLevel:Default", "Debug"},
            });
        });

    builder.UseSerilog((context, configuration) =>
        {
            configuration.ReadFrom.Configuration(context.Configuration);
            configuration.WriteTo.Debug();
        });
        return base.CreateHost(builder);
    }
}

@wzchua
Copy link
Author

wzchua commented Oct 12, 2022

Okay I can workaround this by using ConfigureAppConfiguration instead of ConfigureHostConfiguration.

I am not sure if this is an issue for this library. You can close this if it's not actionable.

@nblumhardt
Copy link
Member

Thanks for all the details, sounds like it needs some investigation 👍

@0xced
Copy link
Member

0xced commented Feb 6, 2023

I think using ConfigureAppConfiguration is the right thing to do:

Sets up the configuration for the remainder of the build process and application. This can be called multiple times and the results will be additive. The results will be available at Configuration for subsequent operations, as well as in Services.

Using ConfigureHostConfiguration is not appropriate to configure Serilog:

Set up the configuration for the builder itself. This will be used to initialize the IHostEnvironment for use later in the build process. This can be called multiple times and the results will be additive.

@peabnuts123
Copy link

I'm pretty sure there is a bug here though I am not entirely sure where (e.g. Serilog, Microsoft.AspNetCore.Mvc.Testing, etc.)

Basically when using WebApplicationFactory (i.e. Integration Tests) the following Serilog config will not work:

// appsettings.json

  "Serilog": {
    "MinimumLevel": {
      "Default": "Information"
    }
  }

This throws an exception like the following:

Error Message:
   System.InvalidOperationException : The value  is not a valid Serilog level.
  Stack Trace:
     at Serilog.Settings.Configuration.ConfigurationReader.ParseLogEventLevel(String value)
…

Note the double space in there: The value␣␣is not a valid Serilog level for (presumably) the empty string to be inserted into (i.e. $"The value {level} is not a valid Serilog level").

But the following works fine:

// appsettings.json

  "Serilog": {
    "MinimumLevel": "Information"
  }

I am using code like the following to set this up but I don't think it matters:

public class ComponentTestsFixture : WebApplicationFactory<Program>
{
    protected override void ConfigureWebHost(IWebHostBuilder builder)
    {
        var config = new ConfigurationBuilder()
                .SetBasePath(Directory.GetCurrentDirectory())
                .AddJsonFile("appsettings.json", optional: false);
        builder.UseConfiguration(config.Build());
    }
}

I've also tried using ConfigureAppConfiguration() but this doesn't appear to be called until after my Program.cs has run which does all the setup for Serilog and stuff so it isn't really useful.

@peabnuts123
Copy link

@nblumhardt

@nblumhardt
Copy link
Member

@peabnuts123 a fix would be welcome if you can track this down, I don't have anything to add.

@wzchua
Copy link
Author

wzchua commented Mar 9, 2024

The min repro is

var configuration = new ConfigurationBuilder()
    .AddCommandLine(new []
    {
        "--Serilog=",
        "--Serilog:MinimumLevel=",
        "--Serilog:MinimumLevel:Default=Information"
    })
    .Build();

var logger = new LoggerConfiguration()
    .ReadFrom.Configuration(configuration)
    .CreateLogger();

the fix likely would involve ignoring the key Serilog:MinimumLevel when Serilog:MinimumLevel:Default exists

@Pastafarian
Copy link

Just run into this myself, be great if we could get a release with the fix in it!

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants