Re: RFR: 8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher [v2]

[Brent Christian]

On Tue, 18 Oct 2022 17:05:08 GMT, Justin Lu <d...@openjdk.org> wrote:

>> 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
>> <img 
>> src="https://user-images.githubusercontent.com/67398801/195936541-bc90db50-8d03-47be-9c4f-95176b19a6a7.png";
>>  width="350" height="350">
>> 
>> 
>> New output on success
>> <img 
>> src="https://user-images.githubusercontent.com/67398801/195936558-f85f4d48-dae2-4c38-aa50-46ef47db3d89.png";
>>  width="350" height="450">
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use data provider, drop exception

Changes requested by bchristi (Reviewer).

test/jdk/java/util/Formatter/Basic.java line 93:

> 91:                                        + fail + " failure(s), first", 
> first);
> 92:         else
> 93:             System.out.printf("all " + pass + " tests passed");

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

test/jdk/java/util/Formatter/BasicTestLauncher.java line 35:

> 33:  * @summary Unit tests for formatter
> 34:  * @library /test/lib
> 35:  * @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...

test/jdk/java/util/Formatter/BasicTestLauncher.java line 47:

> 45:     private static final String TZ_UP = "US/Pacific";
> 46:     // Asia/Novosibirsk time zone
> 47:     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

test/jdk/java/util/Formatter/BasicTestLauncher.java line 49:

> 47:     private static final String TZ_AN = "Asia/Novosibirsk";
> 48:     // Locale flag for testJVM
> 49:     private static final String LOCALE_PROV = 
> "-Djava.locale.providers=CLDR";

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

test/jdk/java/util/Formatter/BasicTestLauncher.java line 51:

> 49:     private static final String LOCALE_PROV = 
> "-Djava.locale.providers=CLDR";
> 50:     // Test class
> 51:     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"?

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

PR: https://git.openjdk.org/jdk/pull/10715