8310049: Refactor Charset tests to use JUnit by justin-curtis-lu · Pull Request #14500 · openjdk/jdk

justin-curtis-lu · 2023-06-15T19:42:56Z

As discussed in https://github.com/openjdk/jdk/pull/14473/files, tests within test/jdk/java/nio/charset/Charset could benefit from using a test framework such as JUnit.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8310049: Refactor Charset tests to use JUnit (Sub-task - P4)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14500/head:pull/14500
$ git checkout pull/14500

Update a local copy of the PR:
$ git checkout pull/14500
$ git pull https://git.openjdk.org/jdk.git pull/14500/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14500

View PR using the GUI difftool:
$ git pr show -t 14500

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14500.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-06-15T19:43:57Z

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-06-15T19:45:13Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

i18n
nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-06-15T19:49:36Z

Webrevs

LanceAndersen

Thanks for starting this migration/refactor.

I would use this as an opportunity to add additional comments introducing the tests and the relevant parameterized lists et al

LanceAndersen · 2023-06-15T19:52:55Z

test/jdk/java/nio/charset/Charset/AvailableCharsetNames.java


 /* @test
- * @bug 4422044
+ * @bug 4422044 8310049


As this is just a refactor, I don't think you need the bug number for converting to junit

Fixed, in this file and the others

…more clear

naotoj

Thanks for the clean-up. Some comments follow:

naotoj · 2023-06-15T21:58:56Z

test/jdk/java/nio/charset/Charset/Contains.java

+     */
+    @ParameterizedTest
+    @MethodSource("standardCharsets")
+    public void standardCharsetsTest(Charset containerCs, Charset cs, boolean cont){


ISO-8859-15 and CP1252 are not StandardCharsets so renaming the method to standardCharsetsTest seems incorrect.

Did not realize, thank you. I simply renamed it to charsets (as I am not sure if those tested are supposed to represent a particular group, or if they are just random).

naotoj · 2023-06-15T22:11:31Z

test/jdk/java/nio/charset/Charset/Default.java

@@ -1,36 +0,0 @@
-/*


This class is used in DefaultCharsetTest so don't remove it.

Fixed, thank you

naotoj · 2023-06-15T22:25:36Z

test/jdk/java/nio/charset/Charset/IllegalCharsetName.java

+    // Charset.forName should throw an exception when passed "default"
+    @Test
+    public void defaultCharsetTest() {
+        assertThrows(UnsupportedCharsetException.class,
+                () -> Charset.forName("default"));
+    }


Seems incorrect to merge this test into IllegalCharsetName.java because it is throwing UnsupportedCharsetException.

I moved it back to RegisteredCharsets.java

This reverts commit 346cb4c.

naotoj · 2023-06-16T17:36:16Z

test/jdk/java/nio/charset/Charset/IllegalCharsetName.java

+    // Charset.forName should throw an exception when passed null
+    @Test
+    public void nullCharsetTest() {
+        assertThrows(IllegalArgumentException.class,
+                () -> Charset.forName(null));
+    }


Missed this one previously. This one is also not IllegalCharsetName, so I'd prefer not to merge this either.

I guess when I think of illegal (I group anything that is wrong, so I initially included null and "default"). But you are right that it does not throw IllegalCharsetNameException so it should be moved into its own test. Fixed.

openjdk · 2023-06-16T18:34:17Z

@justin-curtis-lu 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:

8310049: Refactor Charset tests to use JUnit

Reviewed-by: lancea, naoto

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 25 new commits pushed to the master branch:

16134f4: 8310211: serviceability/jvmti/thread/GetStackTrace/getstacktr03/getstacktr03.java failing
d6c2ee3: 8310054: ScrollPane insets are incorrect
4eb4f20: 8308808: SunMSCAPI public keys returns internal key array
53abba3: 8309499: javac fails to report compiler.err.no.java.lang with annotation processing enabled
cf8d0b0: 8309964: Use directed inheritDoc for javax.lang.model API
a45f929: 8310076: Reduce inclusion of bytecodeStream.hpp
21f6d83: 8309471: Limit key characters in static index pages
44a8aa0: 8308609: java/lang/ScopedValue/StressStackOverflow.java fails with "-XX:-VMContinuations"
b412fc7: 8309937: Add @sealedGraph for some Panama FFM interfaces
fb024fd: 8248149: G1: change _cleaning_claimed from int to bool
... and 15 more: https://git.openjdk.org/jdk/compare/3e0bbd290c534b0f9729c54cd45308d505907797...master

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 master branch, type /integrate in a new comment.

justin-curtis-lu · 2023-06-20T17:20:06Z

/integrate

openjdk · 2023-06-20T17:21:04Z

Going to push as commit 09174e0.
Since your change was applied there have been 54 commits pushed to the master branch:

99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
4a9cc8a: 8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer
4e4e586: 8310194: Generational ZGC: Lock-order asserts in JVMTI IterateThroughHeap
e1906e7: 8310027: Fix -Wconversion warnings in nmethod and compiledMethod related code
4ca548f: 8310326: Incorrect position of the synthetic unnamed class
a059576: 8310187: Improve Generational ZGC jtreg testing
9a68ec8: 8219357: G1: G1GCPhaseTimes::debug_phase uses unnecessary ResourceMark
0878872: 8310020: MacroAssembler::call_VM(_leaf) doesn't consistently check for conflict with C calling convention.
79069c5: 8310314: Misplaced "unnamed classes are a preview feature and are disabled by default" error
... and 44 more: https://git.openjdk.org/jdk/compare/3e0bbd290c534b0f9729c54cd45308d505907797...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-20T17:21:12Z

@justin-curtis-lu Pushed as commit 09174e0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

justin-curtis-lu added 15 commits June 14, 2023 11:16

Remove Default test

346cb4c

Combine Null test with Illegal test, refactor to use JUnit

cc36299

Test all charset aliases

1b8c9f6

Refactor AvailableCharsetNames

e773729

Combine EmptyCharset test with IllegalCharset test

ec2626b

Refactor EncDec test to use JUnit

3078a52

Move default charset test to IllegalCharsetName.java

5260c57

Refactor RegisteredCharsets.java

b531962

Do not use wildcard import in IllegalCharsetName.java

528379d

Add bug id to EncDec.java

4eb22f3

Move bug id for default charset test

0853166

Refactor contains.java

d5caa0f

Remove unusued JUnit import in RegisteredCharsets.java

cd9ddb7

Refactor CharsetContainmentTest.java

cc69404

Merge master and fix conflicts in Contains.java

be352ca

openjdk bot added the rfr Pull request is ready for review label Jun 15, 2023

openjdk bot added nio nio-dev@openjdk.org i18n i18n-dev@openjdk.org labels Jun 15, 2023

LanceAndersen reviewed Jun 15, 2023

View reviewed changes

justin-curtis-lu added 8 commits June 15, 2023 12:58

Review: Refactoring does not require bugID in test

eb374d6

Review: Make CharsetContainmentTest.java data source and test method …

b498e59

…more clear

Review: Add comment for test in AvailableCharsetNames.java

3cd9455

Review: Add comments to Contains.java to explain each test

34f89c0

Update data source: other -> standard Charsets

7bac44c

Update EncDec.java to be more informative + cautious

93a925e

Refactor IllegalCharsetName.java to use method source

243d152

Minor cleanup

6de26fe

Review: Clarify RegisteredCharsets.java and add comments to test methods

6c7a8d0

naotoj reviewed Jun 15, 2023

View reviewed changes

justin-curtis-lu added 3 commits June 15, 2023 15:46

Revert "Remove Default test"

bdd22b9

This reverts commit 346cb4c.

Review: rename standard charsets test

467703d

Review: Move default test back to RegisteredCharsets.java

6a39cea

naotoj reviewed Jun 16, 2023

View reviewed changes

Review: Make null charset its own test

bc50c6f

naotoj approved these changes Jun 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 16, 2023

LanceAndersen approved these changes Jun 16, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 20, 2023

openjdk bot closed this Jun 20, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 20, 2023

Conversation

justin-curtis-lu commented Jun 15, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 15, 2023

Uh oh!

openjdk bot commented Jun 15, 2023

Uh oh!

mlbridge bot commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justin-curtis-lu commented Jun 20, 2023

Uh oh!

openjdk bot commented Jun 20, 2023

Uh oh!

openjdk bot commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

justin-curtis-lu commented Jun 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 15, 2023 •

edited

Loading

openjdk bot commented Jun 16, 2023 •

edited

Loading