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