8381439: [lworld] Extends random values tests to value containers#2291
8381439: [lworld] Extends random values tests to value containers#2291fparain wants to merge 5 commits intoopenjdk:lworldfrom
Conversation
|
👋 Welcome back fparain! A progress list of the required criteria for merging this PR into |
|
@fparain 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 142 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 |
Webrevs
|
Arraying
left a comment
Arraying
left a comment
There was a problem hiding this comment.
Thanks! This is something that is very valuable to have. The changes seem fine, I'm just concerned about a possibly non-terminating loop. Otherwise minor comments.
| Asserts.assertEquals(v, <value_class_name>.getPredefinedValue(3)); | ||
|
|
||
| // Testing field nrv | ||
| npe = null; |
There was a problem hiding this comment.
Nit: redundant. Also subsequent usages.
There was a problem hiding this comment.
They are intentionally redundant, so we can change individual tests or insert a new one without having to care about the status of this variable. I've considered put each test in an individual method, but it adds a lot of boilerplate for not a lot of benefits.
| c.testFieldsViaReflection(); | ||
| } | ||
|
|
||
| void testFields() { |
There was a problem hiding this comment.
Is there a reason that you check for nullability of the exceptions? I think using the Assertions::assertDoesNotThrow would be more robust and reduces code size significantly.
There was a problem hiding this comment.
The tests are not using JUnit.
Arraying
left a comment
Arraying
left a comment
There was a problem hiding this comment.
The changes since last review look good, thank you!
|
Thanks @Arraying for the review. |
|
Going to push as commit 13de33a.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
2 participants
This patch adds a new test with identity objects containing values from randomly generated values classes.
The test checks field accesses through bytecodes and through reflection, and also some layout verification.
The ValueClassGenerator has been extended to generate LooselyConsistent annotated values classes, as well as null-restricted fields (automatically used by previous tests using the ValueClassGenerator).
The patch also includes various cleanup in the ValueClassGenerator and related test files.
This patch contains the fix for JDK-8381213 (this bug was preventing some of the new tests from passing).
New and updated tests have been tested on Mach5 (all standard platforms).
Thanks,
Fred
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2291/head:pull/2291$ git checkout pull/2291Update a local copy of the PR:
$ git checkout pull/2291$ git pull https://git.openjdk.org/valhalla.git pull/2291/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2291View PR using the GUI difftool:
$ git pr show -t 2291Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2291.diff
Using Webrev
Link to Webrev Comment