RFR:8153978:New test to verify the modules info as returned by the JVMTI
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jul 22 08:31:12 UTC 2016
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, serguei.spitsyn at oracle.com wrote:
> On 7/21/16 11:35, serguei.spitsyn at oracle.com 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: serguei.spitsyn at oracle.com
>>> To: christian.tornqvist at oracle.com, alexander.kulyakhtin at oracle.com
>>> Cc: serviceability-dev at openjdk.java.net
>>> 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: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
>>> *Sent:* Monday, May 2, 2016 6:06 PM
>>> *To:* Alexander Kulyakhtin <alexander.kulyakhtin at oracle.com>;
>>> Serviceability-Dev <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/
>>> <http://cr.openjdk.java.net/%7Eakulyakh/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 issuehttps://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/20160722/852bc9f7/attachment-0001.html>
More information about the serviceability-dev
mailing list