Hi Christian,
Yes, my intention was to check the equality of the returned data.
I've changed line 68 to:
Asserts.assertEquals(Layer.boot().modules(), getModulesJVMTI());
and removed line 90 since it's not needed.
As to the line 76, that is how Netbeans has formatted the code. I've changed it
to have {} on the same line now.
Please, find the updated review at:
http://cr.openjdk.java.net/~akulyakh/8153978_7/test/serviceability/jvmti/GetModulesInfo/JvmtiGetAllModulesTest.java.html
Best regards,
Alexander
----- Original Message -----
From: [email protected]
To: [email protected], [email protected]
Cc: [email protected]
Sent: Friday, July 22, 2016 3:50:09 PM GMT +03:00 Iraq
Subject: RE: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
Hi Alexander,
As Serguei said, the lines 68 and 90 doesn’t check the results so they should
either do that or be removed. If you remove those lines, you can remove the
filtering out of unnamed modules in getModulesJVMTI as well since that will no
longer be necessary.
Minor style thing, move the } on 76 to be on the same line as the opening {.
Thanks,
Christian
From: Alexander Kulyakhtin [mailto:[email protected]]
Sent: Friday, July 22, 2016 7:40 AM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: Re: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
Hi Sergey,
Thank you very much for the review. I'm going to wait for any other findings
today and, if everything is fine, will push the fix then.
Best regards,
Alexander
----- Original Message -----
From: [email protected]
To: [email protected] , [email protected]
Cc: [email protected]
Sent: Friday, July 22, 2016 11:31:13 AM GMT +03:00 Iraq
Subject: Re: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
Alexander,
A thumbs up on the push.
I leave it up to you and Christian to tweak and polish the test if you think it
is necessary.
Thank you a lot for working on it!
Thanks,
Serguei
On 7/21/16 14:05, [email protected] wrote:
On 7/21/16 11:35, [email protected] wrote:
Hi Alexander,
JvmtiGetAllModulesTest.java
It looks pretty good but it would be nice if there is any chance to simplify
even more.
However, I can't suggest anything at the moment.
67 // Verify that JVMTI reports exactly the same info as Java regarding the
named modules
68 Layer.boot().equals(getModulesJVMTI()); 69
. . .
89 // Verify the consistency of the whole JVMTI report again
90 Layer.boot().equals(getModulesJVMTI()); 91
The above lines can be removed.
They even do not check the result of comparison.
Thanks,
Serguei
libJvmtiGetAllModulesTest.c
Unneeded indent for all lines.
Otherwise, it is good.
Thanks,
Serguei
On 7/21/16 10:14, Alexander Kulyakhtin wrote:
Christian, Sergey,
I've modified the test per your findings. Now it is one java file and one C
file only.
Please, find the updated review at:
Webrev: http://cr.openjdk.java.net/~akulyakh/8153978_6/
Thank you very much for your help.
Best regards,
Alexander
----- Original Message -----
From: [email protected]
To: [email protected] , [email protected]
Cc: [email protected]
Sent: Thursday, July 21, 2016 6:39:21 PM GMT +03:00 Iraq
Subject: Re: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
On 7/21/16 08:29, Christian Tornqvist wrote:
Hi Alexander,
>The JVMTI always reports 3 unnamed modules: the boot module, the system module
>and the application module.
>The Java API does not report any unnamed modules.
I’ll leave this up to you if this is something that we need to verify or not,
the code for doing this is also overcomplicated and can be reduced to a simple
assertGTE.
The rule is that there is one unnamed module per a class loader.
The options are: to test this rule or remove the check.
For simplicity is better to remove this check as unclear.
Thanks,
Serguei
>This should be doable without using JAR's and custom loaders by using
>Layer.defineModules(), see the examples in
>jdk/test/java/lang/reflect/Layer/BasicLayerTest.java
>The test has been written from the user perspective. The user loads a new
>module in the form of jar using the ModuleLoader.loadModule() API. Then the
>test checks that JVMTI does return the info about that loaded module.
>Probably, defining the module using Layer.defineModules would not be the same
>as loading the module using ModuleLoader.loadModule(), since the JVMTI
>GetAllModules() returns the info about all the currently loaded modules.
>As the JVMTI spec says: "GetAllModules: Return an array of all modules loaded
>in the virtual machine.", it does not mention defining modules.
There are several ways to get modules loaded/defined, the Layer.defineModules
is part of the official Java API and is one of them. It doesn’t matter to JVMTI
if they come from JAR files on disk or if they’re defined using a Java API, so
I suggest you go with Layer.defineModules.
Thanks,
Christian
From: Alexander Kulyakhtin [ mailto:[email protected] ]
Sent: Thursday, July 21, 2016 10:04 AM
To: Serguei Vladimirovich Spitsyn <[email protected]> ;
[email protected]
Cc: [email protected]
Subject: Re: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
Christian,
Thank you very much for your comments. I have some concerns about the proposed
changes:
@45 & @67
Why is this check needed? Why are there least 3 unnamed modules?
The JVMTI always reports 3 unnamed modules: the boot module, the system module
and the application module.
The Java API does not report any unnamed modules.
@54
This should be doable without using JAR's and custom loaders by using
Layer.defineModules(), see the examples in
jdk/test/java/lang/reflect/Layer/BasicLayerTest.java
The test has been written from the user perspective. The user loads a new
module in the form of jar using the ModuleLoader.loadModule() API. Then the
test checks that JVMTI does return the info about that loaded module.
Probably, defining the module using Layer.defineModules would not be the same
as loading the module using ModuleLoader.loadModule(), since the JVMTI
GetAllModules() returns the info about all the currently loaded modules.
As the JVMTI spec says: "GetAllModules: Return an array of all modules loaded
in the virtual machine.", it does not mention defining modules.
Could you, please, clarify these points for me so I fix the test appropriately?
Best regards,
Alexander
----- Original Message -----
From: [email protected]
To: [email protected] , [email protected]
Sent: Wednesday, July 20, 2016 8:11:14 PM GMT +03:00 Iraq
Subject: RE: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
Hi Alexander,
This test is unnecessarily complicated, it could be simplified a lot.
JvmtiGetAllModulesTest.java
Move getModulesNative() into JvmtiGetAllModulesTest.java and have it return a
Set<Module> to be able to use equals later
@27 * @compile JvmtiGetAllModulesTest.java
No need for this, jtreg will compile it for you
@45 & @67
Why is this check needed? Why are there least 3 unnamed modules?
@50
Change this to: assertTrue(Layer.boot().equals(getModulesNative()));
@54
This should be doable without using JAR's and custom loaders by using
Layer.defineModules(), see the examples in
jdk/test/java/lang/reflect/Layer/BasicLayerTest.java
@65
Change this to an assertTrue using the layer containing the new module, similar
to the change @50
@73
No need for this method
@81
Change this method to use the Layer.defineModules() method to define a module
instead, this eliminates the need for external JAR's
@98
No need for this method
If you use Layer.defineModules(), the following files can be removed:
JarBuilder.java
JavaModulesInfo.java
JvmtiModulesInfo.java
ModuleLoader.java
ModulesInfo.java
module-info.java
Thanks,
Christian
From: serviceability-dev [ mailto:[email protected] ]
On Behalf Of [email protected]
Sent: Monday, May 2, 2016 6:06 PM
To: Alexander Kulyakhtin < [email protected] >;
Serviceability-Dev < [email protected] >
Subject: Re: RFR:8153978:New test to verify the modules info as returned by the
JVMTI
Hi Alexander,
Could you, fix a couple of minor issues?
test/serviceability/jvmti/GetModulesInfo/JvmtiGetAllModulesTest.java 58
for(Module mod : my.modules()) { 59 if(!jvmtiModules.contains(mod))
{ A space is missed after the 'for' and 'if' keywords.
test/serviceability/jvmti/GetModulesInfo/ModulesInfo.java. 31 boolean
compareExcludingUnnamed(ModulesInfo other) { I'd suggest to call it
compareNamed.
Otherwise, the new test looks great.
Thanks a lot for taking care about it!
Thanks,
Serguei
On 4/29/16 06:12, Alexander Kulyakhtin wrote:
Hi, Could you, please, review these test-only changes (adding a new test). CR:
https://bugs.openjdk.java.net/browse/JDK-8153978 "New test to verify the
modules info as returned by the JVMTI" Webrev:
http://cr.openjdk.java.net/~akulyakh/8153978_01/ The new test verifies that
JVMTI returns the correct info about the modules loaded at the application
startup. It also verifies that the returned info is consistent with the same
info returned by the Java API. It then loads a new named module and checks the
correctness of the JVMTI info again. Due to a tools issue
https://bugs.openjdk.java.net/browse/CODETOOLS-7901662 the test can only be
pushed in when the updated jtreg is released. The test passes fine with the
nightly jtreg build, containing the CODETOOLS-7901662 fix. Best regards,
Alexander
