search

testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.65k stars 250 forks source link

feat: Make the wait strategy timeout configurable #1145

Closed 0xced closed 2 months ago

0xced commented 3 months ago

What does this PR do?

This pull request introduces a new global (static) timeout setting for wait strategies: TestcontainersSettings.WaitTimeout.

Why is it important?

Before this pull request there was no timeout (Timeout.InfiniteTimeSpan to be precise) so a buggy wait strategy could run forever. With this new timeout (defaulting to 5 minutes) tests will fail after this timeout instead of running forever.

How to test this PR

A new test (WaitStrategyTimeout) was introduced to ensure that the new TestcontainersSettings.WaitTimeout property is observed and that the exception message is actionable.

netlify[bot] commented 3 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit 02f3828d0ecb4b7f34b4e29e3a4040a6db18e5dd
Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/660078059c9b400008497ee3
Deploy Preview https://deploy-preview-1145--testcontainers-dotnet.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

HofmeisterAn commented 2 months ago

I can probably rebase the mentioned branch this weekend and finalize the remaining tasks.

0xced commented 2 months ago

To prevent tests from running forever, you can pass a cancellation token.

Yes that's true but the default behaviour here matters a lot here I think. Maybe the simplest solution would be to hardcode a timeout of 15 minutes instead of an infinite timeout. I mean, who wants to wait 15 minutes for a container to start anyway?

We can bring feature/refactor-wait-strategy-api back to life.

I briefly looked at it and it looks interesting. I have not a particularly strong opinion on this topic but I just think that waiting forever by default is not the best default.

HofmeisterAn commented 2 months ago

but I just think that waiting forever by default is not the best default.

I somehow disagree here. It is very difficult to find the sweet spot. We simply do not know which timeout is appropriate. There are hundreds of use cases and scenarios. We do not know anything about the user's bandwidth, hardware, image size, startup callbacks, complexity of the entrypoint, container engine (Linux/Windows), etc. We simply do not know how long it will take. The timeout will always be fuzzy and inaccurate. The user has the best knowledge of how long the startup will take. I prefer to improve the documentation and make it explicit how Testcontainers for .NET behaves and how it utilizes the CancellationToken. IMO this is the best approach, even better than the proposal of the wait strategy options. If tests (the wait strategies) are, for whatever reason, flaky, the user will likely need to adjust the desired timeout anyway.

HofmeisterAn commented 2 months ago

The branch is now up-to-date. I need to complete the retry implementation and update the docs.

HofmeisterAn commented 2 months ago

I was just thinking that we could add a custom configuration property, like all the others, to set the default values for the interval and timeout. This way, we can support your requirement as well.

HofmeisterAn commented 2 months ago

Most parts are prepared (feature/custom-config-wait-strategy), I just need to complete the documentation and finally integrate the new configuration.

HofmeisterAn commented 2 months ago

I will close this in favor of #1169 :v:.