On Tue, 26 May 2026 07:31:03 GMT, Jaikiran Pai <[email protected]> wrote:
>> Ashay Rane has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address PR comments
>>
>> 1. Use 'https' scheme with `.loopback()`, similar to other tests.
>>
>> 2. Drop reflection to access the connection and the cached header
>> buffer. Instead add accessor methods to HttpClientImplAccess that
>> fetch these members and use them in the tests.
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java
> line 355:
>
>> 353: }
>> 354:
>> 355: Map<String, Http2Connection> getConnections() {
>
> Same comment as the other one, I think we should avoid these new methods
> unless it is absolutely necessary. We do introduce package private methods
> when the production source itself needs such usage. Right now I don't see any
> such usage except in the test, so it would be good to leave this out.
It's OK to have package protected methods that are only used in tests but they
should be identified as such with a comment. It's good practice however to try
to minimize the exposure to what the test really needs, and also minimize the
number of such methods. The alternative is to use MethodHandles / VarHandles
from within the injected accessor class, with the caveat that the test will no
longer detect at compile time that the source has changed - and test failures
could be harder to diagnose.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3303396746
