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