-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8294403: [REDO] make test should report only on executed tests #11824
Conversation
👋 Welcome back shade! A progress list of the required criteria for merging this PR into |
Webrevs
|
This might need more exposure. /label add hotspot |
@shipilev |
@shipilev |
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.
Looks good from a build point of view.
@shipilev 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 27 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
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.
This seems fine to me. Thanks.
I personally always run with -noreport
. :)
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.
Hello Aleksey, this looks good to me. Good to see the timing improvements on make test
.
Please update the copyright years on the files before integrating.
I see that in the additional testing section you note, running tests with RETRY_COUNT. Is that intentional? Does retry count play a role in the report generation?
Updated.
The logic for REPEAT/RETRY_COUNT merges the jtreg reports from "different" repeated runs. We have to check new reporting mode still produces sane results in that mode. |
Thank you for that detail, I wasn't aware of that. |
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 like this!
$ time make run-test TEST=jdk/java/net/BindException/Test.java:
# baseline:
real 0m43.176s
user 0m12.453s
sys 1m13.797s
# patched:
real 0m22.558s
user 0m10.797s
sys 1m1.734s
My longer testing passes well. I would like to integrate it soon, if there are no objections. |
/integrate |
Going to push as commit 5598acc.
Your commit was automatically rebased without conflicts. |
This should help to speed up tests significantly. Currently, if we run "make test" with a subset of tests, JTReg would still read the entirety of test root to report on tests that were not run. Even with current suite of tests it gets expensive. If you add more tests to suite -- for example, mounting a large test archive like pre-generated fuzzer tests -- it starts to take a lot of time.
The reasonable default for older jtreg is
-report:executed
. My own CI -- that has millions of tests mounted intest/
-- was running withJTREG="OPTIONS=-report:executed"
for years now. CODETOOLS-7903323 provides a new reporting option,-report:files
, which makes this whole business easier. This requires new jtreg.Motivational improvements:
Additional testing:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11824/head:pull/11824
$ git checkout pull/11824
Update a local copy of the PR:
$ git checkout pull/11824
$ git pull https://git.openjdk.org/jdk pull/11824/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11824
View PR using the GUI difftool:
$ git pr show -t 11824
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11824.diff