8379195: Refactor Arrays TestNG tests to use JUnit#30111
8379195: Refactor Arrays TestNG tests to use JUnit#30111rgiulietti wants to merge 3 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back rgiulietti! A progress list of the required criteria for merging this PR into |
|
@rgiulietti 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 66 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 |
|
@rgiulietti The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
|
||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) |
There was a problem hiding this comment.
Just curious why this is needed as the method source is a static method.
There was a problem hiding this comment.
This test was converted by an automated tool, and this line was added by it.
I agree that it is not that useful in this case, but doesn't seem to hurt either.
There was a problem hiding this comment.
Just a passing concern that some future maintainer will wonder if these tests have something subtle to require them to run with the same test case instance.
If you choose to not address it, can you file a JBS issue to remove them?
There was a problem hiding this comment.
I assume a deeper migration would change this to assertThrows(UnsupportedOperationException.class, itr::remove).
There was a problem hiding this comment.
In passing, I assume this assertFalse is redundant as it is checked in the loop.
There was a problem hiding this comment.
I see it as redundant as well, but it is harmless.
| @@ -158,7 +160,8 @@ public void testParallelPrefixForInt(int[] data, int fromIndex, int toIndex, Int | |||
| assertArraysEqual(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); | |||
There was a problem hiding this comment.
With JUnit, the first parameter to assertArraysEquals is the "expected" as migration from TestNG will usually means transposing these parameters. It's really only an issue if there is failure of course, and only leads to a confusing message.
There was a problem hiding this comment.
The assertArraysEqual() in this class are not JUnit methods. It's their implementations that eventually invoke the real JUnit method, with expected and actual in the conventional JUnit order.
There was a problem hiding this comment.
I get that, I'm just saying its a hazard to have these wrappers follow the TestNG ordering and then transpose to use the JUnit ordering.
There was a problem hiding this comment.
I see you've fixed this - thanks!
|
/integrate |
|
Going to push as commit f1be820.
Your commit was automatically rebased without conflicts. |
|
@rgiulietti Pushed as commit f1be820. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
A migration from TestNG to JUnit
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30111/head:pull/30111$ git checkout pull/30111Update a local copy of the PR:
$ git checkout pull/30111$ git pull https://git.openjdk.org/jdk.git pull/30111/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30111View PR using the GUI difftool:
$ git pr show -t 30111Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30111.diff
Using Webrev
Link to Webrev Comment