RFR: 8321812: Update GC tests to use execute[Limited]TestJava [v3]
Axel Boldt-Christmas
aboldtch at openjdk.org
Wed Jan 3 08:42:42 UTC 2024
On Wed, 3 Jan 2024 07:59:48 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:
>> A lot of our tests use a multi-step recipe to spawn and wait for a process. Here's an example
>>
>> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(
>> "-XX:-UseTLAB",
>> "-XX:+UnlockDiagnosticVMOptions",
>> "-XX:+VerifyDuringStartup",
>> "-Xlog:gc+verify=debug",
>> "-version");
>> OutputAnalyzer output = new OutputAnalyzer(pb.start());
>> ... do something with output and wait for the process to complete ...
>>
>>
>> These are the steps involved:
>>
>> 1) Create a `ProcessBuilder`
>> 2) Call `ProcessBuilder::start`
>> 3) Create an `OutputAnalyzer`
>> 4) Perform an operation that finally waits for the process to, at least partially, complete (OutputAnalyzer::getOutput, OutputAnalyzer::shouldHaveExitValue(), and more).
>>
>> Almost all our tests could be converted to use a single call to `ProcessTools.executeTestJava` (or `executeLimitedTestJava`), which spawns the process, makes sure that it has fully completed, and then returns a filled-in OutputAnalyzer to the caller. The above example would become:
>>
>>
>> OutputAnalyzer output = ProcessTools.executeTestJava(
>> "-XX:-UseTLAB",
>> "-XX:+UnlockDiagnosticVMOptions",
>> "-XX:+VerifyDuringStartup",
>> "-Xlog:gc+verify=debug",
>> "-version");
>>
>>
>> I propose that we make this change in the GC tests, to make our code simpler and hopefully easier to read.
>>
>> Note: There's a few changes to the throws statements because some ProcessTools APIs throws IOException while others throw Exception.
>>
>> Testing: I've done testing on a similar set of changes, but I'm going to run the appropriate, final tests while this is being considered/reviewed.
>
> Stefan Karlsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
> - Merge branch 'rename_executeTestJvm' into 8321812_use_executeTestJava_in_gc_tests
> - Whitespace fixes
>
> Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>
> - 8321812: Update GC tests to use execute[Limited]TestJava
Marked as reviewed by aboldtch (Reviewer).
test/hotspot/jtreg/gc/g1/TestSharedArchiveWithPreTouch.java line 62:
> 60: dump_args.addAll(Arrays.asList(new String[] { "-Xshare:dump", "-Xlog:cds" }));
> 61:
> 62: pb = ProcessTools.createLimitedTestJavaProcessBuilder(dump_args);
- ProcessBuilder pb;
```
Above no longer used.
test/hotspot/jtreg/gc/testlibrary/Helpers.java line 105:
> 103:
> 104: ProcessBuilder pb = new ProcessBuilder(jar.getCommand());
> 105: OutputAnalyzer output = ProcessTools.executeProcess(pb);
The pattern
OutputAnalyzer output = ProcessTools.executeProcess(jcmd.getCommand());
is used above, but an explicit `ProcessBuilder` is used here. Maybe one style should be picked?
`test/hotspot/jtreg/gc/arguments/TestUseCompressedOopsFlagsWithUlimit.java` does something similar to this as well.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17067#pullrequestreview-1801558649
PR Review Comment: https://git.openjdk.org/jdk/pull/17067#discussion_r1440185432
PR Review Comment: https://git.openjdk.org/jdk/pull/17067#discussion_r1440191965
More information about the hotspot-gc-dev
mailing list