feat: Add WithLogger(ILogger) builder API

[0xced]

What does this PR do?

This pull request moves the logger configuration from the static TestcontainersSettings class to the AbstractBuilder class.

Before:

TestcontainersSettings.Logger = logger;

After:

new TBuilderEntity().WithLogger(logger).Build()

Why is it important?

This will enable a seamless integration with xUnit.net (both with ITestOutputHelper and with IMessageSink).

Related issues

Fixes #996

[netlify]

✅ Deploy Preview for testcontainers-dotnet ready!

Name	Link
🔨 Latest commit	66ec29f
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-dotnet/deploys/65ef31dc4fd9ec0008539e1f
😎 Deploy Preview	https://deploy-preview-1100--testcontainers-dotnet.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[0xced]

Continuing discussion from #1093 (comment) to here (where it belongs).

The PR does not contain the Xunit project, right? I think it is a good idea!

Indeed this pull request does include the new Xunit project. I will submit another pull request dedicated to the new Xunit integration.

A lot of developers will benefit from it. I have not taken a close look into the PR that involves the changes to the logging implementation yet. I took a quick look, and it is quite big.

It's big because it removes the ILogger parameter from all containers in all modules but the actual change in AbstractBuilder`4.cs is suprisingly small.

IIRC, I proposed to split it into two steps: one that provides an internal WithLogger(ILogger) builder method, and another that makes the necessary changes regarding logging the container runtime information and making the method public. After that, we can add the dedicated Xunit, package. WDYT? Splitting it makes reviewing much easier.

I tried but I don't see how it's possible to split in two steps. Again, if you ignore the required changes in the modules the core feature diff is not that big.

[HofmeisterAn]

I haven't had the time yet, but I will review it in the next few days.

[0xced]

Great, I'll refrain from submitting new pull requests in the meantime. 😉

[HofmeisterAn]

It took me some time to review everything, and as I got closer to completion, I identified and refactored a couple of crucial parts. As mentioned in a previous comment, I would like to avoid logging the runtime information either too frequently (as is the case now) or just once in cases where developers cannot easily find it. While it still requires some polishing, I believe we are very close now. I have included tests to ensure that the runtime information is only logged once per logger instance (LoggerTest). This guarantees the presence of the runtime information in cases where developers use the ITestOutputHelper, which, IMO, looks quite good. I can provide more details and necessary follow-ups tomorrow, but for today, I am finished and tired. If you have some time to review the changes as well, that would be awesome and much appreciated. I am certain that we can make further improvements.

[HofmeisterAn]

The PR is ready for a final review. @0xced your feedback is much appreciated (no hurry). I am happy to merge the PR afterward.

[0xced]

Awesome, I'll try to have a look this week.

[0xced]

I just pushed 1ee0e3e which uses the configured ILogger for the resource reaper too.

I think the configured logger should also be passed to the PortForwardingContainer, I'm looking into it…

[HofmeisterAn]

I think the configured logger should also be passed to the PortForwardingContainer, I'm looking into it…

We can also change this with a follow-up PR. Probably, we simply need to replace the singleton property with a method that provides an ILogger overload.

[0xced]

OK, let's take care of PortForwardingContainer in a follow up PR.

I see that you removed the structured logging of runtime information that I had introduced. I re-introduced it in a slightly improved way in fef7faa. I think it makes the LogContainerRuntimeInfoAsync implementation much easier to read and also it's the right way to do structured logging. But if you don't like it, just drop the commit.

One last question: why did you introduce an empty TestcontainersSettings static constructor in e535cbe?

[HofmeisterAn]

I think it makes the LogContainerRuntimeInfoAsync implementation much easier to read and also it's the right way to do structured logging. But if you don't like it, just drop the commit.

Can we please revert it? I really do not like it, and structured logging does not have a lot of benefits here. I would like to remove Logging anyway in the future. Logging the message immediately is much easier. Happy to merge the pull request afterward and publish a beta version.

[0xced]

OK fine, I’ll revert.

What about the empty TestcontainersSettings static constructor?

Happy to merge the pull request afterward and publish a beta version.

Could you please have a look at #1139 before publishing? I promise it’s much simpler than this one. 😉

[HofmeisterAn]

What about the empty TestcontainersSettings static constructor?

This guarantees lazy initialization (probably not really important here), see beforefieldinit.

Could you please have a look at #1139 before publishing? I promise it’s much simpler than this one. 😉

Yes, we can merge this before publishing a beta version too.

[0xced]

I just pushed 1bf2f96 which removes the structure logging and I extracted the logging code into a new method for improved readability, and also catching only the exceptions when running GetSystemInfoAsync and GetVersionAsync.

This guarantees lazy initialization (probably not really important here), see beforefieldinit.

Wow, I had no idea, gotta agree with Mike Marynowski!

[...] controlling the timing of when you want a class to run its static initialization shouldn't be controlled entirely by the presence or absence of a static constructor, that's just so odd...

I think it's now ready to be merged. 😀