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

8208655: use JTreg skipped status in hotspot tests #387

Closed
wants to merge 9 commits into from

Conversation

gnu-andrew
Copy link
Member

@gnu-andrew gnu-andrew commented Nov 22, 2023

This backport brings in jtreg.SkippedException which is used by JDK-8308592 (and likely other potential future backports as well). It is a test-only change which allows a skipped test to be correctly reported to jtreg, rather than it appearing like a successful run.

The actual code changes to the HotSpot tests to use SkippedException were mostly clean. The main manual adaptations involved the placement of import statements (8u also doesn't have 8132919: 'Put compiler tests in packages') and copyright headers. Most tests also needed to have the library location /test/lib added. This is because the HotSpot tests are currently still using the HotSpot test library, not the top-level library. @zzambers work on this should eventually remove the need to reference both libraries.

I've included with this change a number of follow-on changes which were necessary to make the changed tests pass. I don't know how the original version of 8208655 was committed as is, because it contains a number of typos and missing import statements which cause tests to fail. The follow-up changes are:

  • 8023735 backport. This change introduces the code which is altered by 8208655 to use SkippedException. The fix is simple enough to just backport with this change and fix the test in the process. It applied cleanly other than the @ignore line which has been replaced in 8u by a ProblemList.txt addition which is removed by this backport instead.
  • Addition of vm.debug; this is cherry-picked from 8155219: "[TESTBUG] Rewrite compiler/ciReplay/TestVM.sh in java" which first introduced it in later JDKs. The implementation needs to be adapted to work around the absence of jdk.debug (JDK-8139986). Using java.vm.version matches what happens in the Platform.java test library code.
  • 8208701: first fix for typos in 8208655 as committed. Some changes had to be manually applied due to context differences but otherwise a clean backport.
  • 8208706: fixes a missing import line in ConstantGettersTransitionsTest.java, which was applied manually due to differing context.
  • 8213410 backport which is necessary as 8208655 removes a number of checks altogether, replacing them with @requires statements. This is why we need vm.debug above and also this change due to the removed 32-bit check in runtime/CompressedOops/CompressedClassPointers.java
  • For consistency, the debug build check in TestLargePageUseForAuxMemory.java was replaced with vm.debug. In 11u, this is removed through the enhancement JDK-8152491: "Convert TracePageSizes to use UL"

Absent tests were omitted. Some may be worth a later backport and should ideally use SkippedException in backporting.

  • test/compiler/codecache/dtrace/SegmentedCodeCacheDtraceTest.java - 8059625: JEP-197 JDK-8043304: Test task: DTrace- tests for segmented codecache feature
  • test/compiler/intrinsics/base64/TestBase64.java - 8205528: Base64 encoding algorithm using AVX512 instructions
  • test/compiler/jvmci/compilerToVM/MaterializeVirtualObjectTest.java - 8136421: JEP 243: Java-Level JVM Compiler Interface
  • test/runtime/libadimalloc.solaris.sparc/Testlibadimalloc.java -- 8141445: Use of Solaris/SPARC M7 libadimalloc.so can generate unknown signal in hs_err file
  • test/runtime/signal/SigTestDriver.java -- JDK-8200126 Open source VM runtime signal tests (needs dep JDK-8072842)
  • test/serviceability/jvmti/NotifyFramePop/NotifyFramePopTest.java -- 8187289: NotifyFramePop request is not cleared if JVMTI_EVENT_FRAME_POP is disabled
  • test/serviceability/sa/ClhsdbCDSCore.java -- 8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS
  • test/serviceability/sa/ClhsdbCDSJstackPrintAll.java -- 8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS
  • test/serviceability/sa/ClhsdbInspect.java -- 8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands
  • test/serviceability/sa/ClhsdbJdis.java -- 8193124: SA: Testcases for clhsdb jdis and findpc commands
  • test/serviceability/sa/ClhsdbLongConstant.java -- 8190198: SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands
  • test/serviceability/sa/ClhsdbPrintAs.java -- 8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands
  • test/serviceability/sa/ClhsdbRegionDetailsScanOopsForG1.java -- 8175312: SA: clhsdb: Provide an improved heap summary for 'universe' for G1GC
  • test/serviceability/sa/ClhsdbScanOops.java -- 8192985: SA: Test cases for the clhsdb 'inspect', 'scanoops' and 'printas' commands
  • test/serviceability/sa/DeadlockDetectionTest.java -- 8086134: Deadlock detection fails to attach to core file
  • test/serviceability/sa/TestJmapCore.java -- 8203491: [TESTBUG] Port heapdump tests into java
  • test/vmTestbase/gc/g1/unloading/UnloadingTest.java -- 8199370: [TESTBUG] Open source vm testbase GC tests
  • test/vmTestbase/nsk/jdi/AttachingConnector/attach/attach004/TestDriver.java -- 8199382: [TESTBUG] Open source VM testbase JDI tests
  • test/vmTestbase/nsk/monitoring/stress/thread/strace001.java -- 8199375: [TESTBUG] Open source vm testbase monitoring tests

The following two are present, but in much older versions in different locations:

  • test/compiler/codegen/aes/TestAESMain.java -> ./test/compiler/7184394/TestAESMain.java 8207153: Some intrinsic tests take long time to run
  • test/compiler/codegen/aes/TestCipherBlockChainingEncrypt.java -> ./test/compiler/8209951/TestCipherBlockChainingEncrypt.java 8219513: compiler/codegen/aes/TestCipherBlockChainingEncrypt.java timeout on Solaris-sparc

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • JDK-8208706 needs maintainer approval
  • JDK-8183503 needs maintainer approval
  • JDK-8023735 needs maintainer approval
  • JDK-8213410 needs maintainer approval
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • JDK-8208655 needs maintainer approval
  • JDK-8186095 needs maintainer approval
  • JDK-8208701 needs maintainer approval

Issues

  • JDK-8208655: use JTreg skipped status in hotspot tests (Enhancement - P4 - Approved)
  • JDK-8023735: [TESTBUG][macosx] runtime/XCheckJniJsig/XCheckJSig.java fails on MacOS X (Bug - P4 - Approved)
  • JDK-8208701: Fix for JDK-8208655 causes test failures in CI tier1 (Bug - P2 - Approved)
  • JDK-8208706: compiler/tiered/ConstantGettersTransitionsTest.java fails to compile (Bug - P3 - Approved)
  • JDK-8186095: upgrade to jtreg 4.2 b08 (Bug - P4 - Approved)
  • JDK-8183503: Update hotspot tests to allow for unique test classes directory (Bug - P4 - Approved)
  • JDK-8213410: UseCompressedOops requirement check fails fails on 32-bit system (Bug - P4 - Approved)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 387

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 22, 2023

👋 Welcome back andrew! 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 changed the title Backport 17b730d7edf7be91ee841180ea6a59b26c0c567a 8208655: use JTreg skipped status in hotspot tests Nov 22, 2023
@openjdk
Copy link

openjdk bot commented Nov 22, 2023

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

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Nov 22, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 22, 2023

Webrevs

@gnu-andrew
Copy link
Member Author

Setting requiredVersion seems to have broken test.Empty, used by the four failing tests. Looking into the fix now.

@gnu-andrew
Copy link
Member Author

/issue add 8023735,8208701,8208706,8313410,8129355,8186095,8183503

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@gnu-andrew The issue 8129355 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

@gnu-andrew
Copy link
Member Author

8129355 was a clean backport and a pre-requisite for 8186095
8186095 was a clean backport for the test file. The version bump to TEST.ROOT was omitted as we already introduced this in the main patch.
8183503 was a clean backport for the library file we needed to update, hotspot/test/runtime/testlibrary/ClassUnloadCommon.java. hotspot/test/runtime/logging/ClassLoadUnloadTest.java does not exist in 8u (no Unified Logging from 8079408). The hotspot/test/compiler/classUnloading/anonymousClass/TestAnonymousClassUnloading.java change is essentially a reversion of the change from JDK-8132919: "Put compiler tests in packages" which creates a path from the fully qualified class name. We don't have this in 8u so, while we use the more flexible version from 8183503 that doesn't hardcode the class name, the code is essentially unchanged.

@gnu-andrew
Copy link
Member Author

@gnu-andrew The issue 8129355 was not found in the JDK project - make sure you have entered it correctly. As there were validation problems, no additional issues will be added to the list of solved issues.

This is correct, but the bug is private for some reason.

@gnu-andrew
Copy link
Member Author

/issue add 8023735,8208701,8208706,8313410,8186095,8183503

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@gnu-andrew
Adding additional issue to issue list: 8023735: [TESTBUG][macosx] runtime/XCheckJniJsig/XCheckJSig.java fails on MacOS X.

Adding additional issue to issue list: 8208701: Fix for JDK-8208655 causes test failures in CI tier1.

Adding additional issue to issue list: 8208706: compiler/tiered/ConstantGettersTransitionsTest.java fails to compile.

Adding additional issue to issue list: 8313410: Module finder incorrectly assumes default file system path-separator character.

Adding additional issue to issue list: 8186095: upgrade to jtreg 4.2 b08.

Adding additional issue to issue list: 8183503: Update hotspot tests to allow for unique test classes directory.

@gnu-andrew
Copy link
Member Author

/issue remove 8313410
/issue add 8213410

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@gnu-andrew
Removing additional issue from issue list: 8313410.

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@gnu-andrew
Adding additional issue to issue list: 8213410: UseCompressedOops requirement check fails fails on 32-bit system.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Looked through the tests, mainly with an eye for required/skipped replacements of existing manual test exclusion. Also looked a the libjsig test a bit more closely. I did not directly compare all patches against the new one (now I understand why you maintainers dislike compound patches so much).

@@ -30,6 +30,7 @@
* @bug 8000754
* @summary Tests that a MemoryPoolMXBeans is created for metaspace and that a
* MemoryManagerMXBean is created.
* @requires vm.bits == 64
Copy link
Member

Choose a reason for hiding this comment

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

Why? We have metaspace also on 32-bit, just no class space.

Copy link
Member Author

Choose a reason for hiding this comment

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

This and the other test you commented on are https://bugs.openjdk.org/browse/JDK-8213410
The change was backported for the alteration to runtime/CompressedOops/CompressedClassPointers.java which would otherwise have its code-based 64-bit check removed by 8208655 and not replaced.

Both of the other tests have this restriction up to trunk as well. I think the history comes from adding vm.opt.final.UseCompressedOops in JDK-8204167 which causes the error shown in 8213410, even though the actual run would work without compressed oops thanks to -XX:+IgnoreUnrecognizedVMOptions. TestMetaspaceMemoryPool even still includes a if (Platform.is64bit()) and if (InputArguments.contains("-XX:+UseCompressedOops")) in the main method which enforce the same restrictions.

I did look at backporting JDK-8204167 as well, but vm.opt.final.UseCompressedOops returned null for me even on x86_64. Maybe some test infrastructure or HotSpot change we are missing.

I don't see any issue with backporting the whole patch, given the exclusions are also there in 11+. If there is a case for running these on 32-bit, the restriction should probably be dropped in trunk and backported down for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that makes sense. Thanks for clarifying.

I looked at test/hotspot/jtreg/gc/metaspace/TestMetaspaceMemoryPool.java more closely. That test could be run for 32-bit as well, but its not worth fixing.

@@ -25,6 +25,7 @@
* @test CompressedClassSpaceSizeInJmapHeap
* @bug 8004924
* @summary Checks that jmap -heap contains the flag CompressedClassSpaceSize
* @requires vm.bits == 64
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this. The flag itself exists also on 32-bit, so the test should work there; it is pointless though since the flag is never used.

@gnu-andrew
Copy link
Member Author

Looked through the tests, mainly with an eye for required/skipped replacements of existing manual test exclusion. Also looked a the libjsig test a bit more closely. I did not directly compare all patches against the new one (now I understand why you maintainers dislike compound patches so much).

Yes, this is why I at least kept the individual backports to separate commits, so a 1-to-1 comparison can still be performed while this change doesn't break any tests. I could split it into about half a dozen separate PRs but I'm not sure there's much advantage (plus we already have a dependency chain of four to get to #390) and the first change would result in several test failures being introduced.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Good.

@tstuefe
Copy link
Member

tstuefe commented Nov 24, 2023

Argh, again I forgot to mention that I am not an official jdk8 R eviewer.

@gnu-andrew
Copy link
Member Author

Argh, again I forgot to mention that I am not an official jdk8 R eviewer.

Feedback still very welcome and @jerboaa can use your comments in his formal approval.
If you'd like me to start a vote to make you an 8u reviewer, let me know. Anyone who is a Reviewer in updates & trunk, but not in 8u, seems like a bug to me and more to do with timing than any lack of technical qualification.

@tstuefe
Copy link
Member

tstuefe commented Nov 24, 2023

Argh, again I forgot to mention that I am not an official jdk8 R eviewer.

Feedback still very welcome and @jerboaa can use your comments in his formal approval. If you'd like me to start a vote to make you an 8u reviewer, let me know.

Sure, go ahead (but no pressure, this is not urgent).

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

This is a large change. If it wasn't only in the testing area, then this would be hard to argue to get in. GHA looks OK. Thumbs up.

package jtreg;

/**
* {@code SkippedException} is an exception treaded by jtreg as an indication
Copy link
Contributor

Choose a reason for hiding this comment

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

typo treated.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://bugs.openjdk.org/browse/JDK-8209740 :-)
I can follow up with that change and 8267751: "(test) jtreg.SkippedException has no serial VersionUID" but they are not that critical

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

⚠️ @gnu-andrew 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.

@gnu-andrew
Copy link
Member Author

This is a large change. If it wasn't only in the testing area, then this would be hard to argue to get in. GHA looks OK. Thumbs up.

If it wasn't only in the testing area, I wouldn't submit this kind of change for a backport. Making such changes to the codebase is a risk to stability. But, on the other hand, not improving the test suite where we can is also a risk to stability. To me, the two (sources and tests) are quite different things that happen to share the same repository.

In GNU Classpath, we actually used to have the test suite as a separate independent repository, which I think was a better approach. Yes, it's harder to make sure things in there work appropriately on each JDK version, but it means each version is using the latest tests and would cut down on a huge amount of backporting work. 8u's version of the test suite is still quite immature compared with where trunk is now, so it's not a surprise that there are backports like this.

Note also that this is not one change, but seven. I could have filed them all as separate PRs but, as I said to Thomas, it doesn't really make sense. I don't know how 8208655 was accepted in the state it's in, because it has a whole host of things like typos and missing import statements that are fixed in subsequent changes, but should have been caught by running the tests before commit (that's how I found them and then the fixes). So I think what we have here is much closer to how 8208655 should have been to begin with.

Thanks both of you for the reviews. I know it's not easy on larger changes.

@gnu-andrew
Copy link
Member Author

/approval Introduce jtreg.SkippedException into 8u to ease the backport of JDK-8308592 and other subsequent test changes that use it. The backport has a fair chain of minor follow-on fixes which we cover under the same PR to ensure GHA tests continue to pass.

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@gnu-andrew usage: /approval [<id>] (request|cancel) [<text>]

@gnu-andrew
Copy link
Member Author

/approval request Introduce jtreg.SkippedException into 8u to ease the backport of JDK-8308592 and other subsequent test changes that use it. The backport has a fair chain of minor follow-on fixes which we cover under the same PR to ensure GHA tests continue to pass.

@openjdk openjdk bot added the approval label Nov 24, 2023
@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@gnu-andrew
8208655: The approval request was already up to date.
8023735: The approval request was already up to date.
8208701: The approval request has been created successfully.
8208706: The approval request has been created successfully.
8186095: The approval request has been created successfully.
8183503: The approval request has been created successfully.
8213410: The approval request has been created successfully.

@jerboaa
Copy link
Contributor

jerboaa commented Nov 27, 2023

/approve yes

@openjdk
Copy link

openjdk bot commented Nov 27, 2023

@jerboaa
8208655: The approval request has been approved.
8023735: The approval request has been approved.
8208701: The approval request has been approved.
8208706: The approval request has been approved.
8186095: The approval request has been approved.
8183503: The approval request has been approved.
8213410: The approval request has been approved.

@openjdk
Copy link

openjdk bot commented Nov 27, 2023

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

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

8208655: use JTreg skipped status in hotspot tests
8023735: [TESTBUG][macosx] runtime/XCheckJniJsig/XCheckJSig.java fails on MacOS X
8208701: Fix for JDK-8208655 causes test failures in CI tier1
8208706: compiler/tiered/ConstantGettersTransitionsTest.java fails to compile
8186095: upgrade to jtreg 4.2 b08
8183503: Update hotspot tests to allow for unique test classes directory
8213410: UseCompressedOops requirement check fails fails on 32-bit system

Reviewed-by: stuefe, sgehwolf

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

  • 0464401: 8320713: Bump update version of OpenJDK: 8u412
  • b372b4b: 8312489: Increase jdk.jar.maxSignatureFileSize default which is too low for JARs such as WhiteSource/Mend unified agent jar

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 added ready Pull request is ready to be integrated and removed approval labels Nov 27, 2023
@gnu-andrew
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 28, 2023

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

  • 0464401: 8320713: Bump update version of OpenJDK: 8u412
  • b372b4b: 8312489: Increase jdk.jar.maxSignatureFileSize default which is too low for JARs such as WhiteSource/Mend unified agent jar

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Nov 28, 2023

@gnu-andrew Pushed as commit 3b9c787.

💡 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
3 participants