Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8319567: Update java/lang/invoke tests to support vm flags #3153

Closed
wants to merge 1 commit into from

Conversation

GoeLin
Copy link
Member

@GoeLin GoeLin commented Dec 20, 2024

This change did not apply well. I needed the following adaptions:

test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnloadTest.java
is missing in 17. It was added by "8266310: deadlock between System.loadLibrary and JNI FindClass loading another class"
in 18. Skipped.

Resolved test/jdk/java/lang/invoke/lambda/LambdaAccessControlTest.java
bacause "8284209: Replace remaining usages of 'a the' in source code" not in 17.

test/jdk/java/lang/invoke/lambda/LambdaAsm.java
resolved because
8292914: Lambda proxies have unstable names
and 8304846: Provide a shared utility to dump generated classes defined via Lookup API
not in 17

I needed to keep one of the removed imports.

test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java
This needed an extensive resolve.
8304846: Provide a shared utility to dump generated classes defined via Lookup API
and
8307944: ClassFileDumper should only load java.nio.file.Path if enabled
not in 17.

I had to rework this test significantly.

The test assures that a property works that makes the VM dump
lambda classes it has generated. This mechanism has changed between 17 and 21.
E.g., the property name is different. Also, 17 needs to be passed
a directory name, 21 has a default name for this directory.
Some error cases are handled differently, sometimes a bug
is reported where the other implementation continues silently.

I try to take over the test structure of the test in 21,
but I adapt it to test for the same VM behaviour the old
test in 17 did.

The new test compiles the test class to subdirectory "classes/"
where the old test compiled them to ".".
Also, the new test places all dumped lambda classes in
DUMP_LAMBDA_PROXY_CLASS_FILES that is generated in the
working directory, where the old test wrote them directly
into the working directory.

test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java
resolved as
8221642: AccessibleObject::setAccessible throws NPE when invoked by JNI code with no java frame on stack
and
8300139 [AIX] Use pthreads to avoid JNI_createVM call from primordial thread
not in 17


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • JDK-8319677 needs maintainer approval
  • JDK-8319568 needs maintainer approval
  • Change must not contain extraneous whitespace
  • JDK-8319676 needs maintainer approval
  • Commit message must refer to an issue
  • JDK-8319672 needs maintainer approval
  • JDK-8319567 needs maintainer approval

Issues

  • JDK-8319567: Update java/lang/invoke tests to support vm flags (Sub-task - P4 - Approved)
  • JDK-8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java to accept vm flags (Sub-task - P4 - Approved)
  • JDK-8319672: Several classloader tests ignore VM flags (Sub-task - P4 - Approved)
  • JDK-8319676: A couple of jdk/modules/incubator/ tests ignore VM flags (Sub-task - P4 - Approved)
  • JDK-8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as flagless (Sub-task - P4 - Approved)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/3153/head:pull/3153
$ git checkout pull/3153

Update a local copy of the PR:
$ git checkout pull/3153
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/3153/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3153

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/3153.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 20, 2024

👋 Welcome back goetz! 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
Copy link

openjdk bot commented Dec 20, 2024

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

8319567: Update java/lang/invoke tests to support vm flags
8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java to accept vm flags
8319672: Several classloader tests ignore VM flags
8319676: A couple of jdk/modules/incubator/ tests ignore VM flags
8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as flagless

Reviewed-by: mdoerr

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

  • e6fb662: 8345414: Google CAInterop test failures
  • 8f93d8a: 8342602: Remove JButton/PressedButtonRightClickTest test
  • e7fedf5: 8343128: PassFailJFrame.java test result: Error. Bad action for script: build}
  • b121ce5: 8328553: Get rid of JApplet in test/jdk/sanity/client/lib/SwingSet2/src/DemoModule.java
  • 0b14f53: 8326421: Add jtreg test for large arrayCopy disjoint case.
  • 0ea0b86: 8329210: Delete Redundant Printer Dialog Modality Test
  • 409158e: 8315883: Open source several Swing JToolbar tests
  • a35aef8: 8316056: Open source several Swing JTree tests
  • 247815b: 8325937: runtime/handshake/HandshakeDirectTest.java causes "monitor end should be strictly below the frame pointer" assertion failure on AArch64
  • 10ea45e: 8316149: Open source several Swing JTree JViewport KeyboardManager tests
  • ... and 1 more: https://git.openjdk.org/jdk17u-dev/compare/bfc957aa795dd3656c9f1ecf92e68be2b5144ec0...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.

@openjdk openjdk bot changed the title Backport 1588dd934ce4e00a060e329b80f721d894559597 8319567: Update java/lang/invoke tests to support vm flags Dec 20, 2024
@openjdk
Copy link

openjdk bot commented Dec 20, 2024

This backport pull request has now been updated with issues from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Dec 20, 2024
@mlbridge
Copy link

mlbridge bot commented Dec 20, 2024

Webrevs

@GoeLin
Copy link
Member Author

GoeLin commented Dec 22, 2024

GHA failure: wget failted twice, Unrelated

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seems like many manual changes were needed because openjdk/jdk@dd59471 is not in 17u.

@openjdk
Copy link

openjdk bot commented Dec 23, 2024

⚠️ @GoeLin This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

@GoeLin
Copy link
Member Author

GoeLin commented Dec 23, 2024

Thanks for the review!!

@openjdk openjdk bot added approval and removed approval labels Dec 23, 2024
@GoeLin
Copy link
Member Author

GoeLin commented Dec 24, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Dec 24, 2024

@GoeLin This pull request has not yet been marked as ready for integration.

@openjdk openjdk bot added approval ready Pull request is ready to be integrated and removed approval labels Dec 24, 2024
@GoeLin
Copy link
Member Author

GoeLin commented Dec 27, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Dec 27, 2024

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

  • a266ca0: 8323196: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Events are not ordered! Reuse = false"
  • 5623e10: 8331153: JFR: Improve logging of jdk/jfr/api/consumer/filestream/TestOrdered.java
  • e6fb662: 8345414: Google CAInterop test failures
  • 8f93d8a: 8342602: Remove JButton/PressedButtonRightClickTest test
  • e7fedf5: 8343128: PassFailJFrame.java test result: Error. Bad action for script: build}
  • b121ce5: 8328553: Get rid of JApplet in test/jdk/sanity/client/lib/SwingSet2/src/DemoModule.java
  • 0b14f53: 8326421: Add jtreg test for large arrayCopy disjoint case.
  • 0ea0b86: 8329210: Delete Redundant Printer Dialog Modality Test
  • 409158e: 8315883: Open source several Swing JToolbar tests
  • a35aef8: 8316056: Open source several Swing JTree tests
  • ... and 3 more: https://git.openjdk.org/jdk17u-dev/compare/bfc957aa795dd3656c9f1ecf92e68be2b5144ec0...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 27, 2024
@openjdk openjdk bot closed this Dec 27, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 27, 2024
@openjdk
Copy link

openjdk bot commented Dec 27, 2024

@GoeLin Pushed as commit 743724e.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

2 participants