JDK-8316696: Remove the testing base classes: IntlTest and CollatorTest #15954

justin-curtis-lu · 2023-09-28T01:02:46Z

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

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

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 f90266f 3c67679

Removes IntlTest in both the testlib and under TimeZone/
Replaces CollatorTest with CollatorTestUtils

Tiers 1-3 clean

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8316696: Remove the testing base classes: IntlTest and CollatorTest (Bug - P4)

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 47532e8a

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15954/head:pull/15954
$ git checkout pull/15954

Update a local copy of the PR:
$ git checkout pull/15954
$ git pull https://git.openjdk.org/jdk.git pull/15954/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15954

View PR using the GUI difftool:
$ git pr show -t 15954

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15954.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-28T01:04:13Z

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-28T01:06:34Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-09-28T01:11:07Z

Webrevs

naotoj

Great to see this old proprietary test framework removed.

naotoj · 2023-09-29T17:30:42Z

test/jdk/java/text/BreakIterator/BreakIteratorTest.java

+    private int getErrorCount() {
+        return errorCount;
+    }
+
+    private void err(String message) {
+        errorCount++;
+        fail(message);
+    }
+


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

I think the previous code was set up so that if forward and backward did not pass, than expected and actual was skipped. Since IntlTest could be ran with -nothrow which allowed for tests to continue even if an error was encountered.

Since that is no longer the case, I simply adjusted compareFragmentLists to fail if an error is found.

naotoj · 2023-09-29T17:50:12Z

test/jdk/java/text/Format/ChoiceFormat/Bug4185732Test.java

+ * @build Bug4185732Test HexDumpReader
+ * @run junit Bug4185732Test
 * @summary test that ChoiceFormat invariants are preserved across serialization
+ *          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).

naotoj · 2023-09-29T18:07:54Z

test/jdk/java/text/Format/NumberFormat/NumberRoundTrip.java

-    public static void main(String[] args) throws Exception {
-        if (args.length > 0 && args[0].equals("-debug")) {
-            DEBUG = true;
-            String[] newargs = new String[args.length - 1];
-            System.arraycopy(args, 1, newargs, 0, newargs.length);
-            args = newargs;
-        }
-        new NumberRoundTrip().run(args);
-    }
-


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

naotoj · 2023-09-29T18:10:25Z

test/jdk/java/text/Format/common/FormatIteratorTest.java

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


Probably this comment needs to be preserved.

naotoj · 2023-09-29T18:18:25Z

test/jdk/java/text/testlib/CollatorTestUtils.java

+ * Collator related tests can use.
 */
-public abstract class CollatorTest extends IntlTest {
+public final class CollatorTestUtils {


Can the utilities in this class be merged into TestUtils?

Merged CollatorTestUtils into TestUtils. I adjusted some method names which were previously in CollatorTestUtils to make more sense, now that they are in TestUtils.

naotoj · 2023-09-29T18:34:45Z

test/jdk/java/util/TimeZone/TimeZoneBoundaryTest.java

      if (inDaylight != lastDST)
      {
-      logln("Switch " + (inDaylight ? "into" : "out of")
+      System.out.println("Switch " + (inDaylight ? "into" : "out of")


I'd remove this commented-out code entirely.

Removed this and the other occurrence you spotted.

naotoj · 2023-09-29T18:35:17Z

test/jdk/java/util/TimeZone/TimeZoneBoundaryTest.java

+          System.out.println("========================================");
+          System.out.println("Stepping using millis");


naotoj · 2023-09-29T22:00:23Z

test/jdk/java/util/Locale/LegacyCodesClassInvariant.java

+                    fail("Locale didn't maintain invariants for: "+lang);
+                    fail("         got: "+loc);
+                    fail("    excpeted: "+expected);


This will only log the message in the first fail() invocation. This may be auto-converted by the script, so you might want to check other places.

Good point, adjusted all locations of this to be contained in one fail() call.

naotoj

LGTM. Thanks for the changes

openjdk · 2023-09-30T00:17:50Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8316696: Remove the testing base classes: IntlTest and CollatorTest

Reviewed-by: naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 32 new commits pushed to the master branch:

b438cff: 8314085: Fixing scope from benchmark to thread for JMH tests having shared state
ae796a4: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi
89987db: 8303773: Replace "main.wrapper" with "test.thread.factory" property in test code
0e501f6: 8308429: jvmti/StopThread/stopthrd007 failed with "NoClassDefFoundError: Could not initialize class jdk.internal.misc.VirtualThreads"
c47a0ce: 8317235: Remove Access API use in nmethod class
353d139: 8317340: Windows builds are not reproducible if MS VS compiler install path differs
3bcfac1: 8317246: Cleanup java.net.URLEncoder and URLDecoder use of file.encoding property
b6a97c0: 8316880: AArch64: "stop: Header is not fast-locked" with -XX:-UseLSE since JDK-8315880
287b243: 8316893: Compile without -fno-delete-null-pointer-checks
26c21f5: 8314294: Unsafe::allocateMemory and Unsafe::freeMemory are slower than malloc/free
... and 22 more: https://git.openjdk.org/jdk/compare/47569a256cb61f210bf2d9f28656fd3fa5ad27b8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

justin-curtis-lu · 2023-10-03T16:37:18Z

/integrate

openjdk · 2023-10-03T16:38:21Z

Going to push as commit b859da9.
Since your change was applied there have been 32 commits pushed to the master branch:

b438cff: 8314085: Fixing scope from benchmark to thread for JMH tests having shared state
ae796a4: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi
89987db: 8303773: Replace "main.wrapper" with "test.thread.factory" property in test code
0e501f6: 8308429: jvmti/StopThread/stopthrd007 failed with "NoClassDefFoundError: Could not initialize class jdk.internal.misc.VirtualThreads"
c47a0ce: 8317235: Remove Access API use in nmethod class
353d139: 8317340: Windows builds are not reproducible if MS VS compiler install path differs
3bcfac1: 8317246: Cleanup java.net.URLEncoder and URLDecoder use of file.encoding property
b6a97c0: 8316880: AArch64: "stop: Header is not fast-locked" with -XX:-UseLSE since JDK-8315880
287b243: 8316893: Compile without -fno-delete-null-pointer-checks
26c21f5: 8314294: Unsafe::allocateMemory and Unsafe::freeMemory are slower than malloc/free
... and 22 more: https://git.openjdk.org/jdk/compare/47569a256cb61f210bf2d9f28656fd3fa5ad27b8...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-03T16:38:33Z

@justin-curtis-lu Pushed as commit b859da9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

justin-curtis-lu added 3 commits September 27, 2023 15:16

scripted changes

c0ece01

manual changes

9a54910

remove IntlTest and CollatorTest. introduce CollatorUtils

8ee9f9c

openjdk bot added the rfr Pull request is ready for review label Sep 28, 2023

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Sep 28, 2023

justin-curtis-lu added 2 commits September 28, 2023 10:20

CollatorUtils -> CollatorTestUtils

f90266f

cleanup util classes in text/testlib

3c67679

naotoj reviewed Sep 29, 2023

View reviewed changes

justin-curtis-lu added 2 commits September 29, 2023 14:04

implement feedback

6e3bf11

merge master and resolve conflicts

95488eb

naotoj reviewed Sep 29, 2023

View reviewed changes

get rid of subsequent fail()s

47532e8

naotoj approved these changes Sep 30, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 30, 2023

typo in import

d20b085

openjdk bot added the integrated Pull request has been integrated label Oct 3, 2023

openjdk bot closed this Oct 3, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 3, 2023

		System.out.println("========================================");
		System.out.println("Stepping using millis");

JDK-8316696: Remove the testing base classes: IntlTest and CollatorTest #15954

JDK-8316696: Remove the testing base classes: IntlTest and CollatorTest #15954

Uh oh!

Conversation

justin-curtis-lu commented Sep 28, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 28, 2023

Uh oh!

openjdk bot commented Sep 28, 2023

Uh oh!

mlbridge bot commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naotoj Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justin-curtis-lu commented Oct 3, 2023

Uh oh!

openjdk bot commented Oct 3, 2023

Uh oh!

openjdk bot commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

justin-curtis-lu commented Sep 28, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 28, 2023 •

edited

Loading

naotoj Sep 29, 2023 •

edited

Loading

openjdk bot commented Sep 30, 2023 •

edited

Loading