On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:<jar>` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Changes requested by lmesnik (Reviewer). test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 168: > 166: > 167: private static void launchApp(final Path agentJar) throws Exception { > 168: final OutputAnalyzer oa = ProcessTools.executeLimitedTestJava( Is there any reason the test uses 'executeLimitedTestJava' and not 'executeTestJava'? It might make sense to run it with different VM flags. ------------- PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2091382349 PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1622751369
