8295944: Move the Http2TestServer and related classes into a package of its own #12160

jaikiran · 2023-01-24T06:05:30Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8295944: Move the Http2TestServer and related classes into a package of its own

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12160/head:pull/12160
$ git checkout pull/12160

Update a local copy of the PR:
$ git checkout pull/12160
$ git pull https://git.openjdk.org/jdk pull/12160/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12160

View PR using the GUI difftool:
$ git pr show -t 12160

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12160.diff

…of its own

…rties

bridgekeeper · 2023-01-24T06:07:48Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-24T06:10:13Z

@jaikiran The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-24T06:16:27Z

Webrevs

dfuch

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?

dfuch · 2023-01-24T12:03:53Z

test/jdk/java/net/httpclient/FlowAdapterSubscriberTest.java

 *          java.logging
 *          jdk.httpserver


Shouldn't these two lines be removed as well?

Suggested change

* java.logging

* jdk.httpserver

dfuch · 2023-01-24T12:04:44Z

test/jdk/java/net/httpclient/FlowAdapterPublisherTest.java

 *          java.logging
 *          jdk.httpserver


Suggested change

* java.logging

* jdk.httpserver

dfuch · 2023-01-24T12:05:33Z

test/jdk/java/net/httpclient/ExecutorShutdown.java

 *          java.logging
 *          jdk.httpserver


Suggested change

* java.logging

* jdk.httpserver

dfuch · 2023-01-24T12:06:16Z

test/jdk/java/net/httpclient/CustomRequestPublisher.java

 *          java.logging
 *          jdk.httpserver


Suggested change

* java.logging

* jdk.httpserver

dfuch · 2023-01-24T12:06:44Z

test/jdk/java/net/httpclient/CookieHeaderTest.java

 *          java.logging
 *          jdk.httpserver


Suggested change

* java.logging

* jdk.httpserver

dfuch · 2023-01-24T12:24:28Z

test/jdk/java/net/httpclient/FilePublisher/FilePublisherTest.java

- * @modules java.base/sun.net.www.http
- *          java.net.http/jdk.internal.net.http.common
- *          java.net.http/jdk.internal.net.http.frame
- *          java.net.http/jdk.internal.net.http.hpack
 *          jdk.httpserver


Suggested change

* jdk.httpserver

dfuch · 2023-01-24T12:25:58Z

test/jdk/java/net/httpclient/MaxStreams.java

 *          java.logging
 *          jdk.httpserver


Suggested change

* java.logging

* jdk.httpserver

dfuch · 2023-01-24T12:26:43Z

test/jdk/java/net/httpclient/PathSubscriber/BodyHandlerOfFileDownloadTest.java

- * @modules java.base/sun.net.www.http
- *          java.net.http/jdk.internal.net.http.common
- *          java.net.http/jdk.internal.net.http.frame
- *          java.net.http/jdk.internal.net.http.hpack
 *          jdk.httpserver


Suggested change

* jdk.httpserver

dfuch · 2023-01-24T12:27:06Z

test/jdk/java/net/httpclient/PathSubscriber/BodyHandlerOfFileTest.java

- * @modules java.base/sun.net.www.http
- *          java.net.http/jdk.internal.net.http.common
- *          java.net.http/jdk.internal.net.http.frame
- *          java.net.http/jdk.internal.net.http.hpack
 *          jdk.httpserver


Suggested change

* jdk.httpserver

dfuch · 2023-01-24T12:27:47Z

test/jdk/java/net/httpclient/RedirectMethodChange.java

- * @modules java.base/sun.net.www.http
- *          java.net.http/jdk.internal.net.http.common
- *          java.net.http/jdk.internal.net.http.frame
- *          java.net.http/jdk.internal.net.http.hpack
 *          jdk.httpserver


Suggested change

* jdk.httpserver

jaikiran · 2023-01-25T06:31:45Z

Hello Daniel,

There are a few places where some lines should have been removed but were not. I made some suggestions.

Thank you for catching these. These were genuine issues and were a leftover from my @modules removal. I've fixed these now in the updated PR. I went through all the files again and did a search (with a different search term this time) and I think have now covered all these leftovers.

jaikiran · 2023-01-25T07:05:33Z

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?

I think that's reasonable. When I changed most of these tests to remove @modules from their test definition, my selection of "common" declarations missed out these two declarations. I've now updated the TEST.properties to include these and also updated the relevant tests to remove the @modules declaration.

After this change, there remains few tests which use only a limited subset of the modules delcared in TEST.properties. I have left those tests as-is (for example CancelledResponse.java), since it didn't look necessary to remove those @modules. Let me know if you prefer otherwise and I'll update them as well.

Local testing with the latest state of the PR passed. I've now triggered a broader testing with these changes.

openjdk · 2023-01-25T11:56:29Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8295944: Move the Http2TestServer and related classes into a package of its own

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

a5d8e12: 8300244: Replace NULL with nullptr in share/interpreter/
71107f4: 8300651: Replace NULL with nullptr in share/runtime/
3c61d5a: 8300659: Refactor TestMemoryAwareness to use WhiteBox api for host values
b2071f7: 8300657: Remove null filtering in CLD oop handle area
95fafd0: 8300644: Remove gc/shenandoah/jni/TestStringCriticalWithDedup.java
5a478ef: 8297730: C2: Arraycopy intrinsic throws incorrect exception
7465de4: 6603771: Nimbus L&F: Ctrl+F7 keybinding for Jinternal Frame throws a NPE.
fbe5ab0: 8300830: Remove redundant assertion in src/hotspot/share/runtime/javaCalls.cpp
1339461: 8300981: Build failure on 32-bit platforms after JDK-8281213
81d523d: Merge
... and 7 more: https://git.openjdk.org/jdk/compare/544c16e0bdd4335b2624158fd1f6521984aa5079...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jaikiran · 2023-01-25T13:35:02Z

Thank you Daniel for the review. tier1, tier2, tier3 testing came back successful.

jaikiran · 2023-01-25T13:36:14Z

/integrate

openjdk · 2023-01-25T13:38:10Z

Going to push as commit 8a47429.
Since your change was applied there have been 19 commits pushed to the master branch:

c8ad600: 8301004: httpclient: Add more debug to HttpResponseInputStream
74e1a8b: 8300236: Use VarHandle access in Data(Input | Output)Stream classes
a5d8e12: 8300244: Replace NULL with nullptr in share/interpreter/
71107f4: 8300651: Replace NULL with nullptr in share/runtime/
3c61d5a: 8300659: Refactor TestMemoryAwareness to use WhiteBox api for host values
b2071f7: 8300657: Remove null filtering in CLD oop handle area
95fafd0: 8300644: Remove gc/shenandoah/jni/TestStringCriticalWithDedup.java
5a478ef: 8297730: C2: Arraycopy intrinsic throws incorrect exception
7465de4: 6603771: Nimbus L&F: Ctrl+F7 keybinding for Jinternal Frame throws a NPE.
fbe5ab0: 8300830: Remove redundant assertion in src/hotspot/share/runtime/javaCalls.cpp
... and 9 more: https://git.openjdk.org/jdk/compare/544c16e0bdd4335b2624158fd1f6521984aa5079...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-25T13:38:28Z

@jaikiran Pushed as commit 8a47429.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jaikiran added 3 commits January 24, 2023 11:02

8295944: Move the Http2TestServer and related classes into a package …

1c67682

…of its own

move out the @modules declaration from test definition, to TEST.prope…

043d77d

…rties

fix SANTest which was missed out from the refactoring

aae2c1f

openjdk bot added the rfr Pull request is ready for review label Jan 24, 2023

openjdk bot added the net net-dev@openjdk.org label Jan 24, 2023

merge latest from master branch

97cbdc2

dfuch suggested changes Jan 24, 2023

View reviewed changes

jaikiran added 2 commits January 25, 2023 11:28

remove left over content from the @modules that were removed

8cc2721

be consistent and use the full path of the library

ec0a688

review suggestion - remove more common usages of @modules

7cee3fd

dfuch approved these changes Jan 25, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 25, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 25, 2023

openjdk bot closed this Jan 25, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 25, 2023

jaikiran deleted the 8295944 branch January 25, 2023 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295944: Move the Http2TestServer and related classes into a package of its own #12160

8295944: Move the Http2TestServer and related classes into a package of its own #12160

jaikiran commented Jan 24, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 24, 2023

openjdk bot commented Jan 24, 2023

mlbridge bot commented Jan 24, 2023 •

edited

Loading

dfuch left a comment

dfuch Jan 24, 2023 •

edited

Loading

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

dfuch Jan 24, 2023

jaikiran commented Jan 25, 2023

jaikiran commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

jaikiran commented Jan 25, 2023

jaikiran commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

8295944: Move the Http2TestServer and related classes into a package of its own #12160

8295944: Move the Http2TestServer and related classes into a package of its own #12160

Conversation

jaikiran commented Jan 24, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 24, 2023

openjdk bot commented Jan 24, 2023

mlbridge bot commented Jan 24, 2023 • edited Loading

Webrevs

dfuch left a comment

Choose a reason for hiding this comment

dfuch Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran commented Jan 25, 2023

jaikiran commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

jaikiran commented Jan 25, 2023

jaikiran commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

jaikiran commented Jan 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 24, 2023 •

edited

Loading

dfuch Jan 24, 2023 •

edited

Loading