RFR: 8310234: Refactor Locale tests to use JUnit [v3]
Justin Lu
jlu at openjdk.org
Thu Jun 22 19:32:00 UTC 2023
On Thu, 22 Jun 2023 18:47:06 GMT, Naoto Sato <naoto at openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with four additional commits since the last revision:
>>
>> - Method signature is too long
>> - Exceptions in GetInstanceCheck.java are distinct, catch clause should be reserved for the underlying exception
>> - Rename method to be more clear in CaseCheckVariant.java
>> - Method name case in AvailableLocalesTest.java
>
> 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.
Unless I am mistaken, both bug IDs, 4122700 8282319 are in the combined file.
> test/jdk/java/util/Locale/AvailableLocalesTest.java line 61:
>
>> 59: */
>> 60: @Test
>> 61: public void StreamEqualsArrayTest() {
>
> Method names should start with a lowercase character
Missed that, thank you.
> 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.
That's a better name, fixed.
> 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`?
Yes you're right, that's a good point. Reserved the catch clause to handle `InvocationTargetException` and moved `IllegalAccessException` to the method signature.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238940849
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238940976
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238941073
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238942899
More information about the core-libs-dev
mailing list