-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve WaitAllStrategy. #1272
Improve WaitAllStrategy. #1272
Conversation
263be19
to
0de8c36
Compare
core/src/test/java/org/testcontainers/containers/wait/strategy/WaitAllStrategyTest.java
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/containers/wait/strategy/WaitAllStrategyTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/containers/wait/strategy/WaitAllStrategyTest.java
Outdated
Show resolved
Hide resolved
Do we have a race conditions in the tests, that is happening on CI? Looks like this on CirlceCI and Travis was hanging. I retriggered both. |
Hmm, @kiview I added tests with timeouts not longer than 20 milliseconds. If they hang, that would be somewhere deeper. Maybe CI hang because I forced push onto this branch? |
I've retriggered the CI jobs - let's see... |
I've rerun the build several times but it's looking like it's always getting stuck here:
Given that this test involves wait strategies too, I get the feeling that there's something broken. I've not had time to dig into what, just yet. |
That would be this one, right? testcontainers-java/core/src/test/java/org/testcontainers/junit/DockerComposeWaitStrategyTest.java Line 45 in f113979
Does it involve the |
Oh yes it does indeed: The newly created So that "hang" could be fixed by fixing https://github.com/testcontainers/testcontainers-java/blob/master/core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java#L334 which could need some improvement IMHO. The logic described in the comment feels wrong. Have to think about that for a bit. |
@michael-simons Yes I will be at Javaland, would be cool to look into this together and maybe hack on it :) |
Only test current and not deprecated strategies and remove usage of deprecated Mockito argument matchers.
Currently, inner (child) waiting strategies handling their own timeout, don't play nice with the wait all strategy. The commit fixes this by pushing down the configured timeout to all children.
213e04e
to
82a518e
Compare
Hi @kiview. Here’s the change as discussed in JavaLand. It took the longest time to think about names. If you want to make sure that wait all times out when using individual timeouts I’d suggest using another enclosing strategy instead of mixing those concerns. Apart from that, I rebased on master. |
Or, if you prefer a failsafe, that could be another mode, that doesn’t do the check on changing timeouts and doesn’t push down the timeouts to the inner strategies. I could add this as well. While thinking about it: would be a nice solution in the end. |
82a518e
to
533eff1
Compare
That should cover now all the modes. |
Re-triggered Circle-CI, it seems this was something during dependency download. |
@rnorth @bsideup In the current implementation, a user won't be able to use So while we are breaking the default behavior here, I'm in favor of doing it. |
Merged, thanks to @michael-simons :) |
Released in 1.11.2! |
This is a PR addressing #1269.
I have split it up into two commits:
WaitAllStrategyTest
(don't test deprecated but current classes, update references to Moskitos argument matchers), can be cherry picked.WaitAllStrategy
: It now pushes down it's own timeout to all children. I have added then additional tests respectively removed the own using a mock.