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

Justin Lu jlu at openjdk.org
Thu Jun 15 23:09:24 UTC 2023


On Thu, 15 Jun 2023 21:58:56 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> 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
>
> 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.

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).

> 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`.

I moved it back to RegisteredCharsets.java

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14500#discussion_r1231612571
PR Review Comment: https://git.openjdk.org/jdk/pull/14500#discussion_r1231612904


More information about the i18n-dev mailing list