On Tue, 4 Jun 2024 01:34:39 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 three additional > commits since the last revision: > > - Alex suggestion - remove -XX:-CheckIntrinsics from NativeMethodPrefixApp > test too > - Alex's suggestion - no need to include only the bootreporter classes in > boot classpath > - RetransformApp test did not previously use -XX:-CheckIntrinsics I've posted just a question and a nit. Otherwise, the fix looks good. Thank you for taking care about this refactoring! test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java line 34: > 32: * @modules java.management > 33: * java.instrument > 34: * @run shell/timeout=240 MakeJAR2.sh NativeMethodPrefixAgent > NativeMethodPrefixApp 'Can-Retransform-Classes: true' > 'Can-Set-Native-Method-Prefix: true' Just a question. The original test had a timeout:`timeout=240`. My guess is that the default timeout was not enough. Do you think, it is not needed anymore or you just missed to add it? The same question applies to the other test. test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 26: > 24: > 25: import java.io.File; > 26: import java.nio.file.Files; Nit: It seems the import at line 26 is not needed. ------------- Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2095508509 PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625470816 PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625472194
