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

serguei.spit...@oracle.com Wed, 20 Jul 2016 14:58:17 -0700

Christian,

Thank you for the comments.
I agree, this test is over complicated.
Your suggestions for simplifications are good.


Alexander,

Could you, please, update the webrev according to the Christian's comments?
I will re-review it then.
Can you do it tomorrow?
Otherwise, I'm going to vacation starting from the next week.

Thanks,
Serguei


On 7/20/16 10:10, Christian Tornqvist wrote:


Hi Alexander,

This test is unnecessarily complicated, it could be simplified a lot.

JvmtiGetAllModulesTest.java

Move getModulesNative() into JvmtiGetAllModulesTest.java and have itreturn 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 usingLayer.defineModules(), see the examples injdk/test/java/lang/reflect/Layer/BasicLayerTest.java

@65

Change this to an assertTrue using the layer containing the newmodule, similar to the change @50


@73

No need for this method

@81

Change this method to use the Layer.defineModules() method to define amodule 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-boun...@openjdk.java.net] *On Behalf Of*serguei.spit...@oracle.com

*Sent:* Monday, May 2, 2016 6:06 PM

*To:* Alexander Kulyakhtin <alexander.kulyakh...@oracle.com>;Serviceability-Dev <serviceability-dev@openjdk.java.net>*Subject:* Re: RFR:8153978:New test to verify the modules info asreturned 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

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

Reply via email to