On Thu, 15 Jun 2023 21:58:56 GMT, Naoto Sato <[email protected]> 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
