On Thu, 25 Apr 2024 12:28:25 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> robert engels has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix broken test cases
>
> test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 60:
>
>> 58: InetAddress loopback = InetAddress.getLoopbackAddress();
>> 59: InetSocketAddress addr = new InetSocketAddress (loopback, 0);
>> 60: HttpServer server = HttpServer.create (addr, 0);
>
> Given the change we are doing, I think it would be good to even include a
> test (within this same test class) which tests the `HttpsServer`.
done
> test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 79:
>
>> 77: var uri =
>> URIBuilder.newBuilder().scheme("http").loopback().port(server.getAddress().getPort()).path("/chunked").build();
>> 78: var response =
>> client.send(HttpRequest.newBuilder(uri).build(),
>> HttpResponse.BodyHandlers.ofString());
>> 79: if(!response.body().equals("hello")) throw new
>> IllegalStateException("incorrect body");
>
> Same applies here, it would be better to include the failing response body in
> the exception message.
done
> test/jdk/com/sun/net/httpserver/bugs/TcpNoDelayNotRequired.java line 112:
>
>> 110: is.close();
>> 111: rmap.add("content-type","text/plain");
>> 112: t.sendResponseHeaders(200,0);
>
> The value 0 and -1 have been a constant confusion in our tests when calling
> `sendResponseHeaders`. Can you update this line to:
>
>
> t.sendResponseHeaders(200, 0 /* chunked */);
>
> so that it's immediately obvious what the intent here is.
we can fix the test cases with constants after the api change
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579699401
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579698757
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1579700904
