[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR:8148103:add more tests for task "Update JDI and JDWP for modules"
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2016-08-30 10:24:29
Message-ID: 57C55EDD.30206 () oracle ! com
[Download RAW message or body]
Hi Alexander,
On 8/30/16 02:56, Alexander Kulyakhtin wrote:
> Hi Sergey,
>
> Thank you very much for the review. I've submitted JDK-8165017
> <https://bugs.openjdk.java.net/browse/JDK-8165017> as you have suggested.
Ok, thanks!
Is it enough to have one review for test enhancements?
Also, do you have a sponsor for push?
Thanks,
Serguei
>
> Best regards,
> Alexander
>
> ----- Original Message -----
> From: serguei.spitsyn@oracle.com
> To: alexander.kulyakhtin@oracle.com
> Cc: christian.tornqvist@oracle.com, serviceability-dev@openjdk.java.net
> Sent: Monday, August 29, 2016 10:00:17 PM GMT +03:00 Iraq
> Subject: Re: RFR:8148103:add more tests for task "Update JDI and JDWP
> for modules"
>
> Hi Alexander,
>
> It looks good.
> Thank you for the update!
>
> Please, file an enhancement CR on the additional test coverage
> for ClassLoader=2 and Module=19 commands.
>
> Thanks,
> Serguei
>
>
>
> On 8/29/16 05:29, Alexander Kulyakhtin wrote:
>
> Hi Sergey,
>
> Thank you very much for your help. With the changes, as you have
> proposed, the test now passes on all configurations.
>
> Please, find the updated webrev at:
> http://cr.openjdk.java.net/~akulyakh/8148103_03/
>
> Regarding a more thorough verification of the Classloader command,
> I suggest submitting a new enhancement CR for that, so that the
> current test can be integrated while we are working on the
> enhancement.
>
> Best regards,
> Alexander
>
>
> ----- Original Message -----
> From: serguei.spitsyn@oracle.com
> To: alexander.kulyakhtin@oracle.com,
> serviceability-dev@openjdk.java.net
> Cc: christian.tornqvist@oracle.com
> Sent: Friday, August 26, 2016 12:03:54 AM GMT +03:00 Iraq
> Subject: Re: RFR:8148103:add more tests for task "Update JDI and
> JDWP for modules"
>
> Alexander,
>
> The fix is pretty good.
>
> Some comments besides the bug 8164490 we already discussed.
>
> test/serviceability/jdwp/AllModulesCommandTest.java
>
> 75 System.err.println("Could not launch the debuggee. Error: '" + line + \
> "'");
> A suggestion to replace "Error: " => Error at line: ".
> Otherwise, the line number will be confused with the error code.
>
> 81 // Etsablish JDWP socket connection
>
> A typo: Etsablish => Establish
>
>
> The verification in the assertClassLoader is pretty weak.
> It'd be nice to have more sophisticated verification.
> For instance, the following commands can be used for such
> verification:
> cmdset: ClassLoaderReference=14 , cmd: VisibleClasses=1
> cmdset: ReferenceType=2, cmd: ClassLoader=2
> cmdset: ReferenceType=2, cmd: Module=19
>
> It is possible to iterate over all classes of the class loader
> to find
> at least one class with the given module and check if the
> class's class loader
> is the same as module's class loader.
> BTW, this would test the new jdwp command added for Jigsaw support:
> cmdset: ReferenceType=2, cmd: Module=19
>
> But I leave it up to you as it looks unreasonably complicated.
> Maybe, you find a better approach.
>
> test/serviceability/jdwp/AllModulesCommandTestDebuggee.java
>
> 41 Set<String> modNames = new HashSet<>();
> 42 for (Module mod : Layer.boot().modules()) {
> 43 modNames.add(mod.getName());
> 44 String info = String.format("module %s", mod.getName());
> 45 write(info);
> 46 }
>
> The local modNames is not really used and can be removed (lines
> 41, 43).
>
>
> test/serviceability/jdwp/Arch.java
>
> It seems, this file/class is not really needed and can be removed.
>
>
> test/serviceability/jdwp/JdwpReply.java
>
> Nit: the empty lines 39 and 54 are not needed.
>
>
>
> Thanks,
> Serguei
>
>
> On 8/12/16 05:55, Alexander Kulyakhtin wrote:
>
> Hi,
>
> Could you, please, review the following test-only change (adding a new test):
>
> CR:https://bugs.openjdk.java.net/browse/JDK-8148103
> Webrev:http://cr.openjdk.java.net/~akulyakh/8148103_02/
>
> The new test verifies the new JDWP commands: AllModules, Module, Name, ClassLoader, \
> CanRead.
> It does so by launching a debuggee java program with the necessary JDWP-related \
> options, so that a JDWP session can be established between the debuggee and the \
> test.
> When started the debuggee reports its loaded modules to the test.
> The test then initiates the JDWP session and issues AllModules command to get the \
> modules info by means of the JDWP. For each module the test issues Name command. It \
> then verifies that the modules names reported via the JDWP are the same as reported \
> by the debuggee using the Java API. Additionally, for each module the test issues \
> CanRead and Classloader commands and verifies that the corresponding replies are \
> correct.
> Since all the previous JDWP tests were implemented using the deprecated closed test \
> framework, the amount of the code for this test is slightly larger than usually for \
> a single test. The simple JDWP framework, created for this test, allows for porting \
> the other JDWP tests to the jtreg in the same manner.
> Best regards,
> Alexander
>
>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Alexander,<br>
<br>
<br>
On 8/30/16 02:56, Alexander Kulyakhtin wrote:<br>
</div>
<blockquote cite="mid:dfb043a5-0e17-4577-9538-1500e7e36153@default"
type="cite">
<meta http-equiv="Context-Type" content="text/html; charset=UTF-8">
<div>Hi Sergey,<br>
<br>
Thank you very much for the review. I've submitted <a
moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8165017">JDK-8165017</a>
as you have suggested.<br>
</div>
</blockquote>
<br>
Ok, thanks!<br>
Is it enough to have one review for test enhancements?<br>
Also, do you have a sponsor for push?<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote cite="mid:dfb043a5-0e17-4577-9538-1500e7e36153@default"
type="cite">
<div><br>
Best regards,<br>
Alexander<br>
<br>
----- Original Message -----<br>
From: <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a><br>
To: <a class="moz-txt-link-abbreviated" \
href="mailto:alexander.kulyakhtin@oracle.com">alexander.kulyakhtin@oracle.com</a><br>
Cc: <a class="moz-txt-link-abbreviated" \
href="mailto:christian.tornqvist@oracle.com">christian.tornqvist@oracle.com</a>,
<a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><br>
Sent: Monday, August 29, 2016 10:00:17 PM GMT +03:00 Iraq<br>
Subject: Re: RFR:8148103:add more tests for task "Update JDI and
JDWP for modules"<br>
<br>
<div>
<div class="moz-cite-prefix">Hi Alexander,<br>
<br>
It looks good.<br>
Thank you for the update!<br>
<br>
Please, file an enhancement CR on the additional test
coverage<br>
for ClassLoader=2 and Module=19 commands.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 8/29/16 05:29, Alexander Kulyakhtin wrote:<br>
</div>
<blockquote
cite="mid:1e141916-8d0c-4061-829d-7e819f5e5b0e@default">
<div>Hi Sergey,<br>
<br>
Thank you very much for your help. With the changes, as
you have proposed, the test now passes on all
configurations. <br>
<br>
Please, find the updated webrev at:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eakulyakh/8148103_03/"
target="_blank">http://cr.openjdk.java.net/~akulyakh/8148103_03/</a><br>
<br>
Regarding a more thorough verification of the Classloader
command, I suggest submitting a new enhancement CR for
that, so that the current test can be integrated while we
are working on the enhancement.<br>
<br>
Best regards,<br>
Alexander<br>
<br>
<br>
----- Original Message -----<br>
From: <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a><br> To: <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:alexander.kulyakhtin@oracle.com"
target="_blank">alexander.kulyakhtin@oracle.com</a>, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serviceability-dev@openjdk.java.net"
target="_blank"><a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a></a><br>
Cc: <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:christian.tornqvist@oracle.com"
target="_blank">christian.tornqvist@oracle.com</a><br>
Sent: Friday, August 26, 2016 12:03:54 AM GMT +03:00 Iraq<br>
Subject: Re: RFR:8148103:add more tests for task "Update
JDI and JDWP for modules"<br>
<br>
<div>
<div class="moz-cite-prefix">Alexander,<br>
<br>
The fix is pretty good.<br>
<br>
Some comments besides the bug 8164490 we already
discussed.<br>
<br>
test/serviceability/jdwp/AllModulesCommandTest.java<br>
<br>
<pre> 75 System.err.println("Could not launch the \
debuggee. Error: '" + line + "'");</pre> A suggestion to replace "Error: " => \
Error at line: ".<br>
Otherwise, the line number will be confused with
the error code.<br>
<br>
<pre> 81 // Etsablish JDWP socket connection</pre>
A typo: Etsablish => Establish<br>
<br>
<br>
The verification in the assertClassLoader is pretty
weak.<br>
It'd be nice to have more sophisticated
verification.<br>
For instance, the following commands can be used
for such verification:<br>
cmdset: ClassLoaderReference=14 , cmd:
VisibleClasses=1 <br>
cmdset: ReferenceType=2, cmd: ClassLoader=2<br>
cmdset: ReferenceType=2, cmd: Module=19<br>
<br>
It is possible to iterate over all classes of the
class loader to find<br>
at least one class with the given module and check
if the class's class loader<br>
is the same as module's class loader.<br>
BTW, this would test the new jdwp command added for
Jigsaw support:<br>
cmdset: ReferenceType=2, cmd: Module=19<br>
<br>
But I leave it up to you as it looks unreasonably
complicated.<br>
Maybe, you find a better approach.<br>
<br>
test/serviceability/jdwp/AllModulesCommandTestDebuggee.java<br>
<br>
<pre> 41 Set<String> modNames = new \
HashSet<>(); 42 for (Module mod : Layer.boot().modules()) {
43 modNames.add(mod.getName());
44 String info = String.format("module %s", mod.getName());
45 write(info);
46 }
</pre>
The local modNames is not really used and can be
removed (lines 41, 43).<br>
<br>
<br>
test/serviceability/jdwp/Arch.java<br>
<br>
It seems, this file/class is not really needed and
can be removed. <br>
<br>
<br>
test/serviceability/jdwp/JdwpReply.java<br>
<br>
Nit: the empty lines 39 and 54 are not needed.<br>
<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 8/12/16 05:55, Alexander Kulyakhtin wrote:<br>
</div>
<blockquote
cite="mid:ba450714-b57e-4464-988d-b46eeae88d9b@default">
<pre>Hi,
Could you, please, review the following test-only change (adding a new test):
CR: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8148103" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8148103</a>
Webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Eakulyakh/8148103_02/" \
target="_blank">http://cr.openjdk.java.net/~akulyakh/8148103_02/</a>
The new test verifies the new JDWP commands: AllModules, Module, Name, ClassLoader, \
CanRead.
It does so by launching a debuggee java program with the necessary JDWP-related \
options, so that a JDWP session can be established between the debuggee and the test.
When started the debuggee reports its loaded modules to the test.
The test then initiates the JDWP session and issues AllModules command to get the \
modules info by means of the JDWP. For each module the test issues Name command. It \
then verifies that the modules names reported via the JDWP are the same as reported \
by the debuggee using the Java API. Additionally, for each module the test issues \
CanRead and Classloader commands and verifies that the corresponding replies are \
correct.
Since all the previous JDWP tests were implemented using the deprecated closed test \
framework, the amount of the code for this test is slightly larger than usually for a \
single test. The simple JDWP framework, created for this test, allows for porting the \
other JDWP tests to the jtreg in the same manner.
Best regards,
Alexander
</pre>
</blockquote>
<br>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic