On Thu, 15 Jun 2023 20:48:38 GMT, Justin Lu <[email protected]> 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
