[JDK 11] RFR 8201528: Add new test to check for package versioning information in OpenJDK
Chris Yin
xu.y.yin at oracle.com
Fri Jun 8 07:07:29 UTC 2018
Hi, Mandy
Many thanks for your detailed review and comments, updates new webrev as below, and comment inline, thanks
webrev: http://cr.openjdk.java.net/~xyin/8201528/webrev.01/ <http://cr.openjdk.java.net/~xyin/8201528/webrev.01/>
> On 8 Jun 2018, at 6:38 AM, mandy chung <mandy.chung at oracle.com> wrote:
>
> Hi Chris,
>
> On 6/7/18 1:32 AM, Chris Yin wrote:
>> Please review below new added test to check for package versioning
>> information which customized in jar(s) manifest, many thanks
>> bug: https://bugs.openjdk.java.net/browse/JDK-8201528 webrev:
>> http://cr.openjdk.java.net/~xyin/8201528/webrev.00/
>
> Thanks for adding the new test cases that we are missing.
>
> Can you describe all these test cases? In the class javadoc is fine.
Sure, just added class javadoc to explain input arguments and each @run usage
>
> 32 * @run main PackageFromManifest runJar single
> 33 * @run main/othervm PackageFromManifest runUrlLoader single
> 34 * @run main PackageFromManifest runJar 1
> 35 * @run main PackageFromManifest runJar 2
> 36 * @run main/othervm PackageFromManifest runUrlLoader 1
> 37 * @run main/othervm PackageFromManifest runUrlLoader 2
>
> 74 if (runType.equalsIgnoreCase("runTest")) {
>
> The test accepts "runTest" but such test case is not listed above.
Thanks for checking this, “runTest” run type will be used in test logic runJar only (for example, called from runJar method, "java -cp testpack1.jar PackageFromManifest runTest single")
>
> It'd be good to refactor the main method into separate cases and
> a switch/if statement on different runtType will help the readability.
Thanks, refactored in new webrev as you suggested, it looks more clear now
>
> 123 runTest(cl.loadClass(
> 124 PACKAGE_NAME + "." + TEST_CLASS_NAME1)
> 125 .getPackage(), option);
>
> Can you use Class::forName here?
Sure, fixed in new webrev
>
> 200 cmds = new String[] { "-cp",
> 201 TEST_JAR_FILE1 + File.pathSeparator + TEST_JAR_FILE2,
> 202 "PackageFromManifest", "runTest", option };
>
> Thanks for adding the split package test. It'd be good to test
> 1) loading TEST_CLASS_NAME1 first and TEST_CLASS_NAME2 second
> 2) loading TEST_CLASS_NAME2 first and TEST_CLASS_NAME1 second
>
> Package should use the manifest attribute from the JAR file from
> which the first class of package foo is loaded.
Yep, to make it clear, in new webrev, the first loaded class name will be printed (combine load and print) before runTest, such as
System.out.println("Load " + Class.forName(PACKAGE_NAME + “.” + TEST_CLASS_PREFIX + option) + " first");
>
> Looks like it creates the JAR files each time the non-runTest case
> is run. You could do the setup as the first @run and subsequent
> @run are executing individual test cases (or make it testng test
> if you prefer).
Used setup at the first @run in new webrev as you suggested, thanks
Regards,
Chris
>
> Mandy
More information about the core-libs-dev
mailing list