[JDK 11] RFR 8201528: Add new test to check for package versioning information in OpenJDK
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 <https://bugs.openjdk.java.net/browse/JDK-8201528> webrev: http://cr.openjdk.java.net/~xyin/8201528/webrev.00/ <http://cr.openjdk.java.net/~xyin/8201528/webrev.00/> Regards, Chris
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. 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. It'd be good to refactor the main method into separate cases and a switch/if statement on different runtType will help the readability. 123 runTest(cl.loadClass( 124 PACKAGE_NAME + "." + TEST_CLASS_NAME1) 125 .getPackage(), option); Can you use Class::forName here? 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. 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). Mandy
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@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
On 6/8/18 12:07 AM, Chris Yin wrote:
Hi, Mandy
Many thanks for your detailed review and comments, updates new webrev as below, and comment inline, thanks
Thanks for adding the test description, that is very helpful. This is looking pretty good. I have some suggestion on the test arguments that one can translate to the command-line easily. 69 * PackageFromManifest runJar single 73 * PackageFromManifest runUrlLoader single single means running with "testpack1.jar". What about putting the JAR file name as the option which makes it very clear what is being tested. Nit: "testpack1" could be confused with pack200 and maybe simply test1.jar. How about @run main PackageFromManifest runJar test1.jar 77 * PackageFromManifest runJar 1 81 * PackageFromManifest runJar 2 These tests load two JAR files and load foo.Foo<n>. What do you think to express: PackageFromManifest runJar test1.jar test2.jar foo.Foo1 PackageFromManifest runJar test1.jar test2.jar foo.Foo2 85 * PackageFromManifest runUrlLoader 1 89 * PackageFromManifest runUrlLoader 2 PackageFromManifest runUrlLoader test1.jar test2.jar foo.Foo1 PackageFromManifest runUrlLoader test1.jar test2.jar foo.Foo2 Nit: I suggest to group the runType together. i.e. move line 34 to above line 37. "<p>" not strictly needed as we won't generate the javadoc. You can consider leaving it a blank line. Mandy
Hi, Mandy Thanks lot for your suggestions, update webrev as below and comment inline, thanks http://cr.openjdk.java.net/~xyin/8201528/webrev.02/ <http://cr.openjdk.java.net/~xyin/8201528/webrev.02/>
On 9 Jun 2018, at 4:22 AM, mandy chung <mandy.chung@oracle.com> wrote:
On 6/8/18 12:07 AM, Chris Yin wrote:
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/
Thanks for adding the test description, that is very helpful. This is looking pretty good. I have some suggestion on the test arguments that one can translate to the command-line easily.
69 * PackageFromManifest runJar single 73 * PackageFromManifest runUrlLoader single
single means running with "testpack1.jar". What about putting the JAR file name as the option which makes it very clear what is being tested. Nit: "testpack1" could be confused with pack200 and maybe simply test1.jar. How about
Fixed, thanks
@run main PackageFromManifest runJar test1.jar
77 * PackageFromManifest runJar 1 81 * PackageFromManifest runJar 2
These tests load two JAR files and load foo.Foo<n>. What do you think to express:
PackageFromManifest runJar test1.jar test2.jar foo.Foo1 PackageFromManifest runJar test1.jar test2.jar foo.Foo2
85 * PackageFromManifest runUrlLoader 1 89 * PackageFromManifest runUrlLoader 2
PackageFromManifest runUrlLoader test1.jar test2.jar foo.Foo1 PackageFromManifest runUrlLoader test1.jar test2.jar foo.Foo2
Used new style as you suggested, that looks exactly match with the description :)
Nit: I suggest to group the runType together. i.e. move line 34 to above line 37. "<p>" not strictly needed as we won't generate the javadoc. You can consider leaving it a blank line.
Sure, grouped runType and removed <p> (oh, it’s added by IDE during code reformat) Thanks & Regards, Chris
Mandy
On 6/10/18 10:12 PM, Chris Yin wrote:
Hi, Mandy
Thanks lot for your suggestions, update webrev as below and comment inline, thanks
Looks good. Thanks for the update and this is much easier to understand the test cases. Mandy
Thank you, Mandy Regards, Chris
On 12 Jun 2018, at 1:48 AM, mandy chung <mandy.chung@oracle.com> wrote:
On 6/10/18 10:12 PM, Chris Yin wrote:
Hi, Mandy Thanks lot for your suggestions, update webrev as below and comment inline, thanks http://cr.openjdk.java.net/~xyin/8201528/webrev.02/
Looks good. Thanks for the update and this is much easier to understand the test cases.
Mandy
participants (2)
-
Chris Yin
-
mandy chung