On Wed, 28 May 2025 11:26:17 GMT, p-nima <d...@openjdk.org> wrote:
> The AuthenticationFilter did not respect the default retry limit of 3 retries
> in case of invalid credentials supplied.
>
> This PR helps to resolve the bug and tests it with default and updated retry
> limit set via `jdk.httpclient.auth.retrylimit=1`.
>
> The test is green with tiers 1, 2, 3 and the test is stable.
test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 34:
> 32: * @build jdk.httpclient.test.lib.http2.Http2TestServer
> 33: * @run junit HttpClientRetryLimitTest
> 34: * @run junit/othervm -Djdk.httpclient.auth.retrylimit=1
> HttpClientRetryLimitTest
Do you think adding a retrylimit=0 would be beneficial too? This way every
scenario would be covered
test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 47:
> 45: import java.net.http.HttpResponse;
> 46:
> 47: import static jdk.test.lib.Asserts.assertEquals;
minor: Did you mean `import static
org.junit.jupiter.api.Assertions.assertEquals;` here, as you're using junit?
test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 52:
> 50:
> 51: private static final int DEFAULT_RETRY_LIMIT = 3;
> 52: private final int retryLimit =
> Integer.getInteger("jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);
Nit: This feels a bit confusing to me when reading it the first time, why not
have a retry limit of 1 or 0 as a default and then specify if you want more
retries in `@test`?
I think it might be a bit easier to read, but if you want to keep it, it's fine
with me.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111682350
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111670821
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111681002
