On Tue, 3 Oct 2023 22:23:11 GMT, Justin Lu <j...@openjdk.org> wrote:
> Please review this PR which refactors a number of tests under
> `test/text/NumberFormat` to use JUnit.
>
> During the switch to JUnit, the tests had the following updates (to improve
> readability)
> - separate the test data generation from the actual execution of the test
> - create distinct test methods so that all the tests aren't just run under
> one big method (e.g. main)
test/jdk/java/text/Format/NumberFormat/BigDecimalCompatibilityTest.java line
162:
> 160: // Check parse and returned value
> 161: Number parsedValue = assertDoesNotThrow(()->
> df.parse(longString),
> 162: "Should not throw a ParseException");
It could catch any exception, so the message could be broader. Maybe just `a
ParseException` -> `an Exception`.
test/jdk/java/text/Format/NumberFormat/Bug4838107.java line 254:
> 252: "\n\tre-parsed number: " + parsed2 +
> 253: " (" + parsed2.getClass().getName() + ")" +
> 254: "\n\tminus sign: " +
> df.getDecimalFormatSymbols().getMinusSign());
Could use text block to get rid of `\n`/`\t`s, with `.formatted()`. Same for
the following one.
test/jdk/java/text/Format/NumberFormat/Bug4944439.java line 71:
> 69: public void parseLongTest(String s) {
> 70: Number parsedNumber = assertDoesNotThrow(() -> df.parse(s),
> 71: "DecimalFormat.parse(\"%s\") should not throw
> ParseException");
`ParseException` -> `Exception`. Applies to other locations
test/jdk/java/text/Format/NumberFormat/Bug4990596.java line 45:
> 43: public void formatSubclassedNumberTest() {
> 44: assertDoesNotThrow(() -> new DecimalFormat().format(new
> MutableInteger(0)),
> 45: "DecimalFormat.parse() should support subclasses of
> Number");
DecimalFormat.format()?
test/jdk/java/text/Format/NumberFormat/Bug8132125.java line 47:
> 45: NumberFormat nf = NumberFormat.getInstance(deCH);
> 46:
> 47: // i.e. "\u2019" as decimal separator, "\u2019" as grouping
> separator
This is incorrect (sorry, I am to be blamed as I wrote this 😄). Should be
"`\u002E` as decimal separator, " also remove `i.e.` if you move the comment on
top of the variable declaration
test/jdk/java/text/Format/NumberFormat/CurrencyFormat.java line 62:
> 60: // currencySymbolsTest() is only ran for COMPAT
> 61: private static final boolean isCompat =
> 62: "COMPAT".equals(System.getProperty("java.locale.providers"));
This check could be moved into `currencySymbolsTest()`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1350752425
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1350835828
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1350845412
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1350862874
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1350883202
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1350899278
