-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8321812: Update GC tests to use execute[Limited]TestJava #17067
8321812: Update GC tests to use execute[Limited]TestJava #17067
Conversation
👋 Welcome back stefank! A progress list of the required criteria for merging this PR into |
Co-authored-by: Thomas Schatzl <59967451+tschatzl@users.noreply.github.com>
@@ -101,7 +102,7 @@ public static void compileClass(String className, Path root, String source) thro | |||
.addToolArg(sourceFile.toAbsolutePath().toString()); | |||
|
|||
ProcessBuilder pb = new ProcessBuilder(jar.getCommand()); | |||
OutputAnalyzer output = new OutputAnalyzer(pb.start()); | |||
OutputAnalyzer output = ProcessTools.executeProcess(pb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to skip using an explicit ProcessBuilder. However, that hit a problem that some ProcessTools.executeProcess overloads are declared to throw Throwable instead of Exception. Instead of fixing that inside this PR, or creating a workaround for this, I created a separate, trivial PR to update those functions. Please see #17240. I've merged the branch for that PR into this PR so that we can test the updated changes.
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8321812_use_executeTestJava_in_gc_tests
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
…teTestJava_in_gc_tests
@stefank This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
…teTestJava_in_gc_tests
…xecuteTestJava_in_gc_tests
…teTestJava_in_gc_tests
Thanks for the reviews! Tier1-3 testing passes. |
Going to push as commit 1d1cd32. |
A lot of our tests use a multi-step recipe to spawn and wait for a process. Here's an example
These are the steps involved:
ProcessBuilder
ProcessBuilder::start
OutputAnalyzer
Almost all our tests could be converted to use a single call to
ProcessTools.executeTestJava
(orexecuteLimitedTestJava
), 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: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.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17067/head:pull/17067
$ git checkout pull/17067
Update a local copy of the PR:
$ git checkout pull/17067
$ git pull https://git.openjdk.org/jdk.git pull/17067/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17067
View PR using the GUI difftool:
$ git pr show -t 17067
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17067.diff
Webrev
Link to Webrev Comment