8337851: Some tests have name which confuse jtreg #20475

toshiogata · 2024-08-06T05:51:35Z

Rename the tests to prevent unexpected deletion of result files when -retain jtreg option is specified.
Testing: modified tests

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8337851: Some tests have name which confuse jtreg (Bug - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Phil Race (@prrace - Reviewer)
Christian Stein (@sormuras - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20475

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-06T05:52:31Z

👋 Welcome back toshiogata! 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 · 2024-08-06T05:52:38Z

@toshiogata 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:

8337851: Some tests have name which confuse jtreg

Reviewed-by: aivanov, prr, cstein

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 864 new commits pushed to the master branch:

a601cd2: 8342014: RISC-V: ZStoreBarrierStubC2 clobbers rflags
bd62644: 8341924: Improve error message with structurally malformed Code array
a2c7752: 8317356: Fix missing null checks in the ClassFile API
6071346: 8339879: Open some dialog awt tests
a8a8b2d: 8341831: PhaseCFG::insert_anti_dependences asserts with "no loads"
9e262df: 8342002: sun/security/tools/keytool/GenKeyPairSigner.java failed due to missing certificate output
f56a154: 8341881: [REDO] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
ba3774d: 8341637: java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
1581508: 8335091: NMT: VMATree reserve_mapping and commit_mapping APIs need MEMFLAGS while un/-committing API has no MEMFLAGS arg
b20c5c7: 8341967: Unify os::current_file_offset and os::seek_to_file_offset across posix platforms
... and 854 more: https://git.openjdk.org/jdk/compare/20575949612a750a428316635715737183a2d58c...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aivanov-jdk, @prrace, @sormuras) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-08-06T05:53:03Z

@toshiogata The following labels will be automatically applied to this pull request:

client
compiler

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

mlbridge · 2024-08-06T05:55:57Z

Webrevs

magicus · 2024-08-06T16:17:07Z

@jonathan-gibbons Is this a bug in jtreg, or where these files actually improperly named?

jonathan-gibbons · 2024-08-06T17:24:40Z

@jonathan-gibbons Is this a bug in jtreg, or where these files actually improperly named?

Not a bug as such, but maybe a little-known misfeature. jtreg has always had problems with filenames that could cause confusion, and the general sense has generally been, "if it hurts, don't do it!". That being said, our overall library API and infrastructure is way better these days than in times past, and it might be reasonable to file an Enhancement for jtreg to have a utility/check/test to detect these anomalous situations.

In the meantime, renaming the files to avoid the problem is the recommended solution.

alisenchung · 2024-08-14T23:43:14Z

test/langtools/tools/javac/generics/parametricException/ParametricException.java

since you're renaming this file for jtreg testing anyway could you move the jtreg tags down below the imports and expand the wildcard import?

As you mentioned, it's a good time to make a minor improvement, so I modified the test.

I propose not to edit the renamed files.

First of all, the purpose of this PR is to rename tests to workaround a misfeature of jtreg.

Git does not support renames natively, therefore by editing the renamed files you could break Git ability to detect renaming.

Additionally, these tests are not part of client and I am not aware of the practices in Java Compiler team. Expanding wildcard imports should be fine, yet I would rather avoid editing the renamed files as I mentioned above.

Thank you for pointing it out. I was not aware of the git rename issue.
I changed the fix to only rename files.

Thank you for pointing it out. I was not aware of the git rename issue.

Even if it were not for this Git “limitation”, I still think additional changes should be requested by Java Compiler team. I am not aware of their practices.

@alisenchung knows I'm advocating for expanding imports (whenever a test or source file is modified) and for moving jtreg tags below imports closer to the class declaration. At the same time, we haven't reached an agreement about it, although some people seem to support it too.

With my official jtreg hat on, I recommend always placing the jtreg tags immediately after any legal header comments and before any and all of the source code that comprises the test.

With my official jtreg hat on, I recommend always placing the jtreg tags immediately after any legal header comments and before any and all of the source code that comprises the test.

@jonathan-gibbons Are there any disadvantages to placing jtreg tags right before the test class declaration, after all the import statements? It is more convenient when viewing test code in IDE: the jtreg tags are clearly visible. If the tags are after the legal header, they're collapsed together with the legal header (and imports).

There are no technical disadvantages, but I stand by my recommendation to place the test description before any Java code constructions like package, import or type declarations.

We should not change the prevailing style just because of some new prevailing behavior in any given IDE.

alisenchung · 2024-08-14T23:43:56Z

test/langtools/tools/javac/warnings/Serial/Serial.java

move jtreg tags below imports and add copyrights

The copyright has been intentionally omitted as explained in https://bugs.openjdk.org/browse/JDK-8258525
I moved the jtreg tag.

For the reason aivanov-jdk commented, I would like to only rename files in this PR.

aivanov-jdk

I suggest not to edit the files and only rename them as it was in the first iteration of the review.

This reverts commit 9b30df1.

toshiogata · 2024-08-19T01:50:14Z

@aivanov-jdk
I agree with you. I changed the fix to only rename files.

@alisenchung
As aivanov-jdk commented, I would like to hold off on implementing your suggestion for refactoring in this PR. Please check the disucssion.

aivanov-jdk · 2024-08-19T12:35:23Z

/reviewers 2 reviewer

openjdk · 2024-08-19T12:36:17Z

@aivanov-jdk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

aivanov-jdk

Looks good to me.

Someone from the compiler team has to approve it too.

All the changes look good, however, only one test, URLDragTest.java, is from the client area.

prrace

Some one else should decide bout the langtools changes but for the AWT one I agree the minimal rename is all we need here to solve the specific problem which has nothing to do with the test itself.

jonathan-gibbons · 2024-08-23T22:31:04Z

Moving files from one directory to another can sometimes be a bit risky, especially for javac tests, because that implies moving the classes from one unnamed package to a different unnamed package.

Please confirm that all tests under test/langtools/tools/javac continue to pass after the files have been moved. You can either run just those tests, or run all langtools tier1 tests if that is easier.

jonathan-gibbons · 2024-08-23T22:36:51Z

@jonathan-gibbons Is this a bug in jtreg, or where these files actually improperly named?

Not a bug as such, but maybe a little-known misfeature. jtreg has always had problems with filenames that could cause confusion, and the general sense has generally been, "if it hurts, don't do it!". That being said, our overall library API and infrastructure is way better these days than in times past, and it might be reasonable to file an Enhancement for jtreg to have a utility/check/test to detect these anomalous situations.

In the meantime, renaming the files to avoid the problem is the recommended solution.

Filed https://bugs.openjdk.org/browse/CODETOOLS-7903803

toshiogata · 2024-08-26T07:35:37Z

@jonathan-gibbons
Thank you for your comment and filing jtreg enhancement.
I confirmed that all tests under langtools/tools/javac passed after renaming.

$ jtreg_7_4_1/jtreg/bin/jtreg -dir:jdk/test/langtools -retain:all -verbose:summary -ignore:quiet -exclude:jdk/test/langtools/ProblemList.txt -jdk:jdk-24+12 tools/javac
Directory "JTwork" not found: creating
Directory "JTreport" not found: creating
Passed: tools/javac/4241573/T4241573.java
Passed: tools/javac/4846262/CheckEBCDICLocaleTest.java

...

Passed: tools/javac/generics/odersky/Test3.java
Passed: tools/javac/generics/odersky/Test4.java
Passed: tools/javac/generics/parametricException/J.java
Passed: tools/javac/generics/parametricException/ParametricException.java
Passed: tools/javac/generics/rare/6665356/T6665356.java
Passed: tools/javac/generics/rare/Rare1.java

...

Passed: tools/javac/warnings/Serial/RecordExtern.java
Passed: tools/javac/warnings/Serial/RecordSerial.java
Passed: tools/javac/warnings/Serial/Serial.java
Passed: tools/javac/warnings/Serial/SerialMethodArity.java
Passed: tools/javac/warnings/Serial/SerialMethodMods.java

...

Passed: tools/javac/VersionOpt.java
Passed: tools/javac/VoidArray.java
Test results: passed: 3,869

toshiogata · 2024-09-24T00:56:46Z

@jonathan-gibbons
I was wondering if you have had a chance to see my response and aivanov-jdk's question. I would appreciate your reaction. Thank you.

magicus · 2024-10-11T14:20:46Z

@toshiogata Jon Gibbons is no longer working on Oracle. I can't say if that means he will no longer be active at all in the OpenJDK community, but you might want to lower your expectations of getting a reply, and I suggest proceeding with this PR without awaiting a response from him.

jonathan-gibbons · 2024-10-11T15:28:49Z

@toshiogata Jon Gibbons is no longer working on Oracle. I can't say if that means he will no longer be active at all in the OpenJDK community, but you might want to lower your expectations of getting a reply, and I suggest proceeding with this PR without awaiting a response from him.

I'm still around, and working to regain access to participate in OpenJDK, albeit at a significantly reduced level.

jonathan-gibbons · 2024-10-11T15:33:57Z

I see this is already approved/"ready". I see no reason to disagree with that.

toshiogata · 2024-10-14T23:56:18Z

@magicus @jonathan-gibbons
Thank you for your comments. I will proceed with this.

toshiogata · 2024-10-14T23:57:00Z

/integrate

openjdk · 2024-10-14T23:57:16Z

@toshiogata
Your change (at version b1b2b5f) is now ready to be sponsored by a Committer.

sormuras

Renames/moves look also good to me, thus I'll sponsor this PR.

sormuras · 2024-10-15T07:46:34Z

/sponsor

openjdk · 2024-10-15T07:47:45Z

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

a601cd2: 8342014: RISC-V: ZStoreBarrierStubC2 clobbers rflags
bd62644: 8341924: Improve error message with structurally malformed Code array
a2c7752: 8317356: Fix missing null checks in the ClassFile API
6071346: 8339879: Open some dialog awt tests
a8a8b2d: 8341831: PhaseCFG::insert_anti_dependences asserts with "no loads"
9e262df: 8342002: sun/security/tools/keytool/GenKeyPairSigner.java failed due to missing certificate output
f56a154: 8341881: [REDO] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
ba3774d: 8341637: java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
1581508: 8335091: NMT: VMATree reserve_mapping and commit_mapping APIs need MEMFLAGS while un/-committing API has no MEMFLAGS arg
b20c5c7: 8341967: Unify os::current_file_offset and os::seek_to_file_offset across posix platforms
... and 854 more: https://git.openjdk.org/jdk/compare/20575949612a750a428316635715737183a2d58c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-15T07:47:54Z

@sormuras @toshiogata Pushed as commit e6698f5.

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

toshiogata · 2024-10-17T01:55:14Z

/backport jdk23u

openjdk · 2024-10-17T01:55:35Z

@toshiogata To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

toshiogata added 2 commits August 6, 2024 14:45

8337851: Some tests have name which confuse jtreg

0edebc1

Merge branch 'master' into 8337851

4f973c7

openjdk bot added the rfr Pull request is ready for review label Aug 6, 2024

openjdk bot added client [email protected] compiler [email protected] labels Aug 6, 2024

alisenchung reviewed Aug 14, 2024

View reviewed changes

Move jtreg tags and expand wildcard imports

9b30df1

aivanov-jdk suggested changes Aug 16, 2024

View reviewed changes

Revert "Move jtreg tags and expand wildcard imports"

b1b2b5f

This reverts commit 9b30df1.

aivanov-jdk approved these changes Aug 19, 2024

View reviewed changes

prrace approved these changes Aug 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 23, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 14, 2024

sormuras approved these changes Oct 15, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 15, 2024

openjdk bot closed this Oct 15, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 15, 2024

8337851: Some tests have name which confuse jtreg #20475

8337851: Some tests have name which confuse jtreg #20475

Conversation

toshiogata commented Aug 6, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 6, 2024

openjdk bot commented Aug 6, 2024 • edited Loading

openjdk bot commented Aug 6, 2024

mlbridge bot commented Aug 6, 2024 • edited Loading

Webrevs

magicus commented Aug 6, 2024

jonathan-gibbons commented Aug 6, 2024 • edited by bridgekeeper bot 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

jonathan-gibbons Aug 23, 2024 • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

aivanov-jdk Aug 27, 2024 • 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

aivanov-jdk left a comment

Choose a reason for hiding this comment

toshiogata commented Aug 19, 2024

aivanov-jdk commented Aug 19, 2024

openjdk bot commented Aug 19, 2024

aivanov-jdk left a comment

Choose a reason for hiding this comment

prrace left a comment

Choose a reason for hiding this comment

jonathan-gibbons commented Aug 23, 2024 • edited by bridgekeeper bot Loading

jonathan-gibbons commented Aug 23, 2024 • edited by bridgekeeper bot Loading

toshiogata commented Aug 26, 2024

toshiogata commented Sep 24, 2024

magicus commented Oct 11, 2024

jonathan-gibbons commented Oct 11, 2024

jonathan-gibbons commented Oct 11, 2024

toshiogata commented Oct 14, 2024

toshiogata commented Oct 14, 2024

openjdk bot commented Oct 14, 2024

sormuras left a comment • edited Loading

Choose a reason for hiding this comment

sormuras commented Oct 15, 2024

openjdk bot commented Oct 15, 2024

openjdk bot commented Oct 15, 2024

toshiogata commented Oct 17, 2024

openjdk bot commented Oct 17, 2024

toshiogata commented Aug 6, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 6, 2024 •

edited

Loading

mlbridge bot commented Aug 6, 2024 •

edited

Loading

jonathan-gibbons commented Aug 6, 2024 •

edited by bridgekeeper bot

Loading

jonathan-gibbons Aug 23, 2024 •

edited by bridgekeeper bot

Loading

aivanov-jdk Aug 27, 2024 •

edited

Loading

jonathan-gibbons commented Aug 23, 2024 •

edited by bridgekeeper bot

Loading

jonathan-gibbons commented Aug 23, 2024 •

edited by bridgekeeper bot

Loading

sormuras left a comment •

edited

Loading