116 if (result.isOK()) {
117 throw new Exception("Test Passed Unexpectedly!");
118 } else if (result.contains("JNI error")) {
119 result.testOutput.forEach(System.err::println);
120 throw new Exception("Test Failed with JNI error!");
121 }
+ 122 System.out.println("Test passes, failed with expected error message");
As a sanity I would add the above. This will make it clear to someone
trying to triage an error.
Kumar
On 1/25/2017 2:10 AM, Ramanand Patil wrote:
Hi Kumar,
Thank you for the review and suggestions for the test case.
Here is the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8167063/webrev.02/
Regards,
Ramanand.
-----Original Message-----
From: Kumar Srinivasan
Sent: Tuesday, January 24, 2017 2:50 AM
To: Ramanand Patil <ramanand.pa...@oracle.com>
Cc: Alan Bateman <alan.bate...@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if
start-class cannot be initialized
Hi Ramanand,
test/tools/launcher/LauncherMessageTest.java
1)
116 String[] commands = {"java", "--module-path", modules.getPath(),
117 "-m", "mod.b/pkgB.ClassB"};
The execution PATH may or may not contain the JAVA_HOME_UNDER_TEST/bin, so the right
"java" may not be picked up, suggest using the TestHelper's javaCmd field that
will be set correctly.
Used TestHelper.javaCmd for instead of "java"
2)
122 if (!result.isOK() && result.contains("JNI error")) {
123 result.testOutput.forEach(System.err::println);
124 throw new RuntimeException("Test Failed with JNI error!");
125 } else {
126 System.out.println("Test Passed...");
127 }
The problem with 122 if it the test returns back with non-zero code, it will still fail
with "Test Failed with JNI error", I prefer it to be broken up and return back
the right message ie. if the test returns with non-zero code say that.
Changed the way test results were checked to include the check for unexpected
failure when result is not non-zero.(i.e. when isOK() returns true).
3)
Also usage of RuntimeException is over the top, you can simply throw an
Exception and have the main throws Exception, jtreg will do the needful.
Changed. Used an Exception instead of RuntimeException
4)
102 FileUtils.deleteFileWithRetry(Paths.get(modules.getPath() + File.separator
+ "mod.a.jar"));
there are redundant use of File.separator, Paths.get should create the FS
correctly on the target platform
ex: Paths.get(modules.getPath(), "mod.a.jar");
Personally I stay away from using raw File.separator, instead have the APIs do
the work for me.
-Removed all instances of File.separator
Kumar
Hi Alan,
Thank you for the review.
My comments are inline and Webrev is updated here:
http://cr.openjdk.java.net/~rpatil/8167063/webrev.01/
Change Summary:
- Removed SecurityException handling
- Updated the error message in launcher.properties
- Removed loadModuleMainClass0 method and moved the code back into
original loadModuleMainClass
- NoClassDefFoundError is replaced by its parent class LinkageError in
method loadMainClass
Regards,
Ramanand.
-----Original Message-----
From: Alan Bateman
Sent: Friday, January 20, 2017 8:03 PM
To: Ramanand Patil <ramanand.pa...@oracle.com>;
core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred"
if start-class cannot be initialized
On 20/01/2017 13:21, Ramanand Patil wrote:
Hi all,
Please review the following bug fix:
Bug: https://bugs.openjdk.java.net/browse/JDK-8167063
Webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.00/
Handled the SecurityException and LinkageError which can be thrown from
Class.forName(...) method used in LauncherHelper.java and added corresponding
error messages to launcher.properties.
Though the reported bug is because of the LinkageError, Security exception is
also handled to be safe from calling Class.forName method.
I see this changes loadMainClass to abort with resources that are keyed on
java.launcher.cls.error6 and java.launcher.cls.error7 but they don't appear in
the launcher.properties file. Does this work? I would assume
MissingResourceException is thrown.
Thanks for pointing this. I missed to add it to launcher.properties. But now I
have changed error6 to point to java.launcher.cls.error1 and removed security
exception handling.
For java.launcher.module.error3 and java.launcher.module.error4 then "link" is likely to
confuse people. Sure, there may be linkage errors but there are many linkage errors and I think
would be a lot clearer if the message was "Unable to load main class {0} from module
{1}\n\{2}".
Updated the message as suggested.
It's not clear to me that having a different message for security exceptions
make sense, esp. when the exception is printed. So I think I would drop that.
Yes, I agree to drop out the Security exceptions handling.
Also loadModuleMainClass0 is unusual method name, we've mostly (not
always) used the 0 suffix on native methods. In any case, it doesn't look like
it is needed, the code was okay in loadModuleMainClass as it was.
The method was added just to look the exception handling easier. I have removed
the extra method and placed the code back into loadModuleMainClass.
One final point is that is the nesting catching of LinkageError and
SecurityException in loadMainClass, I assume you don't need the inner catch.
I think both the catch blocks were needed, because the first(inner) catch was
already inside catch.(This was same as NoClassDefFoundError catching twice in
the same method). Now since NoClassDefFoundError is subclass of LinkageError, I
have replaced it with LinkageError. And I think still the same abort message
holds good. Please let me know if this is ok.
-Alan
