<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