<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: Times New Roman; font-size: 12pt; color: #000000'>Hi Christian,<br><br>Thank you very much for the review. <br><br>Best regards,<br>Alexander<br><br>----- Original Message -----<br>From: christian.tornqvist@oracle.com<br>To: alexander.kulyakhtin@oracle.com<br>Cc: serguei.spitsyn@oracle.com, serviceability-dev@openjdk.java.net<br>Sent: Friday, July 22, 2016 4:02:11 PM GMT +03:00 Iraq<br>Subject: RE: RFR:8153978:New test to verify the modules info as returned by the JVMTI<br><br><style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p
{mso-style-priority:99;
margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle23
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle24
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle25
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle26
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle27
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle28
{mso-style-type:personal-compose;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--><div lang="EN-US"><div class="WordSection1"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Hi Alexander,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">This looks good, thanks for adding this </span><span style="font-size:11.0pt;font-family:Wingdings;color:windowtext">J</span><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Thanks,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Christian</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><div><div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> Alexander Kulyakhtin [mailto:alexander.kulyakhtin@oracle.com] <br><b>Sent:</b> Friday, July 22, 2016 8:57 AM<br><b>To:</b> christian.tornqvist@oracle.com<br><b>Cc:</b> serguei.spitsyn@oracle.com; serviceability-dev@openjdk.java.net<br><b>Subject:</b> Re: RFR:8153978:New test to verify the modules info as returned by the JVMTI</span></p></div></div><p class="MsoNormal"> </p><div><p class="MsoNormal">Hi Christian,<br><br>Yes, my intention was to check the equality of the returned data.<br><br>I've changed line 68 to:<br><br>Asserts.assertEquals(Layer.boot().modules(), getModulesJVMTI());<br><br>and removed line 90 since it's not needed.<br><br>As to the line 76, that is how Netbeans has formatted the code. I've changed it to have {} on the same line now.<br><br>Please, find the updated review at: <a href="http://cr.openjdk.java.net/~akulyakh/8153978_7/test/serviceability/jvmti/GetModulesInfo/JvmtiGetAllModulesTest.java.html" target="_blank">http://cr.openjdk.java.net/~akulyakh/8153978_7/test/serviceability/jvmti/GetModulesInfo/JvmtiGetAllModulesTest.java.html</a><br><br>Best regards,<br>Alexander<br><br><br><br>----- Original Message -----<br>From: <a href="mailto:christian.tornqvist@oracle.com" target="_blank">christian.tornqvist@oracle.com</a><br>To: <a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">alexander.kulyakhtin@oracle.com</a>, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>Cc: <a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br>Sent: Friday, July 22, 2016 3:50:09 PM GMT +03:00 Iraq<br>Subject: RE: RFR:8153978:New test to verify the modules info as returned by the JVMTI<br><br><br></p><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Hi Alexander,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">As Serguei said, the lines 68 and 90 doesn’t check the results so they should either do that or be removed. If you remove those lines, you can remove the filtering out of unnamed modules in getModulesJVMTI as well since that will no longer be necessary.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Minor style thing, move the } on 76 to be on the same line as the opening {. </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Thanks,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Christian</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><div><div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> Alexander Kulyakhtin [<a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">mailto:alexander.kulyakhtin@oracle.com</a>] <br><b>Sent:</b> Friday, July 22, 2016 7:40 AM<br><b>To:</b> <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br><b>Cc:</b> <a href="mailto:christian.tornqvist@oracle.com" target="_blank">christian.tornqvist@oracle.com</a>; <a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br><b>Subject:</b> Re: RFR:8153978:New test to verify the modules info as returned by the JVMTI</span></p></div></div><p class="MsoNormal"> </p><div><p class="MsoNormal" style="margin-bottom:12.0pt">Hi Sergey,<br><br>Thank you very much for the review. I'm going to wait for any other findings today and, if everything is fine, will push the fix then.<br><br>Best regards,<br>Alexander<br><br>----- Original Message -----<br>From: <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>To: <a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">alexander.kulyakhtin@oracle.com</a>, <a href="mailto:christian.tornqvist@oracle.com" target="_blank">christian.tornqvist@oracle.com</a><br>Cc: <a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br>Sent: Friday, July 22, 2016 11:31:13 AM GMT +03:00 Iraq<br>Subject: Re: RFR:8153978:New test to verify the modules info as returned by the JVMTI</p><div><div><p class="MsoNormal">Alexander,<br><br>A thumbs up on the push.<br>I leave it up to you and Christian to tweak and polish the test if you think it is necessary.<br><br>Thank you a lot for working on it!<br><br>Thanks,<br>Serguei<br><br><br>On 7/21/16 14:05, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a> wrote:</p></div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><p class="MsoNormal">On 7/21/16 11:35, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a> wrote:</p></div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><p class="MsoNormal">Hi Alexander,<br><br>JvmtiGetAllModulesTest.java<br><br> It looks pretty good but it would be nice if there is any chance to simplify even more.<br> However, I can't suggest anything at the moment.</p></div></blockquote><p class="MsoNormal" style="margin-bottom:12.0pt"><br> 67 // Verify that JVMTI reports exactly the same info as Java regarding the named modules<br> 68 Layer.boot().equals(getModulesJVMTI()); 69 <br> . . .<br> 89 // Verify the consistency of the whole JVMTI report again<br> 90 Layer.boot().equals(getModulesJVMTI()); 91 <br><br> The above lines can be removed.<br> They even do not check the result of comparison.<br><br>Thanks,<br>Serguei<br><br><br></p><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><pre><span style="font-size:12.0pt"> </span></pre><p class="MsoNormal"><br>libJvmtiGetAllModulesTest.c<br><br> Unneeded indent for all lines.<br> Otherwise, it is good.<br><br>Thanks,<br>Serguei<br><br><br><br>On 7/21/16 10:14, Alexander Kulyakhtin wrote:</p></div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><p class="MsoNormal" style="margin-bottom:12.0pt">Christian, Sergey,<br><br>I've modified the test per your findings. Now it is one java file and one C file only.<br><br>Please, find the updated review at:<br><br>Webrev: <a href="http://cr.openjdk.java.net/%7Eakulyakh/8153978_6/" target="_blank">http://cr.openjdk.java.net/~akulyakh/8153978_6/</a><br><br>Thank you very much for your help.<br><br>Best regards,<br>Alexander<br><br><br>----- Original Message -----<br>From: <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>To: <a href="mailto:christian.tornqvist@oracle.com" target="_blank">christian.tornqvist@oracle.com</a>, <a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">alexander.kulyakhtin@oracle.com</a><br>Cc: <a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br>Sent: Thursday, July 21, 2016 6:39:21 PM GMT +03:00 Iraq<br>Subject: Re: RFR:8153978:New test to verify the modules info as returned by the JVMTI</p><div><div><p class="MsoNormal">On 7/21/16 08:29, Christian Tornqvist wrote:</p></div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Hi Alexander,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">>The JVMTI always reports 3 unnamed modules: the boot module, the system module and the application module. </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">>The Java API does not report any unnamed modules.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">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.</span></p></blockquote><p class="MsoNormal" style="margin-bottom:12.0pt"><br>The rule is that there is one unnamed module per a class loader.<br>The options are: to test this rule or remove the check.<br>For simplicity is better to remove this check as unclear.<br><br>Thanks,<br>Serguei<br><br></p><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">>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</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">>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.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">>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.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">>As the JVMTI spec says: "GetAllModules: Return an array of all modules loaded in the virtual machine.", it does not mention defining modules.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">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.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Thanks,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Christian</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><div><div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> Alexander Kulyakhtin [<a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">mailto:alexander.kulyakhtin@oracle.com</a>] <br><b>Sent:</b> Thursday, July 21, 2016 10:04 AM<br><b>To:</b> Serguei Vladimirovich Spitsyn <a href="mailto:serguei.spitsyn@oracle.com" target="_blank"><serguei.spitsyn@oracle.com></a>; <a href="mailto:christian.tornqvist@oracle.com" target="_blank">christian.tornqvist@oracle.com</a><br><b>Cc:</b> <a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br><b>Subject:</b> Re: RFR:8153978:New test to verify the modules info as returned by the JVMTI</span></p></div></div><p class="MsoNormal"> </p><div><p class="MsoNormal" style="margin-bottom:12.0pt">Christian,<br><br>Thank you very much for your comments. I have some concerns about the proposed changes:</p><p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">@45 & @67 </span></p><p class="MsoNormal" style="mso-margin-top-alt:auto;margin-bottom:12.0pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Why is this check needed? Why are there least 3 unnamed modules?<br>The JVMTI always reports 3 unnamed modules: the boot module, the system module and the application module. <br>The Java API does not report any unnamed modules.</span></p><p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">@54</span></p><p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">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<br>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.<br>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.<br>As the JVMTI spec says: "GetAllModules: Return an array of all modules loaded in the virtual machine.", it does not mention defining modules.<br><br>Could you, please, clarify these points for me so I fix the test appropriately?<br><br>Best regards,<br>Alexander</span></p><p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> </p><p class="MsoNormal" style="margin-bottom:12.0pt"><br><br><br><br>----- Original Message -----<br>From: <a href="mailto:christian.tornqvist@oracle.com" target="_blank">christian.tornqvist@oracle.com</a><br>To: <a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">alexander.kulyakhtin@oracle.com</a>, <a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br>Sent: Wednesday, July 20, 2016 8:11:14 PM GMT +03:00 Iraq<br>Subject: RE: RFR:8153978:New test to verify the modules info as returned by the JVMTI</p><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Hi Alexander,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">This test is unnecessarily complicated, it could be simplified a lot.</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">JvmtiGetAllModulesTest.java</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Move getModulesNative() into JvmtiGetAllModulesTest.java and have it return a Set<Module> to be able to use equals later</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@27 * @compile JvmtiGetAllModulesTest.java</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">No need for this, jtreg will compile it for you</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@45 & @67 </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Why is this check needed? Why are there least 3 unnamed modules?</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@50</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Change this to: assertTrue(Layer.boot().equals(getModulesNative()));</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@54</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">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</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@65</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Change this to an assertTrue using the layer containing the new module, similar to the change @50</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@73</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">No need for this method</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@81</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Change this method to use the Layer.defineModules() method to define a module instead, this eliminates the need for external JAR's </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">@98</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">No need for this method</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">If you use Layer.defineModules(), the following files can be removed:</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">JarBuilder.java</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">JavaModulesInfo.java</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">JvmtiModulesInfo.java</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">ModuleLoader.java </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">ModulesInfo.java </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">module-info.java</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Thanks,</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Christian</span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> </span></p><div><div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"> serviceability-dev [<a href="mailto:serviceability-dev-bounces@openjdk.java.net" target="_blank">mailto:serviceability-dev-bounces@openjdk.java.net</a>] <b>On Behalf Of </b><a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br><b>Sent:</b> Monday, May 2, 2016 6:06 PM<br><b>To:</b> Alexander Kulyakhtin <<a href="mailto:alexander.kulyakhtin@oracle.com" target="_blank">alexander.kulyakhtin@oracle.com</a>>; Serviceability-Dev <<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>><br><b>Subject:</b> Re: RFR:8153978:New test to verify the modules info as returned by the JVMTI</span></p></div></div><p class="MsoNormal"> </p><div><p class="MsoNormal">Hi Alexander,<br><br><br>Could you, fix a couple of minor issues?<br><br>test/serviceability/jvmti/GetModulesInfo/JvmtiGetAllModulesTest.java</p><pre> 58 for(Module mod : my.modules()) {</pre><pre> 59 if(!jvmtiModules.contains(mod)) {</pre><pre> </pre><pre> A space is missed after the 'for' and 'if' keywords.</pre><p class="MsoNormal"><br>test/serviceability/jvmti/GetModulesInfo/ModulesInfo.java.</p><pre> 31 boolean compareExcludingUnnamed(ModulesInfo other) {</pre><pre> </pre><pre> I'd suggest to call it compareNamed.</pre><p class="MsoNormal"><br>Otherwise, the new test looks great.<br>Thanks a lot for taking care about it!<br><br>Thanks,<br>Serguei<br><br><br><br>On 4/29/16 06:12, Alexander Kulyakhtin wrote:</p></div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><pre>Hi, </pre><pre> </pre><pre>Could you, please, review these test-only changes (adding a new test).</pre><pre> </pre><pre>CR: <a href="https://bugs.openjdk.java.net/browse/JDK-8153978" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8153978</a> "New test to verify the modules info as returned by the JVMTI"</pre><pre>Webrev: <a href="http://cr.openjdk.java.net/%7Eakulyakh/8153978_01/" target="_blank">http://cr.openjdk.java.net/~akulyakh/8153978_01/</a></pre><pre> </pre><pre>The new test verifies that JVMTI returns the correct info about the modules loaded at the application startup. </pre><pre>It also verifies that the returned info is consistent with the same info returned by the Java API.</pre><pre>It then loads a new named module and checks the correctness of the JVMTI info again.</pre><pre> </pre><pre>Due to a tools issue <a href="https://bugs.openjdk.java.net/browse/CODETOOLS-7901662" target="_blank">https://bugs.openjdk.java.net/browse/CODETOOLS-7901662</a> the test can only be pushed in when the updated jtreg is released.</pre><pre>The test passes fine with the nightly jtreg build, containing the CODETOOLS-7901662 fix.</pre><pre> </pre><pre>Best regards,</pre><pre>Alexander</pre></blockquote><p class="MsoNormal"> </p></div></div></blockquote><p class="MsoNormal"> </p></div></div></blockquote><p class="MsoNormal"> </p></blockquote><p class="MsoNormal"> </p></blockquote><p class="MsoNormal"> </p></div></div></div></div></div></div></div></body></html>