RFR: 8310923: Refactor Currency tests to use JUnit [v4]

[Naoto Sato]

On Wed, 28 Jun 2023 20:49:18 GMT, Justin Lu <jlu at 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