On Thu, 28 Sep 2023 18:13:12 GMT, Justin Lu <j...@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
