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

Adds pluggable wait strategy, and HTTP(S) implementation. #133

Merged
merged 1 commit into from
Apr 28, 2016

Conversation

outofcoffee
Copy link
Contributor

@outofcoffee outofcoffee commented Apr 18, 2016

Purpose

This PR adds a pluggable wait strategy, to enable containers to be checked for readiness using arbitrary logic.

It also adds an HTTP(S) wait strategy to wait for a particular endpoint to be available.

Implementation

  • Refactored existing waitForListeningPort(...) logic into its own class.
  • Adds new HTTP(S) wait implementation

Examples

Wait for 200 OK:

@ClassRule
public static GenericContainer elasticsearch =
    new GenericContainer("elasticsearch:2.3")
               .withExposedPorts(9200)
               .waitingFor(Wait.forHttp("/all"));

Wait for arbitrary status code on an HTTPS endpoint:

@ClassRule
public static GenericContainer elasticsearch =
    new GenericContainer("elasticsearch:2.3")
               .withExposedPorts(9200)
               .waitingFor(Wait.forHttp("/all")
               .forStatusCode("301)
               .usingTls());

@outofcoffee outofcoffee force-pushed the feature/wait-strategies branch 3 times, most recently from 636f525 to 6d257cd Compare April 18, 2016 19:57
/**
* The approach to determine if the container is ready.
*/
protected WaitStrategy waitStrategy;
Copy link
Member

Choose a reason for hiding this comment

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

Set to Wait.defaultWaitStrategy() and mark @ NonNull on the field and withWaitStrategy parameter? Then no need for a null check at time of use...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to avoid additional assignment but happy to change if you feel strongly :)

@rnorth
Copy link
Member

rnorth commented Apr 18, 2016

Thanks for this! Just a few comments so far, but looking good!
Please can you add some tests some time ;) ?

BTW the CI builds have both failed on the Virtuoso container module - it's possibly unrelated but I'm looking into it...

@outofcoffee
Copy link
Contributor Author

@rnorth the tests failed because I rebased after pushing ;-) Have switched to pushing additional commits now to track comments above.

@rnorth
Copy link
Member

rnorth commented Apr 18, 2016

Aha! No worries then :)

On 18 Apr 2016, at 21:16, Pete Cornish [email protected] wrote:

@rnorth https://github.com/rnorth the tests failed because I rebased after pushing ;-) Have switched to pushing additional commits now to track comments above.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #133 (comment)

*/
protected void waitUntilContainerStarted() {
getWaitStrategy()
.forContainer(this)
Copy link
Member

Choose a reason for hiding this comment

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

.forContainer() should return new instance or should be removed in favour of an argument in waitUntilReady and stateless strategies.

Motivation - current implementation creates a false positive feeling that we can re-use existing strategy for different containers, but we can't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could fix this by moving the container assignment to the constructor or, alternatively, throwing an IllegalStateException on calling forContainer more than once.

@rnorth thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

constructor sounds fine for me 👍

@outofcoffee
Copy link
Contributor Author

Ah, no, on second thought that would break chaining, as the container ref isn't initialised yet when declaring the class rule field. Might just make the forContainer method private and remove it from the interface. The call from within GenericContainer will still work because AbstractWaitStrategy is an inner class.

@rnorth
Copy link
Member

rnorth commented Apr 24, 2016

FWIW I'll accept this PR with only Travis CI passing, as the CircleCI builds are not receiving the patch on master to disable Virtuoso build. FYI @outofcoffee, when you're ready.

@outofcoffee outofcoffee force-pushed the feature/wait-strategies branch 2 times, most recently from 78fab09 to 7640781 Compare April 28, 2016 14:55
@outofcoffee
Copy link
Contributor Author

Tests failing because of #140

@outofcoffee
Copy link
Contributor Author

@rnorth as discussed; Travis build is passing, but Circle still having issues.

…readiness using arbitrary logic. Adds HTTP(S) wait strategy to wait for a particular endpoint to be available.
@rnorth
Copy link
Member

rnorth commented Apr 28, 2016

Looks good to me - will merge.

@rnorth rnorth merged commit 5bafbe3 into testcontainers:master Apr 28, 2016
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.

3 participants