RFR:8153978:New test to verify the modules info as returned by the JVMTI

Thu Jul 21 15:29:49 UTC 2016

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.

>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:alexander.kulyakhtin at oracle.com] 
Sent: Thursday, July 21, 2016 10:04 AM
To: Serguei Vladimirovich Spitsyn <serguei.spitsyn at oracle.com>; christian.tornqvist at oracle.com
Cc: serviceability-dev at openjdk.java.net
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: christian.tornqvist at oracle.com <mailto:christian.tornqvist at oracle.com> 
To: alexander.kulyakhtin at oracle.com <mailto:alexander.kulyakhtin at oracle.com> , serviceability-dev at openjdk.java.net <mailto:serviceability-dev at openjdk.java.net> 
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:serviceability-dev-bounces at openjdk.java.net] On Behalf Of serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> 
Sent: Monday, May 2, 2016 6:06 PM
To: Alexander Kulyakhtin <alexander.kulyakhtin at oracle.com <mailto:alexander.kulyakhtin at oracle.com> >; Serviceability-Dev <serviceability-dev at openjdk.java.net <mailto:serviceability-dev at openjdk.java.net> >
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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160721/8501cfad/attachment.html>