Re: RFR: 8372844: Improve usage of test/jdk/java/text/testlib/TestUtils.java locale methods [v2]
On Tue, 2 Dec 2025 19:14:02 GMT, Justin Lu wrote: >> This PR updates the call sites of `TestUtils::usesGregorianCalendar`, >> `TestUtils::usesAsciiDigits`, and `TestUtils::hasSpecialVariant` to use >> `Assumptions` to properly abort the test. >> >> The existing usage of these methods involve printing to output and returning >> when locale conditions are not met. Some of these tests do lots of printing, >> so identifying when a test is skipped due to inadequate locale conditions >> may not be obvious. Instead of simply printing to output, it would be better >> for test diagnostics to abort the tests, which is easy to identify in the >> Jtreg output. E.g. >> >>> [ JUnit Tests: found 1189, started 1189, succeeded 1185, failed 0, aborted >>> 4, skipped 0] >> >> As a result of this change, Bug4407042, Bug4845901, Bug6530336, and >> LocaleCategory were converted to JUnit based tests. (I could have decided to >> throw jtreg.SkippedException for those tests, but decided to just convert as >> well.) > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Reflect Naoto's comments LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/28590#pullrequestreview-3536085126
Re: RFR: 8372844: Improve usage of test/jdk/java/text/testlib/TestUtils.java locale methods [v2]
> This PR updates the call sites of `TestUtils::usesGregorianCalendar`, > `TestUtils::usesAsciiDigits`, and `TestUtils::hasSpecialVariant` to use > `Assumptions` to properly abort the test. > > The existing usage of these methods involve printing to output and returning > when locale conditions are not met. Some of these tests do lots of printing, > so identifying when a test is skipped due to inadequate locale conditions may > not be obvious. Instead of simply printing to output, it would be better for > test diagnostics to abort the tests, which is easy to identify in the Jtreg > output. E.g. > >> [ JUnit Tests: found 1189, started 1189, succeeded 1185, failed 0, aborted >> 4, skipped 0] > > As a result of this change, Bug4407042, Bug4845901, Bug6530336, and > LocaleCategory were converted to JUnit based tests. (I could have decided to > throw jtreg.SkippedException for those tests, but decided to just convert as > well.) Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Reflect Naoto's comments - Changes: - all: https://git.openjdk.org/jdk/pull/28590/files - new: https://git.openjdk.org/jdk/pull/28590/files/6de6b7fa..ed9f1ba7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=28590&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=28590&range=00-01 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/28590.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/28590/head:pull/28590 PR: https://git.openjdk.org/jdk/pull/28590
Re: RFR: 8372844: Improve usage of test/jdk/java/text/testlib/TestUtils.java locale methods
On Mon, 1 Dec 2025 23:51:11 GMT, Justin Lu wrote:
> This PR updates the call sites of `TestUtils::usesGregorianCalendar`,
> `TestUtils::usesAsciiDigits`, and `TestUtils::hasSpecialVariant` to use
> `Assumptions` to properly abort the test.
>
> The existing usage of these methods involve printing to output and returning
> when locale conditions are not met. Some of these tests do lots of printing,
> so identifying when a test is skipped due to inadequate locale conditions may
> not be obvious. Instead of simply printing to output, it would be better for
> test diagnostics to abort the tests, which is easy to identify in the Jtreg
> output. E.g.
>
>> [ JUnit Tests: found 1189, started 1189, succeeded 1185, failed 0, aborted
>> 4, skipped 0]
>
> As a result of this change, Bug4407042, Bug4845901, Bug6530336, and
> LocaleCategory were converted to JUnit based tests. (I could have decided to
> throw jtreg.SkippedException for those tests, but decided to just convert as
> well.)
Looks good. A couple of comments follow
test/jdk/java/text/Format/DateFormat/Bug4845901.java line 31:
> 29: * time.
> 30: * @library /java/text/testlib
> 31: * @run junit Bug4845901
Specifying `othervm` is safer, as the test sets the default time zone
test/jdk/java/text/Format/DateFormat/Bug6530336.java line 88:
> 86: date.contains("\u07dc\u07ed\u07d5\u07d6") || // N’Ko
> 87: date.contains("\ua2e7\ua0c5\ua395\ua3e6\ua12e\ua209")
> || // Sichuan Yi, Nuosu
> 88:
> date.contains("\u06af\u0631\u06cc\u0646\u06cc\u0686")) { // Central Kurdish
Could use non-escaped strings
-
PR Review: https://git.openjdk.org/jdk/pull/28590#pullrequestreview-3531463420
PR Review Comment: https://git.openjdk.org/jdk/pull/28590#discussion_r2582270468
PR Review Comment: https://git.openjdk.org/jdk/pull/28590#discussion_r2582443358
