Skip to content

Commit

Permalink
Fix HttpWaitStrategy.forStatusCodeMatching used with HttpWaitStrategy…
Browse files Browse the repository at this point in the history
….forStatusCode

In #630 we introduced predicates but with a default one which is always present whatever what is passed in the `forStatusCodeMatching()` method.

This commit adds a test that demonstrates the issue:

* We have a service returning `200 OK`
* The predicate expects anything which is a code >= to `300`
* The test should throw a Timeout as this condition is never reached but without the current fix, the test never throws the Timeout as 200 matches the default builtin predicate.

This commit fixes the problem by checking at startup time what is/are the predicates that needs to be applied.

Note that in most cases, an HTTP service is expected to throw a `200 OK` status so that fix might not fix actually any real problem and might be a theory only. But I'd prefer to have code that actually implements what is supposed to work.

Closes #880.
  • Loading branch information
dadoonet committed Sep 24, 2018
1 parent 007c3b9 commit e8ed77c
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,7 @@ public class HttpWaitStrategy extends AbstractWaitStrategy {
private String username;
private String password;
private Predicate<String> responsePredicate;
private Predicate<Integer> statusCodePredicate = responseCode -> {
// If we did not provide any status code, we assume by default HttpURLConnection.HTTP_OK
if (statusCodes.isEmpty() && HttpURLConnection.HTTP_OK == responseCode) return true;
return statusCodes.contains(responseCode);
};
private Predicate<Integer> statusCodePredicate = null;
private Optional<Integer> livenessPort = Optional.empty();

/**
Expand All @@ -63,7 +59,7 @@ public HttpWaitStrategy forStatusCode(int statusCode) {
* @return this
*/
public HttpWaitStrategy forStatusCodeMatching(Predicate<Integer> statusCodePredicate) {
this.statusCodePredicate = this.statusCodePredicate.or(statusCodePredicate);
this.statusCodePredicate = statusCodePredicate;
return this;
}

Expand Down Expand Up @@ -159,7 +155,22 @@ protected void waitUntilReady() {

log.trace("Get response code {}", connection.getResponseCode());

if (!statusCodePredicate.test(connection.getResponseCode())) {
// Choose the statusCodePredicate strategy depending on what we defined.
Predicate<Integer> predicate;
if (statusCodes.isEmpty() && statusCodePredicate == null) {
// We have no status code and no predicate so we expect a 200 OK response code
predicate = responseCode -> HttpURLConnection.HTTP_OK == responseCode;
} else if (!statusCodes.isEmpty() && statusCodePredicate == null) {
// We use the default status predicate checker when we only have status codes
predicate = responseCode -> statusCodes.contains(responseCode);
} else if (statusCodes.isEmpty()) {
// We only have a predicate
predicate = statusCodePredicate;
} else {
// We have both predicate and status code
predicate = statusCodePredicate.or(responseCode -> statusCodes.contains(responseCode));
}
if (!predicate.test(connection.getResponseCode())) {
throw new RuntimeException(String.format("HTTP response code was: %s",
connection.getResponseCode()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,21 @@ public void testWaitUntilReadyWithTimeoutAndWithManyStatusCodesAndLambda() {
));
}

/**
* Expects that the WaitStrategy throws a {@link RetryCountExceededException} after not receiving any of the
* error code defined with {@link HttpWaitStrategy#forStatusCode(int)}
* and {@link HttpWaitStrategy#forStatusCodeMatching(Predicate)}. Note that a 200 status code should not
* be considered as a successful return as not explicitly set.
* Test case for: https://github.com/testcontainers/testcontainers-java/issues/880
*/
@Test
public void testWaitUntilReadyWithTimeoutAndWithLambdaShouldNotMatchOk() {
waitUntilReadyAndTimeout(startContainerWithCommand(createShellCommand("200 OK", GOOD_RESPONSE_BODY),
createHttpWaitStrategy(ready)
.forStatusCodeMatching(it -> it >= 300)
));
}

/**
* Expects that the WaitStrategy throws a {@link RetryCountExceededException} after not receiving an HTTP 200
* response from the container within the timeout period.
Expand Down

0 comments on commit e8ed77c

Please sign in to comment.