RFR:8153978:New test to verify the modules info as returned by the JVMTI
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jul 21 21:05:25 UTC 2016
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 <mailto: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/20160721/6b9f0088/attachment-0001.html>
More information about the serviceability-dev
mailing list