Testcontainers #589
Testcontainers #589
Conversation
...ient5-testing/src/test/java/org/apache/hc/client5/testing/compatibility/ContainerImages.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hc/client5/testing/compatibility/async/HttpAsyncClientCompatibilityTest.java
Show resolved
Hide resolved
| EntityUtils.consume(response.getEntity()); | ||
| } | ||
|
|
||
| Thread.sleep(2000); |
There was a problem hiding this comment.
What's this waiting for? Is there something we could check using something like awaitility instead of hardcoding a sleep which can lead to unpredictable test behaviour.
There was a problem hiding this comment.
What's this waiting for? Is there something we could check using something like
awaitilityinstead of hardcoding asleepwhich can lead to unpredictable test behaviour.
@massdosage It is waiting for a cache entry to become stale and trigger its re-validation. I am not sure this can be done without a sleep given this is an integration test. I can however try to reduce it.
I must admit I do not know what awaitility is and whether it can be applied here.
There was a problem hiding this comment.
It's https://github.com/awaitility/awaitility, so in this case you could do something like (pseudo code):
await().atMost(2, SECONDS).until(cacheIsStale())
It's nice for turning these kind of unpredictable sleep calls into something that effectively is "keep trying for something to happen, and if it doesn't happen within some timeout, then fail the test".
I see that this sleep was copied over from a previous version of this test so I also understand if you'd rather keep the behaviour like this for now and push out the refactoring the sleep for another day if this hasn't caused any issues with flaky tests up to now.
There was a problem hiding this comment.
@massdosage Have mercy on me.I just discovered TestContainers no so long ago and one throws awaitility at me on top of it.
Seriously our test cases are full of CountDownLatch based synchronization code and other ugliness. I would prefer to take a look at awaitility as a way of improving our async testing code overall and not just to replace an occasional Thread#sleep
There was a problem hiding this comment.
Agreed, sounds better to take the approach where you fix more things at once than just this one :) So just leave this "as is" for now. If you want to create an issue summarising where you think something like awaitility could improve the tests I might be able to take a look at it.
I'm a big fan of TestContainers, have used it successfully on a number of projects to bring the "Docker Tests" alongside the rest of the Java tests so I'm happy to see it being used here. One just has to be careful to limit the number of these tests as they are slowly and more resource hungry.
There was a problem hiding this comment.
@ok2c The changes look reasonable to me, even though I don't have prior experience with TestContainers.
@arturobernalg Honestly, nether do it. It is a learning process for me as well. But it is good to have an extra pair of eye balls looking at a big change to the project. |
Apache HTTPD / Squid compatibility tests have been migrated to TestContainers.
This also ensures that more complex authentication scenarios that are difficult to create with our own integration test framework (various scenarios involving a proxy) will now be run along with the rest of integration tests on a regular basis.
This will especially be important for #577 and this would also provide an opportunity to create integration test cases for Kerberos auth in the future.
@arturobernalg Please take a look.