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.

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

feat: Make the wait strategy timeout configurable #1145

Closed
wants to merge 1 commit into from

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Mar 24, 2024

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.

Also default to 5 minutes instead of infinite.
Copy link

netlify bot commented Mar 24, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 02f3828
🔍 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.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to avoid introducing a global configuration. We can bring feature/refactor-wait-strategy-api back to life. IIRC, there is not much left anymore. I just wanted to clean up a couple of things and change the implementation in UnsafeStartAsync(CancellationToken). WDYT? To prevent tests from running forever, you can pass a cancellation token to the container start (CancellationTokenSource(int)).

@HofmeisterAn
Copy link
Collaborator

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

@0xced
Copy link
Contributor Author

0xced commented Apr 20, 2024

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

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

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

@HofmeisterAn
Copy link
Collaborator

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

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

@HofmeisterAn
Copy link
Collaborator

I will close this in favor of #1169 ✌️.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants