RFR: 8295944: Move the Http2TestServer and related classes into a package of its own [v2]
Daniel Fuchs
dfuchs at openjdk.org
Tue Jan 24 12:41:28 UTC 2023
On Tue, 24 Jan 2023 09:59:38 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:
>> Can I please get a review of this change which addresses https://bugs.openjdk.org/browse/JDK-8295944?
>>
>> As noted in the issue, the HttpClient testing helper infrastructure code resides in a default (unnamed) packages within the test hierarchy. The commit in this PR moves those classes into a specific package. The tests have been updated to refer to them correctly with proper package name references.
>> Additionally as noted in the issue, the `@modules` declaration used in the httpclient tests, were all the same and were repeated in all these tests. The commit in this PR moves those module references into the `TEST.properties` to centralize such reference. Any test which needs additional modules can then use specific `@modules` declaration in the test definition.
>>
>> No functional changes have been done in this PR.
>>
>> tier1, tier2 and tier3 testing passed after these changes.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>
> - merge latest from master branch
> - fix SANTest which was missed out from the refactoring
> - move out the @modules declaration from test definition, to TEST.properties
> - 8295944: Move the Http2TestServer and related classes into a package of its own
Very nice refactoring overall. Thanks for taking that on! There are a few places where some lines should have been removed but were not. I made some suggestions. Also there are 9 classes that have kept their custom `@modules` clause because they want to access
* java.base/sun.net.www
* java.base/sun.net
```
I wonder if we should just add these two in TEST.properties - and then remove the `@modules` from these nine classes as well?
test/jdk/java/net/httpclient/BasicRedirectTest.java line 28:
> 26: * @summary Basic test for redirect and redirect policies
> 27: * java.logging
> 28: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/ConcurrentResponses.java line 30:
> 28: * unprocessed HTTP data
> 29: * java.logging
> 30: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/CookieHeaderTest.java line 29:
> 27: * @summary Test for multiple vs single cookie header for HTTP/2 vs HTTP/1.1
> 28: * java.logging
> 29: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/CustomRequestPublisher.java line 28:
> 26: * @summary Checks correct handling of Publishers that call onComplete without demand
> 27: * java.logging
> 28: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/ExecutorShutdown.java line 30:
> 28: * new tasks while the client is still running
> 29: * java.logging
> 30: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/FilePublisher/FilePublisherPermsTest.java line 32:
> 30: * policy 2: custom permission for test classes
> 31: * policy 3: custom permission for test classes and httpclient
> 32: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/FilePublisher/FilePublisherTest.java line 29:
> 27: * @summary Confirm that HttpRequest.BodyPublishers#ofFile(Path)
> 28: * assumes the default file system
> 29: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/FlowAdapterPublisherTest.java line 69:
> 67: * @summary Basic tests for Flow adapter Publishers
> 68: * java.logging
> 69: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/FlowAdapterSubscriberTest.java line 70:
> 68: * @summary Basic tests for Flow adapter Subscribers
> 69: * java.logging
> 70: * jdk.httpserver
Shouldn't these two lines be removed as well?
Suggestion:
test/jdk/java/net/httpclient/HeadTest.java line 29:
> 27: * @summary (httpclient) Add tests for HEAD and 304 responses.
> 28: * java.logging
> 29: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/MaxStreams.java line 30:
> 28: *
> 29: * java.logging
> 30: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/PathSubscriber/BodyHandlerOfFileDownloadTest.java line 29:
> 27: * @summary Confirm HttpResponse.BodySubscribers#ofFileDownload(Path)
> 28: * works only with the default file system
> 29: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/PathSubscriber/BodyHandlerOfFileTest.java line 30:
> 28: * works with default and non-default file systems
> 29: * when SecurityManager is enabled
> 30: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/PathSubscriber/BodySubscriberOfFileTest.java line 30:
> 28: * works with default and non-default file systems
> 29: * when SecurityManager is enabled
> 30: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/RedirectMethodChange.java line 27:
> 25: * @test
> 26: * @summary Method change during redirection
> 27: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/RedirectWithCookie.java line 28:
> 26: * @summary Test for cookie handling when redirecting
> 27: * java.logging
> 28: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/Response1xxTest.java line 54:
> 52: * @summary Tests behaviour of HttpClient when server responds with 102 or 103 status codes
> 53: * java.logging
> 54: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/RetryWithCookie.java line 29:
> 27: * @summary Test for cookie handling when retrying after close
> 28: * java.logging
> 29: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/StreamingBody.java line 29:
> 27: * strong (or any ) reference to the client.
> 28: * java.logging
> 29: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/UnauthorizedTest.java line 33:
> 31: * should simply let the caller deal with the unauthorized response.
> 32: * java.logging
> 33: * jdk.httpserver
Suggestion:
test/jdk/java/net/httpclient/UserCookieTest.java line 30:
> 28: * server-cookies for HTTP/2 vs HTTP/1.1
> 29: * java.logging
> 30: * jdk.httpserver
Suggestion:
-------------
Changes requested by dfuchs (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12160
More information about the net-dev
mailing list