8305329: [8u] Unify test libraries into single test library - step 1

[zzambers]

I would like to start effort to unify test libraries in openjdk 8 into one unified library. This is first step of that effort, which moves newest copy of jdk test library (added with jfr backport) from jdk/test/lib to test/lib. For more details please continue reading.

Problem:
Test libraries in jdk8 are a bit messy.
There are currently 3 different test libraries:

• hotspot testlibrary - placed in hotspot/test/testlibrary, (pkgs com.oracle.java.testlibrary, sun.hotspot... ), includes testlibrary tests in hotspot/test/testlibrary_tests
• jdk testlibrary (old) - placed in jdk/test/lib/testlibrary, (pkgs jdk.testlibrary, com.oracle.testlibrary.jsr292)
• jdk test lib - placed directly in jdk/test/lib, was added with jfr backport, (pkgs jdk.test.lib sun.hotspot...)

Many test library classes exist in multiple different copies (using different pkgs). Few examples:

• Platform.java - 3 copies (hs, jdk (old), jdk (jfr))
• OutputAnalyzer.java - 3 copies (hs, jdk (old), jdk (jfr))
• WhiteBox.java - 2 copies (hotspot, jdk (jfr))

Few additional observations:

• classes in jdk test lib (jfr) use java packages compatible with newer jdks, other 2 don't (other 2 are actually not even compatible with each other)
• test lib added with jfr has newest classes, but some of them are NOT JDK8 compatible, some of them do not even compile on 8 (see e.g. SecurityTools.java, InMemoryJavaCompiler.java ...) , some whitebox stuff in sun.hotspot also seem incompatible with 8 (e.g. NMethod.java (compare to NMethod.java from hs))
• most jdk tests (with exception of jfr ones) use old testlibrary, but few tests already started using test lib added with jfr
• paths to test lib as specified @library jtreg tag is different from newer jdks (for all 3 test libraries)

This situation complicates backporting and leads to confusion. (What is correct library path? What package test lib classes use? Which library to use and to which one do backports? Which part are broken and why? etc...)
See some examples: #129 #255 (comment)

Solution:
Proposed solution is to have single test library placed in test/lib directory (+ test library tests in test/lib-test) by fixing/merging test libraries. (same locations are used in newer jdks)
Goal is following:

• single test library, removed code duplication
• all test lib classes compatible with JDK8
• locations of test lib files and java packages (used by test library classes) same as in later jdks
• path as specified by @library jtreg tag compatible with later jdks (@library /test/lib)
• only test lib is moved. Tests and TEST.ROOT files are not moved, so this should not break existing CIs.

Because this is big change it should be performed in steps:

1. move jdk test library added by jfr to test/lib and do necessary modifications to tests (add external.lib.roots to TEST.ROOT files, modify @library tag of affected test)
2. Fix test lib in new location to work with hotspot tests (fix/merge with hotspot testlibrary), modify hotpot tests to use it, remove hotspot testlibrary
3. continue with jdk tests, doing more fixes to test lib, if necessary (fix/merge with jdk testlibrary), eventually migrating all tests and removing jdk testlibrary (old)

Changes in this PR:
This is just the first step and consist of:

• moved test lib classes jdk/test/lib -> test/lib (excluding testlibrary, security dirs belonging to the old jdk testlibrary)
• added appropriate external.lib.roots to TEST.ROOT files of jdk and hotspot
• modified affected tests to use moved library (just modifications of @library, with excpetion of TestNative.sh)

Testing:
tier1: passed (see Checks)
jdk_core: OK (no regressions to master)
jdk_jfr: OK (no regressions to master)

---

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
• JDK-8305329 needs maintainer approval

Issue

• JDK-8305329: [8u] Unify test libraries into single test library - step 1 (Enhancement - P4 - Approved)

Reviewers

• Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 294

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back zzambers! 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.

[mlbridge]

Webrevs

• 02: Full - Incremental (5f384666)
• 01: Full - Incremental (14666aa2)
• 00: Full (affb1253)

[bridgekeeper]

@zzambers 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!

[bridgekeeper]

@zzambers This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[zzambers]

/open

[openjdk]

@zzambers This pull request is now open

[bridgekeeper]

@zzambers 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!

[gnu-andrew]

This is something I'm aware of and I've thought about tackling it in a similar way to you a few times. Thanks for finding the time to do so which has eluded me so far.

I don't have any real issue with this PR, but then I guess this is the easy one. I'm a bit concerned about steps 2 & 3, because you seem to be suggesting that the existing HotSpot & JDK test libraries should be migrated to work with the ones from the JFR library. This seems like the wrong direction to me and a lot more work than migrating the JFR ones to work with the existing libraries (which is what the JFR backport should have done in the first place).

As you say yourself, some of the JFR imported library code doesn't even compile and it includes stuff for newer JDKs which isn't appropriate in 8u. This PR would seem like a good opportunity to remove rather than move any of these files that don't compile (which presumably also means they aren't being used by the JFR tests).

Have you verified that all the moved files actually compile? And if not, can we check that and delete any that don't?

Then, in the HotSpot stage, we can merge in the HotSpot library and replace any duplicates with the 8u versions, and see what (if anything) breaks in the JFR tests.

[gnu-andrew]

Should have put my above comment under a review...

[gnu-andrew]

I think we should also probably have a step 4 to actually move the tests. That will then solve the backporting problem where tests keep being introduced in the wrong location.

[zzambers]

Hi @gnu-andrew,
thank you for your review,

This is something I'm aware of and I've thought about tackling it in a similar way to you a few times. Thanks for finding the time to do so which has eluded me so far.

I don't have any real issue with this PR, but then I guess this is the easy one. I'm a bit concerned about steps 2 & 3, because you seem to be suggesting that the existing HotSpot & JDK test libraries should be migrated to work with the ones from the JFR library. This seems like the wrong direction to me and a lot more work than migrating the JFR ones to work with the existing libraries (which is what the JFR backport should have done in the first place).

I haven't looked in too much detail, on how they should be merged, but probably on case by case basis. I think, in case of some more generic classes, rather than trying to merge hs and jdk versions (and fixing java package), it could make sense to just go with jfr version (if it looks ok for 8). I mean more generic classes like Asserts.java, OutputAnalyzer.java, maybe Platform.java. In other cases like broken/risky(low-level) stuff it would probably be better to go with hs (or jdk) version. (e.g. whitebox stuff)

As you say yourself, some of the JFR imported library code doesn't even compile and it includes stuff for newer JDKs which isn't appropriate in 8u. This PR would seem like a good opportunity to remove rather than move any of these files that don't compile (which presumably also means they aren't being used by the JFR tests).

Have you verified that all the moved files actually compile? And if not, can we check that and delete any that don't?

I have not excluded any files, just moved all of them. However excluding broken stuff here is probably good idea. Keeping and trying to fix unused broken code, would probably be waste of time, as they can be re-added later, if needed by future backport. (I'll take a look at that)

Then, in the HotSpot stage, we can merge in the HotSpot library and replace any duplicates with the 8u versions, and see what (if anything) breaks in the JFR tests.

For hotspot-specific or low-level stuff it is probably safest approach. In case of more generic classes (which are also used by jdk tests), keeping jfr version may be easier and safer option.

Also we could use this opportunity to move whitebox classes to jdk.test.lib package (used by jdk 17+, see JDK-8067223, probably can be backported to 11)

[zzambers]

I think we should also probably have a step 4 to actually move the tests. That will then solve the backporting problem where tests keep being introduced in the wrong location.

Even though, it would be good for backporting, I have so far excluded tests themselfs from move, as there are some additional difficulties with that. While changing test library location can be done more or less transparently, in case of tests there are some complications:

• It would be more invasive change needing changes to build system (makefiles)
• It would break existing CI testing (would require changes), in cases, where jtreg is being called directly, rater than through main makefile, (e.g. Adoptium aqa-tests, see: openjdk.mk playlist.xml.)

[zzambers]

I have removed lib classes, which do not compile on jdk 8 (and ones depending on them). Retested -> OK.

[bridgekeeper]

@zzambers 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!

[bridgekeeper]

@zzambers This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[gnu-andrew]

I have removed lib classes, which do not compile on jdk 8 (and ones depending on them). Retested -> OK.

This looks good. All seem to have been added by the JFR backport. While some are unique and only used by that (LingeredApp.java, LingeredAppWithDeadlock.java, ModuleInfoMaker.java), the rest seem to be duplicated by (presumably working) versions in the hotspot and jdk libraries.

$ find -name RedefineClassHelper.java
./hotspot/test/testlibrary/RedefineClassHelper.java
./jdk/test/lib/RedefineClassHelper.java

$ find -name CompilerUtils.java
./jdk/test/lib/jdk/test/lib/compiler/CompilerUtils.java
./jdk/test/lib/testlibrary/CompilerUtils.java

$ find -name InMemoryJavaCompiler.java
./hotspot/test/testlibrary/com/oracle/java/testlibrary/InMemoryJavaCompiler.java
./jdk/test/lib/jdk/test/lib/compiler/InMemoryJavaCompiler.java
./jdk/test/lib/testlibrary/jdk/testlibrary/InMemoryJavaCompiler.java

$ find -name JarUtils.java
./jdk/test/lib/jdk/test/lib/util/JarUtils.java
./jdk/test/lib/testlibrary/jdk/testlibrary/JarUtils.java

$ find -name SecurityTools.java
./jdk/test/lib/jdk/test/lib/SecurityTools.java
./jdk/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java

[gnu-andrew]

I think we should also probably have a step 4 to actually move the tests. That will then solve the backporting problem where tests keep being introduced in the wrong location.

Even though, it would be good for backporting, I have so far excluded tests themselfs from move, as there are some additional difficulties with that. While changing test library location can be done more or less transparently, in case of tests there are some complications:

* It would be more invasive change needing changes to build system (makefiles)

* It would break existing CI testing (would require changes), in cases, where jtreg is being called directly, rater than through main makefile, (e.g. Adoptium aqa-tests, see: [openjdk.mk](https://github.com/adoptium/aqa-tests/blob/07cac1895144ecb63a2ad03c3214a52fb108cfbc/openjdk/openjdk.mk#L122) [playlist.xml](https://github.com/adoptium/aqa-tests/blob/07cac1895144ecb63a2ad03c3214a52fb108cfbc/openjdk/playlist.xml#L717).)

I know what you mean, and it is what has put me off doing this earlier. We can see how things look once the library work is done, but I think it would actually help remove a lot of special casing for 8u long term.

[zzambers]

/open

[openjdk]

@zzambers This pull request is now open

[zzambers]

Seems like this PR will need to get updated (rebased), due to changes introduced by:
#347

[openjdk]

@zzambers Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[zzambers]

I have rebased changes on current master to fix conflict as shown by GH. (Bot does not like rebasing, but I find it less evil and more readable, than merging in master.)

Rebase using git rebase master worked out of box, no manual interaction was needed. I verified that renamed NetworkConfiguration.java contains changes by JDK-8207404 backport. There is only small change on top fixing @library tag in affected tests. I have found no other issues.

[gnu-andrew]

Yes, I agree a rebase makes sense here. The bot comment makes sense to encourage people to respond to review comments with further commits, but not really with merges. Worst of all is with dependent pull requests, where merging the two identical change requests would be more dangerous than just rebasing and having git just recognise and drop the duplicate. Of course, that duplicate is only created because the bot itself does a rebase...

The additional changes look fine to me. Let's get this in.

[openjdk]

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

[zzambers]

/approval starts process of unifying test libraries and moving them to same place as in newer jdks, it only affects tests, GH testing OK

[openjdk]

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

[zzambers]

/approval request starts process of unifying test libraries and moving them to same place as in newer jdks, it only affects tests, GH testing OK

[openjdk]

@zzambers
8305329: The approval request has been created successfully.

[gnu-andrew]

/approve yes

[openjdk]

@gnu-andrew
8305329: The approval request has been approved.

[openjdk]

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

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

8305329: [8u] Unify test libraries into single test library - step 1

Reviewed-by: andrew

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 1 new commit pushed to the master branch:

• dd43bdd: 8319405: [s390] [jdk8] Increase javac default stack size for s390x zero

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.

[zzambers]

@gnu-andrew thank you

/integrate

[openjdk]

Going to push as commit eace2d7.
Since your change was applied there has been 1 commit pushed to the master branch:

• dd43bdd: 8319405: [s390] [jdk8] Increase javac default stack size for s390x zero

Your commit was automatically rebased without conflicts.

[openjdk]

@zzambers Pushed as commit eace2d7.

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