On Thu, 22 Jun 2023 18:55:31 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Please review this PR as apart of >> [JDK-8307843](https://bugs.openjdk.org/browse/JDK-8307843) which refactors >> some tests in Locale to use JUnit. Other cleanup and small changes are >> included as well. More refactoring in Locale tests will be done in separate >> PRs. >> >> If the test had a bugNNNNN.java name, it was also renamed to something more >> [descriptive](https://openjdk.org/jtreg/faq.html#how-should-i-name-a-test). >> >> Below is a list of all the changes, >> >> - Refactor Bug4316602.java as LocaleConstructors.java >> - Refactor Bug4210525.java as CaseCheckVariant.java >> - Refactor bug6277243.java as RootLocale.java >> - Refactor bug6312358.java as GetInstanceCheck.java >> - Refactor Bug8154797.java as CompareProviderFormats.java >> - Refactor Bug8004240.java as GetAdapterPreference.java >> - Refactor bug4122700.java into AvailableLocalesTest.java (and combined with >> StreamAvailableLocales.java) > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Review: Clarify LocaleConstructors.java with comment for each test method test/jdk/java/util/Locale/AvailableLocalesTest.java line 46: > 44: /** > 45: * Test that Locale.getAvailableLocales() is non-empty. > 46: * Print out the locales. If you combine two tests into a single one, I'd put the corresponding bug id into the method description. test/jdk/java/util/Locale/AvailableLocalesTest.java line 61: > 59: */ > 60: @Test > 61: public void StreamEqualsArrayTest() { Method names should start with a lowercase character test/jdk/java/util/Locale/CaseCheckVariant.java line 50: > 48: @ParameterizedTest > 49: @MethodSource("variants") > 50: public void variantNormalizationTest(String variant) { "Normalization" -> "Case"? Locale does not normalize the variant. test/jdk/java/util/Locale/GetInstanceCheck.java line 79: > 77: } catch (InvocationTargetException | IllegalAccessException exc) { > 78: Throwable cause = exc.getCause(); > 79: if (exc.getCause() instanceof NullPointerException) { Should this condition only be meaningful for `InvocationTargetException`? test/jdk/java/util/Locale/GetInstanceCheck.java line 99: > 97: } catch (InvocationTargetException | IllegalAccessException exc) { > 98: Throwable cause = exc.getCause(); > 99: if (cause instanceof NullPointerException) { Same as above ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238910004 PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238912059 PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238915580 PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238927882 PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238927101
