serilog / serilog-settings-configuration

A Serilog configuration provider that reads from Microsoft.Extensions.Configuration
Apache License 2.0
446 stars 129 forks source link

Make sure that single-file apps can find assemblies that contains sinks #353

Closed 0xced closed 1 year ago

0xced commented 1 year ago

Before this commit, when single-file app was detected, the behaviour was to fallback on DLL scanning. But DLL scanning would not find anything for an app published as a single-file, by sheer definition of single-file app!

After this commit, an exception is thrown if the app is published as a single-file AND no Serilog:Using section is defined in the configuration. The error message explains that either a Serilog:Using section must be added or show how to explicitly configure assemblies through the ConfigurationReaderOptions.

0xced commented 1 year ago

Oops, I forgot about .NET Framework. 😁 I still have some work to do...

SimonCropp commented 1 year ago

is it not possible to add a test for this sceanrio?

0xced commented 1 year ago

Yeah absolutely. This will probably require an integration test though, compiling and running some test app. I'll probably use CliWrap to run dotnet commands.

SimonCropp commented 1 year ago

This will probably require an integration test though, compiling and running some test app. I'll probably use CliWrap to run dotnet commands.

sounds good to me

0xced commented 1 year ago

I have now added the integration tests.

👍 The upside is that it uncovered issues in the implementation that I did not catch with manual testing. It also allowed me to greatly improve my initial implementation.

👎 The downside is that running the tests went from less than 2 seconds to, well, much more than 2 seconds now that compiling an app and running it is involved.

nblumhardt commented 1 year ago

Happy to merge this one when you are; I don't think test execution time is a huge problem right now.

0xced commented 1 year ago

I think I finally figured out how to safely run integration tests for several target frameworks concurrently. I want to experiment with running tests concurrently on the command line too in order to improve execution time on AppVeyor. I'll let you know when it's ready.

0xced commented 1 year ago

I finally figured out how to run tests concurrently across target frameworks, both in an IDE (Rider or Visual Studio) and on the command line with dotnet test, see https://github.com/dotnet/sdk/issues/19147#issuecomment-1484886680. 🥳

While working on this pull request, 66 out of 661 tests were not passing in build 46609632 but the final result on AppVeyor was still ✔️ with a Build success message on the last line.

@SimonCropp I have seen that you've been working on Build.ps1 recently, do you have any idea why the build would not fail? It seems to me that the last line is correct (if($LASTEXITCODE -ne 0) { exit 3 }) so is there something to do in instead in the appveyor.yml file?

sungam3r commented 1 year ago

https://help.appveyor.com/discussions/problems/4498-powershell-exception-in-test_script-does-not-fail-build ?

sungam3r commented 1 year ago

372

0xced commented 1 year ago

This pull request is now ready to merge.