On Thu, 13 Jul 2023 23:23:42 GMT, Justin Lu <[email protected]> wrote:
> Please review this PR which refactors more java.util.Locale tests to JUnit
> with some minor cleanup as well.
>
> Although some of the files could benefit from being renamed bugNNNNNNN to
> something more descriptive, this makes reviewing harder, and will be handled
> separately.
Thanks for the conversion, Justin. Looks good overall. Some minor comments
follow.
test/jdk/java/util/Locale/Bug8135061.java line 56:
> 54: assertNull(match, "[Locale.lookup failed on language"
> 55: + " range: " + ranges + " and language tags "
> 56: + locales + "]");
Removing try-catch will lose the information on what kind of exception was
thrown
test/jdk/java/util/Locale/Bug8135061.java line 70:
> 68: assertEquals(match.toLanguageTag(), "nv", "[Locale.lookup failed
> on language"
> 69: + " range: " + ranges + " and language tags "
> 70: + locales + "]");
Same as above
test/jdk/java/util/Locale/Bug8159420.java line 67:
> 65: @BeforeAll
> 66: static void setTurkishLocale() {
> 67: Locale.setDefault(Locale.of("tr", "TR"));
Should the tests run under `othervm` mode?
test/jdk/java/util/Locale/Bug8179071.java line 94:
> 92: .map(Locale::toLanguageTag)
> 93: .forEach(tag -> {if(LegacyAliases.contains(tag))
> {invalidTags.add(tag);}});
> 94: assertEquals(true, invalidTags.isEmpty(),
`assertTrue()` may fit better here
test/jdk/java/util/Locale/ThaiGov.java line 51:
> 49: // Test number formatting for thai
> 50: @Test
> 51: public void numberTest() throws RuntimeException {
Not your change, but this `throws` is redundant
-------------
PR Review: https://git.openjdk.org/jdk/pull/14881#pullrequestreview-1531097899
PR Review Comment: https://git.openjdk.org/jdk/pull/14881#discussion_r1264205170
PR Review Comment: https://git.openjdk.org/jdk/pull/14881#discussion_r1264205697
PR Review Comment: https://git.openjdk.org/jdk/pull/14881#discussion_r1264209622
PR Review Comment: https://git.openjdk.org/jdk/pull/14881#discussion_r1264216543
PR Review Comment: https://git.openjdk.org/jdk/pull/14881#discussion_r1264218988
