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

[serguei.spitsyn at oracle.com]

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 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/20160720/de08a0e2/attachment.html>