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

8259267: Refactor LoaderLeak shell test as java test. #1577

Conversation

@frkator
Copy link
Member

@frkator frkator commented Dec 2, 2020

Refactor test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh as java test.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8259267: Refactor LoaderLeak shell test as java test.

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577
$ git checkout pull/1577

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 2, 2020

👋 Welcome back isipka! 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 openjdk bot added the rfr label Dec 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

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

  • core-libs

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.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

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

8259267: Refactor LoaderLeak shell test as java test.

Reviewed-by: rriggs, iignatyev, 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 86 new commits pushed to the master branch:

  • a118185: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics
  • 2d2ef08: 8262885: Shenandoah: FullGC prologue does not need to save/restore heap has_forwarded_object flag
  • 1d2c1e6: 8248314: Parallel: Parallelize parallel full gc Adjust Roots phase
  • 3d3eb5c: 8262368: wrong verifier message for bogus return type
  • 6d3c858: 8259235: javac crashes while attributing super method invocation
  • bf90e85: 8262926: JDK-8260966 broke AIX build
  • 54dfd79: 8262256: C2 intrinsincs should not modify IR when bailing out
  • 0265ab6: 8262466: linux libsaproc/DwarfParser.cpp delete DwarfParser object in early return
  • c15801e: 8261142: AArch64: Incorrect instruction encoding when right-shifting vectors with shift amount equals to the element width
  • 044e2a2: 8183569: Assert the same limits are used in parse_xss and globals.hpp
  • ... and 76 more: https://git.openjdk.java.net/jdk/compare/a50725db2ab621e1a17cb5505f78e4bc73972a27...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 (@RogerRiggs, @iignatev, @dfuch) 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 openjdk bot added the ready label Dec 9, 2020
@frkator
Copy link
Member Author

@frkator frkator commented Dec 9, 2020

/integrate

@openjdk openjdk bot added the sponsor label Dec 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

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

@frkator frkator changed the title 8166026: Refactor java/lang shell tests to java 8259267: Refactor LoaderLeak shell test as java test. Jan 5, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 3, 2021

@frkator This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk openjdk bot removed the sponsor label Feb 3, 2021
@frkator frkator force-pushed the frkator:JDK-8166026-refactor-shell-to-java-loaderleak branch 2 times, most recently from fc74f13 to f6ba717 Feb 8, 2021
@frkator
Copy link
Member Author

@frkator frkator commented Feb 9, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Feb 9, 2021

@frkator Only Committers are allowed to sponsor changes.

@frkator
Copy link
Member Author

@frkator frkator commented Feb 9, 2021

/integrate

@openjdk openjdk bot added the sponsor label Feb 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 9, 2021

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

test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
test/jdk/java/lang/annotation/LoaderLeakTest.java Outdated Show resolved Hide resolved
@frkator frkator force-pushed the frkator:JDK-8166026-refactor-shell-to-java-loaderleak branch from f6ba717 to d9d41eb Feb 20, 2021
@openjdk openjdk bot removed the sponsor label Feb 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@frkator Only Committers are allowed to sponsor changes.

@frkator
Copy link
Member Author

@frkator frkator commented Feb 21, 2021

/integrate

@openjdk openjdk bot added the sponsor label Feb 21, 2021
@frkator frkator force-pushed the frkator:JDK-8166026-refactor-shell-to-java-loaderleak branch 2 times, most recently from ad38775 to d1ea61c Feb 24, 2021
@frkator
Copy link
Member Author

@frkator frkator commented Mar 3, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

@frkator
Your change (at version 8cad13d) is now ready to be sponsored by a Committer.

@dfuch
dfuch approved these changes Mar 3, 2021
Copy link
Member

@dfuch dfuch left a comment

LGTM

catch (NullPointerException npe) {
throw new AssertionError("c.get() should never return null", npe);
}
Comment on lines 72 to 74

This comment has been minimized.

@iignatev

iignatev Mar 3, 2021
Member

I don't think it's the right way to handle that, you don't know if this NPE is from c.get, so the exception messages might be misleading. I'd just remove try/catch, if NPE occurs jtreg will report the test as failed w/ NPE as a reason, so whoever analyzes it will be able to understand.

alternatively, you can save c.get into a local variable which you nulls later one, e.g.

static void doTest(boolean readAnn) throws Exception {
...
var tmp = c.get();
if (t == null) throw new AssertionError("c.get is null");
if (t.getClassLoader() != loader) throw new AssertionError("wrong classloader");
t = null;

This comment has been minimized.

@frkator

frkator Mar 3, 2021
Author Member

@iignatev I just reemove the try catch I think the comments are informative and will help with analysis with potential NPE

@openjdk openjdk bot removed the sponsor label Mar 3, 2021
@frkator
Copy link
Member Author

@frkator frkator commented Mar 3, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

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

@iignatev
Copy link
Member

@iignatev iignatev commented Mar 3, 2021

/sponsor

@openjdk openjdk bot closed this Mar 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

@iignatev @frkator Since your change was applied there have been 86 commits pushed to the master branch:

  • a118185: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics
  • 2d2ef08: 8262885: Shenandoah: FullGC prologue does not need to save/restore heap has_forwarded_object flag
  • 1d2c1e6: 8248314: Parallel: Parallelize parallel full gc Adjust Roots phase
  • 3d3eb5c: 8262368: wrong verifier message for bogus return type
  • 6d3c858: 8259235: javac crashes while attributing super method invocation
  • bf90e85: 8262926: JDK-8260966 broke AIX build
  • 54dfd79: 8262256: C2 intrinsincs should not modify IR when bailing out
  • 0265ab6: 8262466: linux libsaproc/DwarfParser.cpp delete DwarfParser object in early return
  • c15801e: 8261142: AArch64: Incorrect instruction encoding when right-shifting vectors with shift amount equals to the element width
  • 044e2a2: 8183569: Assert the same limits are used in parse_xss and globals.hpp
  • ... and 76 more: https://git.openjdk.java.net/jdk/compare/a50725db2ab621e1a17cb5505f78e4bc73972a27...master

Your commit was automatically rebased without conflicts.

Pushed as commit 75aa154.

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

@frkator frkator deleted the frkator:JDK-8166026-refactor-shell-to-java-loaderleak branch Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants