On Wed, 28 Jun 2023 20:49:18 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Please review this PR which refactors Currency tests to use JUnit.
>>
>> The most significant change occurs in `ValidateISO4217.java`. Other changes
>> to this file excluding the JUnit refactoring include
>>
>> - Tests are no longer dependent on each other (order of execution does not
>> matter)
>> - Testing does not occur at the same time as data generation (The data is
>> fully generated before any tests are executed)
>> - Various cleanup (dead-code, clarifying comments, more descriptive method
>> and var names)
>>
>> `Bug4512215.java` now renamed to `MinorUndefinedCodes` was updated to remove
>> redundant testing, and the file changed to focus on testing minor undefined
>> currency codes instead.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove unusued JUnit imports from ValidateISO4217.java
test/jdk/java/util/Currency/CurrencyNameProviderTest.java line 43:
> 41: import static org.junit.jupiter.api.Assertions.assertThrows;
> 42:
> 43: public class CurrencyNameProviderTest {
Since this is not a generic CurrencyNameProvider test, but specific to the
default impl of CNP.getDisplayName(), the class name could be more specific.
test/jdk/java/util/Currency/CurrencyTest.java line 81:
> 79: }
> 80:
> 81: // Calling getInstance() with an illegal name should throw an IAE
illegal name -> invalid currency code
test/jdk/java/util/Currency/CurrencyTest.java line 241:
> 239: Arguments.of("JOD", 3), Arguments.of("KWD", 3),
> 240: Arguments.of("LYD", 3), Arguments.of("OMR", 3),
> 241: Arguments.of("TND", 3), Arguments.of("TRL", 0), //
> Turkish Lira
The turkish lira comment should apply to both TRL and TRY
test/jdk/java/util/Currency/CurrencyTest.java line 321:
> 319: Currency currency2 = Currency.getInstance(currencyCode);
> 320: assertEquals(currency1, currency2, "Didn't get same instance for
> same currency code");
> 321: assertEquals(currency1.getCurrencyCode(), currencyCode,
> "Currency code changed");
Error message more like "wrong currency code", than "changed"
test/jdk/java/util/Currency/MinorUndefinedCodes.java line 27:
> 25: * @test
> 26: * @bug 4512215 4818420 4819436 8310923
> 27: * @summary Test some minor undefined currencies.
It'd be more readable something like "currencies without minor units." The
class name can also be renamed to something like NoMinorUnitCurrenciesTest
test/jdk/java/util/Currency/ValidateISO4217.java line 82:
> 80: // Codes derived from ISO4217 golden-data file
> 81: private static final List<Arguments> ISO4217Codes = new
> ArrayList<Arguments>();
> 82: // Additional codes not from the ISO4217 golden-data file
Are these from the golden file?
test/jdk/java/util/Currency/ValidateISO4217.java line 96:
> 94: setUpISO4217Codes();
> 95: setUpAdditionalCodes();
> 96: finishTestCurrencies();
The method name `finish` is kind of confusing. I'd explicitly describe what the
method does, i.e, setup `other` currencies.
test/jdk/java/util/Currency/ValidateISO4217.java line 192:
> 190: /* Defined neither in ISO 4217 nor ISO 3166 list */
> 191: {"XK", "EUR", "978", "2"}, // Kosovo
> 192: };
I'd prefer this array to be placed as static in the class, not within the
method.
test/jdk/java/util/Currency/ValidateISO4217.java line 214:
> 212: +
> "PTE-ROL-RUR-SDD-SIT-SLL-SKK-SRG-STD-TMM-TPE-TRL-VEF-UYI-USN-USS-VEB-VED-"
> 213: +
> "XAG-XAU-XBA-XBB-XBC-XBD-XDR-XFO-XFU-XPD-XPT-XSU-XTS-XUA-XXX-"
> 214: + "YUM-ZMK-ZWD-ZWN-ZWR";
Ditto as `extraCodes`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247234295
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247239767
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247238613
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247237567
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247247681
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247255026
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247253772
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247254253
PR Review Comment: https://git.openjdk.org/jdk/pull/14682#discussion_r1247254528
