<i18n dev> RFR: JDK-8316696: Remove the testing base classes: IntlTest and CollatorTest [v3]

[Naoto Sato]

On Thu, 28 Sep 2023 18:13:12 GMT, Justin Lu <jlu at openjdk.org> wrote:

>> Please review this PR which removes the i18n related testing base classes `IntlTest` and `CollatorTest` and converts all the tests that use them,
>> 
>> IntlTest and CollatorTest are testing classes which are extended by tests in `text/`, `util/Locale`, `util/TimeZone`, and `util/Calendar`. The abstract testing classes are quite dated and have caused issues such as: variation between OS, hiding stack trace, and causing tests to spuriously pass.
>> 
>> This change mainly automates a low level conversion of all the tests (75) using the frameworks; all tests were converted to use JUnit instead. (With the exception of `DateFormatRoundTripTest` due to the nature of the test).
>> 
>> The main changes can be viewed in the following commits
>> 
>> scripted changes - [c0ece01 ](https://github.com/openjdk/jdk/commit/c0ece01e91479a020d5c6dce937dc827472b763b)
>> - Converts the IntlTest methods logln, log, err, and errln
>> - Adds the JUnit Test annotation to tests that should be ran
>> - Adjusts the Jtreg tags accordingly
>> - Remove the main method
>> - Insert initAll() methods for tests that previously adjusted the JVM default Locale/TimeZone in the main method
>> 
>> manual changes - [9a54910](https://github.com/openjdk/jdk/commit/9a5491065a94a4dc7a05194f3b8330efba8077b7)
>> - Some tests that had extensive logic in the main method or did not follow the general IntlTest format had to be manually adjusted
>> - Also clarified some tests that had optional argument setup in the main method (now removed)
>> 
>> removal of IntlTest and CollatorTest - [8ee9f9c](https://github.com/openjdk/jdk/commit/8ee9f9c79f79210ee6244186970d87a1cac05556) [f90266f](https://github.com/openjdk/jdk/pull/15954/commits/f90266f4f019c031c5f985dd658f62a02cc8b422) [3c67679](https://github.com/openjdk/jdk/pull/15954/commits/3c676794dd0c703db066a346c36e213d113d9acc)
>> - Removes IntlTest in both the testlib and under TimeZone/
>> - Replaces CollatorTest with CollatorTestUtils
>> 
>> Tiers 1-3 clean
>
> Justin Lu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cleanup util classes in text/testlib

Great to see this old proprietary test framework removed.

test/jdk/java/text/BreakIterator/BreakIteratorTest.java line 112:

> 110:         fail(message);
> 111:     }
> 112: 

We could simply eliminate these, by making `compareFragmentLists` return success indicator.

test/jdk/java/text/Format/ChoiceFormat/Bug4185732Test.java line 31:

> 29:  * @run junit Bug4185732Test
> 30:  * @summary test that ChoiceFormat invariants are preserved across serialization
> 31:  *          Run prepTest() if the invariant files are not yet generated.

I presume those invariant files are already in the test directory. I would simply remove this `prepTest()` altogether (also seen in other tests).

test/jdk/java/text/Format/NumberFormat/NumberRoundTrip.java line 62:

> 60:         new NumberRoundTrip().run(args);
> 61:     }
> 62: 

Probably we could eliminate `DEBUG`, and always print the information.

test/jdk/java/text/Format/common/FormatIteratorTest.java line 117:

> 115:             // The current tests are only appropriate for US. If tests are
> 116:             // added for other locales are added, then a property should be
> 117:             // added to each file (test) to be able to specify the locale.

Probably this comment needs to be preserved.

test/jdk/java/text/testlib/CollatorTestUtils.java line 32:

> 30:  * Collator related tests can use.
> 31:  */
> 32: public final class CollatorTestUtils {

Can the utilities in this class be merged into `TestUtils`?

test/jdk/java/util/TimeZone/TimeZoneBoundaryTest.java line 343:

> 341:       if (inDaylight != lastDST)
> 342:       {
> 343:       System.out.println("Switch " + (inDaylight ? "into" : "out of")

I'd remove this commented-out code entirely.

test/jdk/java/util/TimeZone/TimeZoneBoundaryTest.java line 398:

> 396:           {
> 397:           System.out.println("========================================");
> 398:           System.out.println("Stepping using millis");

This too

-------------

PR Review: https://git.openjdk.org/jdk/pull/15954#pullrequestreview-1651224754
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341638631
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341653934
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341667327
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341669151
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341675031
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341688583
PR Review Comment: https://git.openjdk.org/jdk/pull/15954#discussion_r1341689009