Support multiple HTTP status codes for HttpWaitStrategy #630

dadoonet · 2018-04-01T13:48:18Z

In the context of elasticsearch test containers module, I'd like to
add an ElasticsearchWaitStrategy class which extends the HttpWaitStrategy
with some default settings so it will be even easier for a end user to
start an ElasticsearchTestContainer.

Anyway, in this context, I found helpful that the HttpWaitStrategy expects
more than on status code.

For example, you can imagine running elasticsearch in secure mode or
without any security. In which cases the elasticsearch service might answer 200 or 401.

This commit proposes this change.

In the context of elasticsearch test containers module, I'd like to add an ElasticsearchWaitStrategy class which extends the HttpWaitStrategy with some default settings so it will be even easier for a end user to start an ElasticsearchTestContainer. Anyway, in this context, I found helpful that the HttpWaitStrategy expects more than on status code. For example, you can imagine running elasticsearch in secure mode or without any security. In which cases the elasticsearch service might answer 200 or 401. This commit proposes this change.

kiview · 2018-04-01T18:19:21Z

core/src/main/java/org/testcontainers/containers/wait/HttpWaitStrategy.java

     */
+    @Deprecated


Do you really think we should deprecate the old API? From a technical standpoint you are of course right, I'm just thinking about the developer experience, with a lot of people using this API right now I assume.

I would keep this method and add one more with ranges. ES' case (two values, 200 and 401) is not as popular as e.g. ranges like 200-299 where any 2xx code is considered valid.

Or, to keep this PR small, we can allow calling forStatusCode multiple times:

Wait.forHttp("/all").forStatusCode(200).forStatusCode(401)`

If it works for the ES module, I'd be fine with calling the method multiple times.

@bsideup

ES' case (two values, 200 and 401) is not as popular as e.g. ranges like 200-299 where any 2xx code is considered valid.

Yeah. I thought about that as well as I saw how fabric8 docker maven plugin offers that as well. But my plan was to come with this later may be with something like forStatusCodes(String...).

Wait.forHttp("/all").forStatusCode(200).forStatusCode(401)

Yeah. I agree with this proposal. And I can then support as well:

Wait.forHttp("/all").forStatusCode(200, 299).forStatusCode(401)

WDYT?

forStatusCode(200, 299) is too unambiguous :(

I would actually go for

Wait.forHttp("/all").forStatusCodeMatching(it -> it >= 200 && it < 300 || it == 401)

And also keep forStatusCode, plus maybe make it chain-able ( Wait.forHttp("/all").forStatusCode(200).forStatusCode(401) )

Awesome. Sounds like great ideas. I'll do that

dadoonet · 2018-04-01T21:14:07Z

I don't know TBH. I could also have only change the existing signature.

Up to you. I can update the PR if you wish but having 2 methods doing the same does not look appealing to me.

barrycommins · 2018-04-02T09:24:12Z

Sorry to butt in here, but this class has never been released, so maybe there's no need to deprecate the method at all?

This class (and package) was created as part of #600, and the previous implementation just wraps it.

kiview · 2018-04-02T10:34:05Z

Thx for the comment @barrycommins. If this isn't released yet, WDYT about changing this internal API for now to allow for multiargs by default?

barrycommins · 2018-04-02T11:27:49Z

@kiview I see no harm in it, but I think if it were up to me, I'd keep the original method name forStatusCode and just update it to support varargs.

I think that allows the easiest upgrade path for current users of HttpWaitStrategy, if they want to move to the new class. They would just have to change the package name and the method would still work.

For users changing from a custom subclass, they would need to change the method signature to support varargs, but since they were changing the superclass anyway I don't think it's too big a problem.

Of course, this is contingent on this PR being merged before the next release, because otherwise it would become a breaking change :-)

dadoonet · 2018-04-02T11:53:43Z

Makes sense to me. I wanted initially to add the plural form to make obvious that you can many status codes but I'm fine keeping the singular form.

I'll update the PR that way.

barrycommins · 2018-04-02T12:16:07Z

@dadoonet Just a suggestion, I'm not a maintainer, so hopefully @kiview has an opinion on this.

kiview · 2018-04-02T13:00:07Z

The plural is of course much nicer regarding API readability and discoverability. I think I even would be fine with having 2 public methods, with the singular one delegating to the varargs variant.

I'm sure @bsideup or @rnorth have a stronger opinion on the API design 😅

`forStatusCodeMatching()` comes with a default Predicate which checks status codes that have been provided with `forStatusCode()`. Also copied the default tests which were using the deprecated package to the new one to make sure we test the new methods.

bsideup · 2018-04-07T15:28:18Z

core/src/test/java/org/testcontainers/junit/wait/strategy/AbstractWaitStrategyTest.java

@@ -0,0 +1,93 @@
+package org.testcontainers.junit.wait.strategy;


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

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

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

@dadoonet ping :)

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

But I added a call to my method

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

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

Yes, thanks :)

bsideup · 2018-04-07T16:37:31Z

core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java

+     * @param statusCodePredicate The predicate to test the response against
+     * @return this
+     */
+    public HttpWaitStrategy forStatusCodeMatching(Predicate<Integer> statusCodePredicate) {


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?

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...

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

Also use a Set instead of a List to avoid duplicates.

bsideup · 2018-04-07T17:18:54Z

core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java

+     * @return this
+     */
+    public HttpWaitStrategy forStatusCodeMatching(Predicate<Integer> statusCodePredicate) {
+        this.statusCodePredicate.and(statusCodePredicate);


Should be:

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

I was unsure and thought that we can directly modify the existing predicate and chain it to a new one without changing its reference. Let me update that.

bsideup · 2018-04-07T17:36:37Z

modules/vault/src/test/java/org/testcontainers/vault/VaultContainerTest.java

@@ -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;


unrelated to this PR

…ough)

dadoonet · 2018-04-08T05:12:15Z

The travis failure seems unrelated to my changes.

And stop testing deprecated methods

dadoonet · 2018-04-13T09:05:45Z

I believe it's ok now.
Please LMK if I should squash it and rebase.

bsideup · 2018-04-13T09:30:21Z

core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java

@@ -59,7 +59,9 @@ protected void waitUntilReady() {
                super.waitUntilReady();
                ready.set(true);
            }
-        }.forResponsePredicate(s -> s.equals(GOOD_RESPONSE_BODY));
+        }
+        .forResponsePredicate(s -> s.equals(GOOD_RESPONSE_BODY))


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

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...)

But we're still missing a test where we call

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

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

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

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

bsideup · 2018-04-13T11:25:32Z

@dadoonet ok, looks good now! Just the last thing before we can merge - CHANGELOG.md :)
Sorry for not pointing at it before, I keep forgetting it :(

P.S. we squash automatically when we merge, no need to squash it yourself 👍

dadoonet · 2018-04-13T12:04:45Z

No problem.

bsideup · 2018-04-13T12:05:15Z

@dadoonet merged, thanks a lot! Will be released this weeks :)

dadoonet · 2018-04-13T12:05:47Z

Awesome! Thanks for guiding me for my 1st code PR :)

….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.

….forStatusCode (#881) In #630 we introduced predicates but with a default one which is always present whatever 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.

dadoonet requested review from bsideup, kiview and rnorth as code owners April 1, 2018 13:48

dadoonet mentioned this pull request Apr 1, 2018

Provide an OOTB ElasticsearchWaitStrategy dadoonet/testcontainers-java-module-elasticsearch#21

Closed

kiview reviewed Apr 1, 2018

View reviewed changes

bsideup added the type/feature label Apr 4, 2018

dadoonet added 5 commits April 6, 2018 16:27

Revert changes in Deprecated class

da44bd9

Merge branch 'master' into pr/http-multiple-ports

0af2eab

Revert changes in Deprecated class

e5ddd16

Fix quality

f2d1592

bsideup reviewed Apr 7, 2018

View reviewed changes

Fix compile issue

bf12100

bsideup added this to the next milestone Apr 7, 2018

Support both Predicates and Status codes

3b34909

Also use a Set instead of a List to avoid duplicates.

bsideup reviewed Apr 7, 2018

View reviewed changes

Fix setter

afe06dc

bsideup reviewed Apr 7, 2018

View reviewed changes

dadoonet added 3 commits April 8, 2018 06:30

Revert unrelated change

9f1b28a

Merge branch 'master' into pr/http-multiple-ports

cb4875d

Fix error message (note that with predicates it might be incorrect th…

bb39373

…ough)

Fix default predicate

10ca9cd

Move WaitStrategy tests to the right package

370c35a

And stop testing deprecated methods

bsideup reviewed Apr 13, 2018

View reviewed changes

dadoonet added 2 commits April 13, 2018 11:46

Add a test (and fix the bug!)

808dcf8

Add more tests

d6837f7

Add entry in changelog

6b24c2a

bsideup merged commit 1d85a38 into testcontainers:master Apr 13, 2018

dadoonet deleted the pr/http-multiple-ports branch April 13, 2018 12:05

dadoonet mentioned this pull request Sep 24, 2018

HttpWaitStrategy.forStatusCodeMatching always expect 200 code #880

Closed

dadoonet mentioned this pull request Sep 24, 2018

Fix HttpWaitStrategy.forStatusCodeMatching used with HttpWaitStrategy.forStatusCode #881

Merged

		@@ -0,0 +1,93 @@
		package org.testcontainers.junit.wait.strategy;

Support multiple HTTP status codes for HttpWaitStrategy #630

Support multiple HTTP status codes for HttpWaitStrategy #630

Conversation

dadoonet commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Apr 1, 2018

barrycommins commented Apr 2, 2018

kiview commented Apr 2, 2018

barrycommins commented Apr 2, 2018 • edited

dadoonet commented Apr 2, 2018

barrycommins commented Apr 2, 2018

kiview commented Apr 2, 2018

bsideup Apr 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Apr 8, 2018

dadoonet commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup commented Apr 13, 2018

dadoonet commented Apr 13, 2018

bsideup commented Apr 13, 2018

dadoonet commented Apr 13, 2018

barrycommins commented Apr 2, 2018 •

edited

bsideup Apr 7, 2018 •

edited