On Mon, 10 Oct 2022 18:18:55 GMT, Bill Huang <bhu...@openjdk.org> wrote:
> The jarsigner and keytool are localized into English, German, Japanese and > Simplified Chinese. This task is to modify the existing i18n tests to > validate i18n compliance in these tools. > > In addition, this task also contains changes for manual test enhancement and > simplification which originated from > [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663. Looks good overall. Some minor suggestions. test/jdk/sun/security/tools/keytool/i18n.java line 62: > 60: * @library /test/lib > 61: * @run main/manual/othervm i18n zh CN > 62: */ Do you need to triplicate these `@test` tags? Would 3-lines of `@run` suffice? Also setting the locale by `-Duser.language/country` and `getProperty` them in the main would be preferable to passing them as the test case arguments. test/jdk/sun/security/tools/keytool/i18n.java line 250: > 248: private Thread currentThread = null; > 249: > 250: public static class DialogBuilder { This seems to be a boilerplate for displaying the panel. Could this be separated from the test and converted into some library? test/jdk/sun/security/tools/keytool/i18n.java line 334: > 332: } else if (args.length == 2) { > 333: Locale.setDefault(Locale.of(args[0], args[1])); > 334: } Can be eliminated with the suggestion above. test/jdk/sun/security/tools/keytool/i18n.java line 335: > 333: Locale.setDefault(Locale.of(args[0], args[1])); > 334: } > 335: final String LANG = Locale.getDefault().getDisplayLanguage(); Instead of `getDisplayLanguage()`, it should issue `getDisplayName()`, as for `zh-CN` case, it simply displays `Chinese` in the current impl. It's ambiguous whether it is simplified or traditional. Also, `LANG` should be lowercase, as it is not a constant. ------------- PR: https://git.openjdk.org/jdk/pull/10635
