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
8272291: mark hotspot runtime/logging tests which ignore external VM flags #5075
Conversation
|
Webrevs
|
Thanks, Misha. can I also get a Review from a Reviewer? -- Igor |
ping? |
Hi Igor,
There are a couple of changes to existing @requires that I don't agree with. Otherwise all seems okay.
Thanks,
David
@@ -25,7 +25,7 @@ | |||
/* | |||
* @test ClassLoadUnloadTest | |||
* @bug 8142506 | |||
* @requires vm.opt.final.ClassUnloading |
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 new requirement is weaker than the old. If ClassUnloading were not enabled by default then this test would now run when it should not.
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.
nice catch, I guess I assumed that the test makes sure its child JVMs are run in the correct setting (as CompressedOopsTest.java
does). I'll update the test to pass -XX:+ClassUnloading
in ClassLoadUnloadTest::exec
.
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.
They are not exactly equivalent conditions, but when combined with vm.flagless this change seems okay.
* @requires vm.bits == 64 & vm.opt.final.UseCompressedOops == true | ||
* @requires vm.bits == 64 | ||
* @requires vm.flagless |
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.
Similar comment here, you have weakened the condition that enables the test.
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 test explicitly passes -XX:+UseCompressedOops
to a child JVM, so the condition hasn't been weakened, if anything this change increases the coverage for JVMs where UseCompressedOops
isn't true by default as now the test will be included in out-of-box runs.
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.
Okay. Thinking more about this case I think the old condition was to stop this test from being run when compressed oops was actually disabled, because it was going to enable them anyway. But now vm.flagless takes care of that case.
@@ -25,7 +25,7 @@ | |||
/* | |||
* @test ClassLoadUnloadTest | |||
* @bug 8142506 | |||
* @requires vm.opt.final.ClassUnloading |
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.
They are not exactly equivalent conditions, but when combined with vm.flagless this change seems okay.
* @requires vm.bits == 64 & vm.opt.final.UseCompressedOops == true | ||
* @requires vm.bits == 64 | ||
* @requires vm.flagless |
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.
Okay. Thinking more about this case I think the old condition was to stop this test from being run when compressed oops was actually disabled, because it was going to enable them anyway. But now vm.flagless takes care of that case.
@iignatev This change now passes all automated pre-integration checks. 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 68 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.
|
David, thank you for the review. /integrate |
Going to push as commit 05d64da.
Your commit was automatically rebased without conflicts. |
Hi all,
could you please review the patch that adds
@requires vm.flagless
to 23 hotspot runtime/logging tests that ignore external VM flags?Thanks,
-- Igor
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5075/head:pull/5075
$ git checkout pull/5075
Update a local copy of the PR:
$ git checkout pull/5075
$ git pull https://git.openjdk.java.net/jdk pull/5075/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5075
View PR using the GUI difftool:
$ git pr show -t 5075
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5075.diff