jdk8u-dev icon indicating copy to clipboard operation
jdk8u-dev copied to clipboard

8327993: [8u] Unify test libraries into single test library - step 2

Open zzambers opened this issue 1 year ago • 7 comments

This is second step in the effort to unify duplicate test libraries in jdk8 into single shared test library (as in newer jdks). Motivation is to remove code duplication and make backporting simpler. It only affects tests. More details in step 1.

This changeset eliminates duplicate test lib in hotspot ( hotspot/test/testlibrary ) by unifying it with shared test library ( test/lib ). Hotspot tests have been updated to use (unified) shared test library.

DETAILS: Unification of test libraries was done on per file basis, comparing (diff) classes from both libraries and deciding on approach used. (Generally, files in shared test lib (originally coming from jfr backport) are more updated. However there were some compatibility problems with jdk8 in some places.)

  • Legend:

    • kept - kept existing file in shared test library as is
    • same - files in both libraries (hotspot, shared) are effectively same (copyright/package differences etc.)
    • superset - file in shared test library is superset (in terms of API) of hotspot one
  • testlib packageless (test/lib)

    • ClassFileInstaller.java - kept (same)
    • RedefineClassHelper.java - used updated one from hs test lib (was missing in shared lib)
  • testlib general (test/lib/jdk/test/lib)

    • Asserts.java - kept (superset)
    • BuildHelper.java - kept (same)
    • ByteCodeLoader.java - kept (superset)
    • JDKToolFinder.java - kept (same)
    • JDKToolLauncher.java - kept, with reverted changes from JDK-8178415 to match one from hs test lib (otherwise no real differences)
    • Platform.java - kept shared version (superset), with some fixes. Updated debug build detection code as jdk8 does not have jdk.debug system property. Removed areCustomLoadersSupportedForCDS as this is only useful for appcds tests (not in jdk8)
    • Utils.java - kept, added missing getUnsafe() method, required by hotspot tests
  • testlib cli (test/lib/jdk/test/lib/cli)

    • only used by hotspot tests, classes in shared test lib were unused so far
    • used updated files from hotspot test lib, as shared version contained incompatible changes from JDK-8054892 (not backported to jdk8) and did not work with hotspot tests
  • testlib compiler (test/lib/jdk/test/lib/compiler)

    • InMemoryJavaCompiler.java - used updated one from hs test lib (was missing in shared test lib)
  • testlib conainer (test/lib/jdk/test/lib/container)

  • testlib management (test/lib/jdk/test/lib/management)

    • DynamicVMOption.java - kept (same)
  • testlib process (test/lib/jdk/test/lib/process)

    • ExitCode.java - kept (same)
    • OutputAnalyzer.java - kept (superset)
    • OutputBuffer.java - kept (same)
    • ProcessTools.java - kept, with added getPlatformSpecificVMArgs (effectively reverted changes from JDK-8178415), updated getProcessId (better to throw on error, kept it returning long (as on newer jdks)), removed pid() method from ProcessImpl (not necessary, Process in jdk8 does not have pid method)
    • StreamPumper.java - kept (superset)
  • whitebox (test/lib/sun/hotspot)

    • Whitebox.java - went through differences, removed functions not implemented in hotspot from shared lib. (Also fixed few calls in shared lib, wrongly expecting changes from JDK-8171008 in jdk8). Result is close to one from hs testlibrary, some wrapper functions around low level functions kept (used in code).
    • Nmethod.java - used updated file from hs test lib, lack of hotspot support (shared lib version expected wb.getNMethod to return data in new format - not in jdk8)
    • CodeBlob.java - kept, removed getCodeBlobs method (jdk8 hotspot lacks support for WB.getCodeHeapEntries)
    • CPUInfo.java - kept (same)
    • DiagnosticCommand.java - used updated file from hs test lib (jdk8u is missing changes by JDK-8065783 expected by shared lib version)
    • GC.java - removed - unused and (hotspot) support for this is missing in jdk8 (see JDK-8154096)
  • testlibrary other

    • PerfCounter.java - moved/updated to hotspot/test/gc/testlibrary corresponding to newer jdks
    • PerfCounters.java - moved/updated to hotspot/test/gc/testlibrary as in newer jdks
    • InputArguments.java - moved/updated to hotspot/test/gc/metaspace as in newer jdks (see changes by JDK-8157957)
  • ctw (compile the world, hotspot/test/testlibrary/ctw)

    • ctw was kept in hotspot's testlibrary, as in newer jdks. Makefile was updated, as necessary.
  • tests

    • imports and @library tags were fixed, as necessary
    • small fixes to some tests as ProcessTools.getProcessId() now returns long (not int). Similar to test modifications done by JDK-8153992 (backport not possible due to ProcessHandle unavailable on jdk8)
    • hotspot/test/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java was updated, as necessary, to work with Platform class from shared test lib

Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] JDK-8327993 needs maintainer approval

Issue

  • JDK-8327993: [8u] Unify test libraries into single test library - step 2 (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 467

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

Using diff file

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

Webrev

Link to Webrev Comment

zzambers avatar Mar 12 '24 15:03 zzambers

:wave: 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.

bridgekeeper[bot] avatar Mar 12 '24 15:03 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Mar 12 '24 15:03 mlbridge[bot]

Test failures in jdk/tier1 ( jdk_security_infra ) are unrelated. They seem to have appeared recently and they also affect newer JDKs, see: JDK-8324583 (see my comment there) (Newer JDKs just don't run jdk_security_infra as part of tier1.)

zzambers avatar Mar 12 '24 15:03 zzambers

Aditional testing: jdk_jfr: OK jdk_core: OK (no regressions to master; AKISerialNumber.java was fixed in the meantime)

zzambers avatar Mar 13 '24 15:03 zzambers

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

openjdk[bot] avatar Mar 13 '24 20:03 openjdk[bot]

AKISerialNumber.java fix has been merged.

sendaoYan avatar Mar 15 '24 09:03 sendaoYan

@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[bot] avatar Apr 12 '24 12:04 bridgekeeper[bot]

@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.

bridgekeeper[bot] avatar May 10 '24 17:05 bridgekeeper[bot]