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

Support multiple HTTP status codes for HttpWaitStrategy #630

Merged
merged 17 commits into from
Apr 13, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ public static HostPortWaitStrategy forListeningPort() {
*/
public static HttpWaitStrategy forHttp(String path) {
return new HttpWaitStrategy()
.forPath(path)
.forStatusCode(HttpURLConnection.HTTP_OK);
.forPath(path);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.net.HttpURLConnection;
import java.net.URI;
import java.net.URL;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
Expand All @@ -32,11 +33,16 @@ public class HttpWaitStrategy extends AbstractWaitStrategy {
private static final String AUTH_BASIC = "Basic ";

private String path = "/";
private int statusCode = HttpURLConnection.HTTP_OK;
private Set<Integer> statusCodes = new HashSet<>();
private boolean tlsEnabled;
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
return (!statusCodes.isEmpty() || HttpURLConnection.HTTP_OK == responseCode) &&
statusCodes.contains(responseCode);
};

/**
* Waits for the given status code.
Expand All @@ -45,7 +51,17 @@ public class HttpWaitStrategy extends AbstractWaitStrategy {
* @return this
*/
public HttpWaitStrategy forStatusCode(int statusCode) {
this.statusCode = statusCode;
statusCodes.add(statusCode);
return this;
}

/**
* Waits for the status code to pass the given predicate
* @param statusCodePredicate The predicate to test the response against
* @return this
*/
public HttpWaitStrategy forStatusCodeMatching(Predicate<Integer> statusCodePredicate) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking about the use case where somebody will call Wait.forHttp().forStatusCode(200).forStatusCodeMatching(it -> it > 400). Maybe we should throw an error, or handle both statuses?

Copy link
Member

Choose a reason for hiding this comment

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

I do really like the Predicate approach here - it opens the door to having some useful ready-to-use predicates in the future (e.g. anySuccessfulStatusCode() or anyServerErrorStatusCode() etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the latest change below I'm now hopefully supporting both.

this.statusCodePredicate = this.statusCodePredicate.and(statusCodePredicate);
return this;
}

Expand Down Expand Up @@ -122,7 +138,7 @@ protected void waitUntilReady() {
connection.setRequestMethod("GET");
connection.connect();

if (statusCode != connection.getResponseCode()) {
if (!statusCodePredicate.test(connection.getResponseCode())) {
throw new RuntimeException(String.format("HTTP response code was: %s",
connection.getResponseCode()));
}
Expand All @@ -144,7 +160,7 @@ protected void waitUntilReady() {

} catch (TimeoutException e) {
throw new ContainerLaunchException(String.format(
"Timed out waiting for URL to be accessible (%s should return HTTP %s)", uri, statusCode));
"Timed out waiting for URL to be accessible (%s should return HTTP %s)", uri, statusCodes));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ public static HostPortWaitStrategy forListeningPort() {
*/
public static HttpWaitStrategy forHttp(String path) {
return new HttpWaitStrategy()
.forPath(path)
.forStatusCode(HttpURLConnection.HTTP_OK);
.forPath(path);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package org.testcontainers.junit.wait.strategy;
Copy link
Member

@bsideup bsideup Apr 7, 2018

Choose a reason for hiding this comment

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

Hm... I'm not sure these changes are related to the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's somewhat needed to test the new methods I added. See strategy/HttpWaitStrategyTest.java class.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should just start using new implementation (under org.testcontainers.containers.wait.strategy package) in HttpWaitStrategyTest & AbstractWaitStrategyTest since the old classes are just wrappers now

Copy link
Member

Choose a reason for hiding this comment

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

@dadoonet ping :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum. I'm missing something. I probably misunderstood what you are expecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I added a call to my method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I added a call to my method

Copy link
Member

Choose a reason for hiding this comment

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

We should just fix the imports in the old test and start using new wait strategies, so that you can also add your new method to that test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. I'll do that. In which case, I'll also move the tests to the new package, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thanks :)


import org.jetbrains.annotations.NotNull;
import org.junit.Before;
import org.rnorth.ducttape.RetryCountExceededException;
import org.rnorth.visibleassertions.VisibleAssertions;
import org.testcontainers.containers.ContainerLaunchException;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.containers.wait.strategy.WaitStrategy;

import java.time.Duration;
import java.util.concurrent.atomic.AtomicBoolean;

import static org.junit.Assert.assertTrue;

/**
* Common test methods for {@link WaitStrategy} implementations.
*
* @author Pete Cornish {@literal <[email protected]>}
*/
public abstract class AbstractWaitStrategyTest<W extends WaitStrategy> {
private static final long WAIT_TIMEOUT_MILLIS = 3000;
private static final String IMAGE_NAME = "alpine:latest";

/**
* Indicates that the WaitStrategy has completed waiting successfully.
*/
private AtomicBoolean ready;

/**
* Subclasses should return an instance of {@link W} that sets {@code ready} to {@code true},
* if the wait was successful.
*
* @param ready the AtomicBoolean on which to indicate success
* @return WaitStrategy implementation
*/
@NotNull
protected abstract W buildWaitStrategy(final AtomicBoolean ready);

@Before
public void setUp() throws Exception {
ready = new AtomicBoolean(false);
}

/**
* Starts a GenericContainer with the {@link #IMAGE_NAME} image, passing the given {@code shellCommand} as
* a parameter to {@literal sh -c} (the container CMD).
*
* @param shellCommand the shell command to execute
* @return the (unstarted) container
*/
private GenericContainer startContainerWithCommand(String shellCommand) {
final WaitStrategy waitStrategy = buildWaitStrategy(ready)
.withStartupTimeout(Duration.ofMillis(WAIT_TIMEOUT_MILLIS));

// apply WaitStrategy to container
return new GenericContainer(IMAGE_NAME)
.withExposedPorts(8080)
.withCommand("sh", "-c", shellCommand)
.waitingFor(waitStrategy);
}

/**
* Expects that the WaitStrategy returns successfully after connection to a container with a listening port.
*
* @param shellCommand the shell command to execute
*/
protected void waitUntilReadyAndSucceed(String shellCommand) {
final GenericContainer container = startContainerWithCommand(shellCommand);

// start() blocks until successful or timeout
container.start();

assertTrue(String.format("Expected container to be ready after timeout of %sms",
WAIT_TIMEOUT_MILLIS), ready.get());
}

/**
* Expects that the WaitStrategy throws a {@link RetryCountExceededException} after unsuccessful connection
* to a container with a listening port.
*
* @param shellCommand the shell command to execute
*/
protected void waitUntilReadyAndTimeout(String shellCommand) {
final GenericContainer container = startContainerWithCommand(shellCommand);

// start() blocks until successful or timeout
VisibleAssertions.assertThrows("an exception is thrown when timeout occurs (" + WAIT_TIMEOUT_MILLIS + "ms)",
ContainerLaunchException.class,
container::start);

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package org.testcontainers.junit.wait.strategy;

import org.jetbrains.annotations.NotNull;
import org.junit.Test;
import org.rnorth.ducttape.RetryCountExceededException;
import org.testcontainers.containers.wait.strategy.HttpWaitStrategy;

import java.util.concurrent.atomic.AtomicBoolean;

/**
* Tests for {@link HttpWaitStrategy}.
*
* @author Pete Cornish {@literal <[email protected]>}
*/
public class HttpWaitStrategyTest extends AbstractWaitStrategyTest<HttpWaitStrategy> {
/**
* newline sequence indicating end of the HTTP header.
*/
private static final String NEWLINE = "\r\n";

private static final String GOOD_RESPONSE_BODY = "Good Response Body";

/**
* Expects that the WaitStrategy returns successfully after receiving an HTTP 200 response from the container.
*/
@Test
public void testWaitUntilReadyWithSuccess() {
waitUntilReadyAndSucceed(createShellCommand("200 OK", GOOD_RESPONSE_BODY));
}

/**
* Expects that the WaitStrategy throws a {@link RetryCountExceededException} after not receiving an HTTP 200
* response from the container within the timeout period.
*/
@Test
public void testWaitUntilReadyWithTimeout() {
waitUntilReadyAndTimeout(createShellCommand("400 Bad Request", GOOD_RESPONSE_BODY));
}

/**
* Expects that the WaitStrategy throws a {@link RetryCountExceededException} after not the expected response body
* from the container within the timeout period.
*/
@Test
public void testWaitUntilReadyWithTimeoutAndBadResponseBody() {
waitUntilReadyAndTimeout(createShellCommand("200 OK", "Bad Response"));
}

/**
* @param ready the AtomicBoolean on which to indicate success
* @return the WaitStrategy under test
*/
@NotNull
protected HttpWaitStrategy buildWaitStrategy(final AtomicBoolean ready) {
return new HttpWaitStrategy() {
@Override
protected void waitUntilReady() {
// blocks until ready or timeout occurs
super.waitUntilReady();
ready.set(true);
}
}
.forResponsePredicate(s -> s.equals(GOOD_RESPONSE_BODY))
Copy link
Member

Choose a reason for hiding this comment

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

please add a test where both forStatusCodeMatching and withStatusCode are used (to check that it's not being overwritten)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. My code is actually failing! :p
I've been wondering how to add a test and by your question I actually understood how to write. (Was so easy that I feel dumb not having found that before...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

But we're still missing a test where we call

forHttp()
    .forStatusCode(200) // <- 
    .forStatusCode(205) // <- 
   .forStatusCodeMatching(it -> it > 400)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I clone the class or just change the buildWaitStrategy method?

Copy link
Member

Choose a reason for hiding this comment

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

just add a test method which is not based on buildWaitStrategy's result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it. A bit differently though so I can reuse most of the existing code and that could help in the future.
I hope it's ok for you

.forStatusCodeMatching(it -> it >= 200 && it < 300 || it == 401);
}

private String createShellCommand(String header, String responseBody) {
int length = responseBody.getBytes().length;
return "while true; do { echo -e \"HTTP/1.1 "+header+NEWLINE+
"Content-Type: text/html"+NEWLINE+
"Content-Length: "+length +NEWLINE+ "\";"
+" echo \""+responseBody+"\";} | nc -lp 8080; done";
}
}
8 changes: 5 additions & 3 deletions docs/usage/docker_compose.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,15 @@ public static DockerComposeContainer environment =
Wait.forListeningPort().withStartupTimeout(Duration.ofSeconds(30)));
```

Wait for arbitrary status code on an HTTPS endpoint:
Wait for arbitrary status codes on an HTTPS endpoint:
```java
@ClassRule
public static DockerComposeContainer environment =
new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
.withExposedService("elasticsearch_1", ELASTICSEARCH_PORT,
Wait.forHttp("/all")
.forStatusCode(301)
.forStatusCode(200)
.forStatusCode(401)
.usingTls());
```

Expand All @@ -91,7 +92,8 @@ public static DockerComposeContainer environment =
.withExposedService("redis_1", REDIS_PORT, Wait.forListeningPort())
.withExposedService("elasticsearch_1", ELASTICSEARCH_PORT,
Wait.forHttp("/all")
.forStatusCode(301)
.forStatusCode(200)
.forStatusCode(401)
.usingTls());
```

Expand Down
19 changes: 16 additions & 3 deletions docs/usage/options.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,17 +97,30 @@ public static GenericContainer elasticsearch =
.waitingFor(Wait.forHttp("/all"));
```

Wait for arbitrary status code on an HTTPS endpoint:
Wait for 200 or 401 status codes on an HTTPS endpoint:
```java
@ClassRule
public static GenericContainer elasticsearch =
new GenericContainer("elasticsearch:2.3")
.withExposedPorts(9200)
.waitingFor(
Wait.forHttp("/all")
.forStatusCode(301)
.forStatusCode(200)
.forStatusCode(401)
.usingTls());
```
```

Wait for 200...299 or 401 status codes on an HTTPS endpoint:
```java
@ClassRule
public static GenericContainer elasticsearch =
new GenericContainer("elasticsearch:2.3")
.withExposedPorts(9200)
.waitingFor(
Wait.forHttp("/all")
.forStatusCodeMatching(it -> it >= 200 && it < 300 || it == 401)
.usingTls());
```

If the used image supports Docker's [Healthcheck](https://docs.docker.com/engine/reference/builder/#healthcheck) feature, you can directly leverage the `healthy` state of the container as your wait condition:
```java
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import org.junit.ClassRule;
import org.junit.Test;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.containers.wait.Wait;
import org.testcontainers.containers.wait.strategy.Wait;
Copy link
Member

Choose a reason for hiding this comment

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

unrelated to this PR


import java.io.IOException;

Expand Down