On Tue, 21 Jan 2025 11:01:00 GMT, Jaikiran Pai <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix `HttpResponse` copyright year
>
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 84:
>
>> 82:
>> 83: static Arguments[] sufficientCapacities() {
>> 84: return capacityArgs(Long.MAX_VALUE, RESPONSE_BODY.length);
>
> If I'm reading this (and the `insufficientCapacities()`) correctly, then it
> appears that we are testing the "edge" capacities. I think including at least
> one more arbitrary capacity value which isn't an "edge" might be good too.
> Something like:
>
> final long randomCapacity = new Random(RESPONSE_BODY.length + 1,
> Long.MAX_VALUE);
>
> Similar idea for `insufficientCapacities()`.
Implemented in e10771317ef57f80d25be58df77eb5c5b0083b0c.
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 148:
>
>> 146:
>> 147: // Issue the request
>> 148: var request = HttpRequest
>
> Are we intentionally not setting the HTTP version (being passed to this
> method) on the request or the client?
I thought it wasn't necessary. Nevertheless, added in
e10771317ef57f80d25be58df77eb5c5b0083b0c.
> test/jdk/java/net/httpclient/HttpResponseLimitingTest.java line 150:
>
>> 148: var request = HttpRequest
>> 149: .newBuilder(requestUri)
>> 150: .timeout(Duration.ofSeconds(5))
>
> Given the context of this test class, I think configuring the request with a
> timeout shouldn't be necessary. In general, we rarely use timeouts (hardcoded
> ones specifically) in our tests because these tests run on a variety of hosts
> (some of which are slow or are running too many tests concurrently) and thus
> there's no right value for the timeout and can lead to intermittent failures.
Removed the timeouts in e10771317ef57f80d25be58df77eb5c5b0083b0c.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924296707
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924296492
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924296394
