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

HttpWaitStrategy.forStatusCodeMatching always expect 200 code #880

Closed
dadoonet opened this issue Sep 24, 2018 · 1 comment · Fixed by #881
Closed

HttpWaitStrategy.forStatusCodeMatching always expect 200 code #880

dadoonet opened this issue Sep 24, 2018 · 1 comment · Fixed by #881
Labels

Comments

@dadoonet
Copy link
Contributor

I did a mistake while implementing #630.

We have by default this predicate:

    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);
    };

Then we test the status code using:

    public HttpWaitStrategy forStatusCodeMatching(Predicate<Integer> statusCodePredicate) {
        this.statusCodePredicate = this.statusCodePredicate.or(statusCodePredicate);
        return this;
    }

This means that code 200 is always considered as a "good" status code.
Where I'd expect the developper to provide it explicitly.

I mean that:

new HttpWaitStrategy().forStatusCodeMatching(response -> response == HTTP_UNAUTHORIZED);

Should only match HTTP_UNAUTHORIZED and not HTTP_OK.

I'll come with a fix.

@kiview
Copy link
Member

kiview commented Sep 24, 2018

Good catch, I assume you encountered it while using it? 😉

dadoonet referenced this issue in dadoonet/testcontainers-java Sep 24, 2018
….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.
rnorth pushed a commit that referenced this issue Sep 25, 2018
….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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants