<i18n dev> RFR: 8311663: Additional refactoring of Locale tests to JUnit

Naoto Sato naoto at openjdk.org
Fri Jul 14 22:39:15 UTC 2023


On Thu, 13 Jul 2023 23:23:42 GMT, Justin Lu <jlu at openjdk.org> 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


More information about the i18n-dev mailing list