search

apple / swift-log

A Logging API for Swift
https://apple.github.io/swift-log/
Apache License 2.0
3.48k stars 284 forks source link

Add a testing log handler proposal #316

Open brimworks opened 3 weeks ago

brimworks commented 3 weeks ago

Proposal for implementing a testing log handler which is provided by swift-log. For additional context, please see this github issue: https://github.com/apple/swift-log/issues/242

brimworks commented 3 weeks ago

So, I started putting together an example implementation, and in the process I'm refactoring the existing swift-log tests so they don't use LoggingSystem.bootstrapInternal() since multiple calls to LoggingSystem.boostrap() triggers the "Logging.swift:617: Precondition failed: logging system can only be initialized once per process." error.

...and low and behold, the LoggingTest testMultiplex test is failing due to the fact that some extra medatadata (["example": example-value]) is getting injected as a side-effect of another concurrently executing test.

So, whatever solution we come up with needs to be a bit less fragile in the face of concurrent test execution than the current solution. Note that I intend to do minimal changes to the current test solution so we can be assured that a bug in our test logger doesn't get hidden. Additionally, it will allow us to iterate on the test logger implementation provided by this proposal without impacting the other unit tests. Longer term we can see about removing that redundancy, but I'd rather do that as a separate PR.

I'll update this PR with my proposal soon.

brimworks commented 3 weeks ago

PR has been updated with a proposed implementation. I also refactored the tests so they rely on bootstrapInternal() a bit less, although I couldn't completely remove the usage of this since some of the tests are testing the interactions at a "global" level.

Overall, I'd suggest that we minimize the usage of bootstrapInternal() though.

brimworks commented 1 day ago

@FranzBusch I have been thinking over your comment over the course of the last week:

I am still not convinced this is the right thing to do in tests here. IMO if developers want to test their logs I don't think they should use bootstrapping of a test log handler but rather they should accept a Logger and we can create a Logger(handler: TestLogerHandler(). This would make this proposal a lot simpler since we don't need containers anymore.

Furthermore, it would push developers into the right pattern which is to accept a Logger at their APIs and potentially fallback to create a Logger if none is passed.

Although I agree with your sentiment (everyone should allow a Logger to be "injected" into their public APIs), I don't think this is a realistic requirement that we should be hoisting on anyone that wants to add some rigor to their logs by testing that the desired log output is achieved.

Therefore, I would rather "embrace the genius of the and" and encourage users of the library to inject in their log handler manually, but still allow bootstrapping to a singular test log handler instance.

So, the way I take your feedback is that we need an easy way to inject the test log handler without requiring a bootstrap. Let me see if I can come up with a good solution to this problem.

brimworks commented 17 hours ago

@brimworks said: So, the way I take your feedback is that we need an easy way to inject the test log handler without requiring a bootstrap. Let me see if I can come up with a good solution to this problem.

BTW, I changed the test case to use the "easily inject a TestLogHandler instance" way which avoids bootstrapping. Let me know if this addresses your concern.

Thanks!