8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher #10715

justin-curtis-lu · 2022-10-14T20:38:32Z

Issue: Formatter unit tests are launched via basic.sh

Fix: Replace basic.sh with a Java test launcher

Note: Java.internal.math was included in the original configuration of Basic, but I removed it as it was not used within the Basic unit tests

Original output on success

New output on success

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-8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher

Reviewers

Lance Andersen (@LanceAndersen - Reviewer) ⚠️ Review applies to 676a1025
Brent Christian (@bchristi-git - Reviewer)
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10715

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

Using diff file

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

bridgekeeper · 2022-10-14T20:39:15Z

👋 Welcome back justin-curtis-lu! 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 · 2022-10-14T20:45:37Z

@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.

test/jdk/java/util/Formatter/Basic.java

test/jdk/java/util/Formatter/BasicTestLauncher.java

mlbridge · 2022-10-17T22:09:41Z

Webrevs

LanceAndersen

Hi Justin,

A few comments on a quick pass through your changes

LanceAndersen · 2022-10-17T22:18:01Z

test/jdk/java/util/Formatter/Basic.java

@@ -101,6 +90,6 @@ public static void main(String[] args) {
            throw new RuntimeException((fail + pass) + " tests: "
                                       + fail + " failure(s), first", first);
        else
-            out.println("all " + (fail + pass) + " tests passed");
+            System.out.println("all " + (fail + pass) + " tests passed");


Perhaps use System.out.printf vs println and "fail" should not be needed as all tests passed

Thank you Lance, will take a look at this and the rest of the comments

Careful. I suspect that "fail" is the number of tests that are expected to fail.

Thanks Iris,
It got hidden in the code snippet, but line 89 is if (fail != 0),
so I do believe fail is redundant in 93.

LanceAndersen · 2022-10-17T22:20:11Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+
+
+    @Test
+    public void testUsPac() throws IOException{


You could use a DataProvider

I had the same thought

Thanks Lance and Brent, replaced it with a JUnit parametrized test (DataProvider equivalent)

LanceAndersen · 2022-10-17T22:22:30Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+                    .reportDiagnosticSummary();
+        }catch(RuntimeException err){
+            throw new RuntimeException(String.format("$$$ %s: Test(s) failed or TestJVM did not build correctly." +
+                    " Check stderr output from diagnostics summary above%n",  err.getMessage()));


Do you need to catch the RuntimeException and then throw a new RuntimeException? I think you might want to consider reworking this

Thank you Lance, I dropped the catch altogether as there is already enough info in the test log output

bchristi-git · 2022-10-18T17:26:52Z

test/jdk/java/util/Formatter/Basic.java

@@ -101,6 +90,6 @@ public static void main(String[] args) {
            throw new RuntimeException((fail + pass) + " tests: "
                                       + fail + " failure(s), first", first);
        else
-            out.println("all " + (fail + pass) + " tests passed");
+            System.out.printf("all " + pass + " tests passed");


This line is missing a newline; add a \n, or use println().

I would suggest rewriting as System.out.printf("all %s tests passed%n", pass);

You could make a similar change to line 90 using String.format as done in the line 98 which was deleted

I dropped the new line since when the output comes out of the sub process log

Without the newline it looks like
[ ... ]

vs

[...
]

bchristi-git · 2022-10-18T17:28:49Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+    private static final String TZ_UP = "US/Pacific";
+    // Asia/Novosibirsk time zone
+    private static final String TZ_AN = "Asia/Novosibirsk";


IMO it's not necessary to create constants if they'll only be used as a ValueSource

True, but really a personal choice as it makes the ValueSource less wordy ;-)

Also, when reading through a ValueSource / DataProvider, it's nice to see the actual values (vs variable/constant names) when practical/possible.

bchristi-git · 2022-10-18T17:31:45Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+    // Asia/Novosibirsk time zone
+    private static final String TZ_AN = "Asia/Novosibirsk";
+    // Locale flag for testJVM
+    private static final String LOCALE_PROV = "-Djava.locale.providers=CLDR";


A name like "JAVA_OPTS" would better express how this value is used.

bchristi-git · 2022-10-18T17:34:09Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+    // Locale flag for testJVM
+    private static final String LOCALE_PROV = "-Djava.locale.providers=CLDR";
+    // Test class
+    private static final String SOURCE_CLASS = "Basic";


This is the name of a compiled class to be run, not a source file, so perhaps, "TEST_CLASS"?

bchristi-git · 2022-10-18T17:36:24Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+/* @test
+ * @summary Unit tests for formatter
+ * @library /test/lib
+ * @compile Basic.java


For the record, I've not deduced how/why the rest of the .java files in the test get compiled to .class files...

I suspect because the other classes are in the test.src directory they are being compiled which can be verified by Justin looking at the jtr log for the test

LanceAndersen

Hi Justin,

We are getting close ;-)

Please see below

LanceAndersen · 2022-10-18T17:49:01Z

test/jdk/java/util/Formatter/Basic.java

@@ -101,6 +90,6 @@ public static void main(String[] args) {
            throw new RuntimeException((fail + pass) + " tests: "
                                       + fail + " failure(s), first", first);
        else
-            out.println("all " + (fail + pass) + " tests passed");
+            System.out.printf("all " + pass + " tests passed");


I would suggest rewriting as System.out.printf("all %s tests passed%n", pass);

You could make a similar change to line 90 using String.format as done in the line 98 which was deleted

LanceAndersen · 2022-10-18T17:49:16Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+    // Locale flag for testJVM
+    private static final String LOCALE_PROV = "-Djava.locale.providers=CLDR";
+    // Test class
+    private static final String SOURCE_CLASS = "Basic";


bchristi-git · 2022-10-18T18:46:16Z

test/jdk/java/util/Formatter/Basic.java

        else
-            out.println("all " + (fail + pass) + " tests passed");
+            System.out.printf("All %s tests passed", pass);


Right, will change that

And one on the previous line

bchristi-git · 2022-10-18T18:58:33Z

test/jdk/java/util/Formatter/Basic.java

        else
-            out.println("all " + (fail + pass) + " tests passed");
+            System.out.printf("All %s tests passed", pass);


And one on the previous line

bchristi-git · 2022-10-18T19:20:58Z

test/jdk/java/util/Formatter/Basic.java

-        else
-            out.println("all " + (fail + pass) + " tests passed");
+        if (fail != 0) {
+            throw new RuntimeException(String.format("%d tests: %d failure(s)" +


You might consider including ", first" with the rest of the message string, instead of concatenating it. That line might end up slightly long, but it may be worth it.

Also, use %s forfirst, as it's a Throwable ;)
(You could also perhaps change first -> first.toString() in the final argument to format, to clarify.)

Good point, will reassign it to one line.

"Also, use %s forfirst, as it's a Throwable",
I believe I am assigning %d to pass+fail and fail. I pass first as a Throwable to the runtime exception constructor.

Oh, right. Carry on.

Instead of using String.format() static method, using "...".formatted() is slightly shorter/concise.

Everything under test.src compiled

bchristi-git · 2022-10-19T22:20:56Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+            // Build and run Basic class with correct configuration
+            ProcessBuilder pb = ProcessTools.createTestJvm(JAVA_OPTS, TEST_CLASS);
+            pb.environment().put("TZ", timeZone);
+            Process process = pb.start();


Nit: indentation

Fixed, thanks!

bchristi-git · 2022-10-19T22:21:06Z

test/jdk/java/util/Formatter/BasicTestLauncher.java

+            output.shouldHaveExitValue(0)
+                    .reportDiagnosticSummary();


Nit: indentation

openjdk · 2022-10-20T19:37:27Z

@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:

8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher

Reviewed-by: lancea, bchristi, 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 141 new commits pushed to the master branch:

f5dabf9: 8295088: Update External Spec page to show tabs for hosts
2181042: 8295375: debug agent class tracking should not piggy back on the cbClassPrepare() callback
f41711e: 8295650: JFR: jfr scrub should warn if an event type doesn't exist
0c13d66: 8295530: Update Zlib Data Compression Library to Version 1.2.13
15bebf9: 8295666: Linux x86 build fails after 8292591
5064718: 8294460: CodeSection::alignment checks for CodeBuffer::SECT_STUBS incorrectly
8b010e0: 8030616: sun/management/jmxremote/bootstrap/RmiBootstrapTest fails intermittently with cannot find a free port
b35922b: 8295714: GHA ::set-output is deprecated and will be removed
dfd2d83: 8295657: SA: Allow larger object alignments
a345df2: 8280131: jcmd reports "Module jdk.jfr not found." when "jdk.management.jfr" is missing
... and 131 more: https://git.openjdk.org/jdk/compare/ac1941425bdb1a25aa3364eef9eb1092ee716761...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@LanceAndersen, @bchristi-git, @naotoj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

LanceAndersen · 2022-10-20T20:09:18Z

test/jdk/java/util/Formatter/Basic.java

+        if (fail != 0) {
+            var tests_message = "%d tests: %d failure(s)%n".formatted(fail + pass, fail);
+            var trace_message = "Traceback of the first error located";
+            String message = "%s %s".formatted(tests_message, trace_message);


could use "var" as the previous statements ;-)

justin-curtis-lu · 2022-10-21T21:12:43Z

/integrate

openjdk · 2022-10-21T21:15:03Z

@justin-curtis-lu
Your change (at version 781b352) is now ready to be sponsored by a Committer.

bchristi-git · 2022-10-21T22:58:08Z

/sponsor

openjdk · 2022-10-21T22:59:31Z

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

f5dabf9: 8295088: Update External Spec page to show tabs for hosts
2181042: 8295375: debug agent class tracking should not piggy back on the cbClassPrepare() callback
f41711e: 8295650: JFR: jfr scrub should warn if an event type doesn't exist
0c13d66: 8295530: Update Zlib Data Compression Library to Version 1.2.13
15bebf9: 8295666: Linux x86 build fails after 8292591
5064718: 8294460: CodeSection::alignment checks for CodeBuffer::SECT_STUBS incorrectly
8b010e0: 8030616: sun/management/jmxremote/bootstrap/RmiBootstrapTest fails intermittently with cannot find a free port
b35922b: 8295714: GHA ::set-output is deprecated and will be removed
dfd2d83: 8295657: SA: Allow larger object alignments
a345df2: 8280131: jcmd reports "Module jdk.jfr not found." when "jdk.management.jfr" is missing
... and 131 more: https://git.openjdk.org/jdk/compare/ac1941425bdb1a25aa3364eef9eb1092ee716761...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-21T22:59:46Z

@bchristi-git @justin-curtis-lu Pushed as commit 902162c.

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

Remove basic.sh, replace with java test launcher

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

a732f8c

openjdk bot added core-libs i18n labels Oct 14, 2022

naotoj reviewed Oct 14, 2022

View reviewed changes

Address PR comments (Junit, exceptions)

f750adb

justin-curtis-lu marked this pull request as ready for review October 17, 2022 22:04

openjdk bot added the rfr label Oct 17, 2022

LanceAndersen reviewed Oct 17, 2022

View reviewed changes

Use data provider, drop exception

1cbca6e

bchristi-git suggested changes Oct 18, 2022

View reviewed changes

LanceAndersen reviewed Oct 18, 2022

View reviewed changes

justin-curtis-lu added 3 commits October 18, 2022 11:01

Remove time zone constants (using ValueSource)

089c6ba

Adjust constant names

bb01f86

Adjust output formating in Basic

fe67e05

bchristi-git suggested changes Oct 18, 2022

View reviewed changes

justin-curtis-lu added 2 commits October 18, 2022 11:51

Add curly braces to Basic if else

5b9225c

Fix formatter in Basic

148b3d3

bchristi-git suggested changes Oct 18, 2022

View reviewed changes

Remove testing change

9483f16

bchristi-git suggested changes Oct 18, 2022

View reviewed changes

justin-curtis-lu added 3 commits October 18, 2022 13:22

Remove @compile

81fc6a3

Everything under test.src compiled

Add all BasicXXX files to @compile for clarity

00c32ee

Additional cleanup and string formatting

56eae09

bchristi-git reviewed Oct 19, 2022

View reviewed changes

Fix indentation

5a0ee74

bchristi-git approved these changes Oct 20, 2022

View reviewed changes

openjdk bot added the ready label Oct 20, 2022

Spacing conventions

676a102

LanceAndersen approved these changes Oct 20, 2022

View reviewed changes

Format with var

781b352

naotoj approved these changes Oct 20, 2022

View reviewed changes

bchristi-git approved these changes Oct 20, 2022

View reviewed changes

openjdk bot added the sponsor label Oct 21, 2022

openjdk bot added the integrated label Oct 21, 2022

openjdk bot closed this Oct 21, 2022

openjdk bot removed ready rfr sponsor labels Oct 21, 2022

8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher #10715

8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher #10715

Conversation

justin-curtis-lu commented Oct 14, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 14, 2022

openjdk bot commented Oct 14, 2022

mlbridge bot commented Oct 17, 2022 • edited Loading

Webrevs

LanceAndersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

justin-curtis-lu commented Oct 21, 2022

openjdk bot commented Oct 21, 2022

bchristi-git commented Oct 21, 2022

openjdk bot commented Oct 21, 2022

openjdk bot commented Oct 21, 2022

justin-curtis-lu commented Oct 14, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 17, 2022 •

edited

Loading

openjdk bot commented Oct 20, 2022 •

edited

Loading