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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from May 5, 2023
Merged

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

merged 13 commits into from May 5, 2023

Conversation

0xced
Copy link
Member

@0xced 0xced commented Mar 10, 2023

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 0xced requested review from nblumhardt and removed request for nblumhardt March 10, 2023 10:35
@0xced 0xced marked this pull request as draft March 10, 2023 20:42
@0xced
Copy link
Member Author

0xced commented Mar 10, 2023

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

@0xced 0xced force-pushed the single-app-no-assemblies-exception branch from 561e41e to 0a2672a Compare March 10, 2023 21:55
@0xced 0xced marked this pull request as ready for review March 10, 2023 21:55
@SimonCropp
Copy link
Contributor

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

@0xced
Copy link
Member Author

0xced commented Mar 10, 2023

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
Copy link
Contributor

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 0xced force-pushed the single-app-no-assemblies-exception branch 4 times, most recently from 488787a to c48a4a2 Compare March 14, 2023 23:23
@0xced 0xced force-pushed the single-app-no-assemblies-exception branch 8 times, most recently from cad4f87 to a7baebf Compare March 16, 2023 01:10
@0xced 0xced force-pushed the single-app-no-assemblies-exception branch from a7baebf to 431413a Compare March 16, 2023 07:14
@0xced
Copy link
Member Author

0xced commented Mar 16, 2023 •
edited

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.

0xced added a commit to 0xced/serilog-settings-configuration that referenced this pull request Mar 16, 2023
@0xced
Improve SelfLog when a configuration method is not found
ccf4dc8 
List candidate methods only if there are any. Discovered this weird log while working on serilog#353:
> Unable to find a method called WithThreadName. Candidate methods are:
@0xced 0xced mentioned this pull request Mar 16, 2023
Merged
@nblumhardt
Copy link
Member

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

@0xced
Copy link
Member Author

0xced commented Mar 25, 2023

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
Make sure that single-file apps can find assemblies that contains sinks
e8b2a39 
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
Add integration tests
ecddb13
@0xced
Rename AutoAssemblyFinder into CompositeAssemblyFinder
a812ab1
@0xced
Add some diagnostics with ITestOutputHelper when running the TestApp exe
9a28018
@0xced
Run tests on the TestApp when published as self-contained
92d37fc 
This is when the default DependencyContext is actually null.

Also run all the tests on .NET 6 which is a Long Term Support (LTS) release.
@0xced
Simplify loading assemblies
74402f8 
Use a HashSet instead of a Dictionary keyed by the assembly full name. If an assembly is loaded twice with the same AssemblyName it will be the same instance so a HashSet (without even a custom IEqualityComparer<Assembly>) is the perfect solution.

Also, Assembly.Load can throw many exceptions but won't return null.
@0xced
Make tests successfully run concurrently across multiple target frame…
2b6c040 
…works
@0xced
Parallelize tests across target frameworks on AppVeyor
e71c549
@0xced
Use a nuget.config file instead of specifying multiple sources on the…
4828123 
… command line

This works around dotnet/sdk#27202
It also has the benefit of using settings _only_ from the specified config file, ignoring the global nuget.config where package source mapping could interfere with the local source.
@0xced 0xced force-pushed the single-app-no-assemblies-exception branch from c9185a2 to 4828123 Compare March 27, 2023 09:36
@0xced
Copy link
Member Author

0xced commented Mar 27, 2023

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 dotnet/sdk#19147 (comment). 🥳

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
Copy link
Contributor

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

@sungam3r
Copy link
Contributor

sungam3r commented Apr 2, 2023

#372

@0xced
Refactor the TestApp fixture
2cb45f5 
Add a new PublishAsync(PublishMode) method and also add a comment explaining why it would not work to publish multiple apps in parallel.
@0xced
Mark integration tests as such with [Trait("Category", "Integration")]
895f83d
@0xced
Copy link
Member Author

0xced commented Apr 6, 2023

This pull request is now ready to merge.

@nblumhardt nblumhardt merged commit 325577e into serilog:dev May 5, 2023
1 check passed
@nblumhardt nblumhardt mentioned this pull request May 10, 2023
Merged
@0xced 0xced deleted the single-app-no-assemblies-exception branch May 12, 2023 08:04
@tetsuo13 tetsuo13 mentioned this pull request May 24, 2023
Closed
@0xced 0xced mentioned this pull request Jul 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonCropp SimonCropp SimonCropp left review comments

@sungam3r sungam3r sungam3r left review comments

@nblumhardt nblumhardt nblumhardt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@0xced @SimonCropp @nblumhardt @sungam3r