<i18n dev> RFR: 8310049: Refactor Charset tests to use JUnit [v3]

Naoto Sato naoto at openjdk.org
Thu Jun 15 22:29:06 UTC 2023


On Thu, 15 Jun 2023 20:48:38 GMT, Justin Lu <jlu at openjdk.org> wrote:

>> 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. 
>> 
>> In addition, this PR groups the emptyCharset, nullCharset, and defaultCharset tests into _illegalCharsets.java_. The _default.java_ file was removed, as it did not test anything.
>
> Justin Lu has updated the pull request incrementally with eight additional commits since the last revision:
> 
>  - Review: Clarify RegisteredCharsets.java and add comments to test methods
>  - Minor cleanup
>  - Refactor IllegalCharsetName.java to use method source
>  - Update EncDec.java to be more informative + cautious
>  - Update data source: other -> standard Charsets
>  - Review: Add comments to Contains.java to explain each test
>  - Review: Add comment for test in AvailableCharsetNames.java
>  - Review: Make CharsetContainmentTest.java data source and test method more clear

Thanks for the clean-up. Some comments follow:

test/jdk/java/nio/charset/Charset/Contains.java line 51:

> 49:     @ParameterizedTest
> 50:     @MethodSource("standardCharsets")
> 51:     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.

test/jdk/java/nio/charset/Charset/Default.java line 1:

> 1: /*

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

test/jdk/java/nio/charset/Charset/IllegalCharsetName.java line 51:

> 49:         assertThrows(UnsupportedCharsetException.class,
> 50:                 () -> Charset.forName("default"));
> 51:     }

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

-------------

PR Review: https://git.openjdk.org/jdk/pull/14500#pullrequestreview-1482442857
PR Review Comment: https://git.openjdk.org/jdk/pull/14500#discussion_r1231572797
PR Review Comment: https://git.openjdk.org/jdk/pull/14500#discussion_r1231585756
PR Review Comment: https://git.openjdk.org/jdk/pull/14500#discussion_r1231594567


More information about the i18n-dev mailing list