RFR:8148103:add more tests for task "Update JDI and JDWP for modules"

[serguei.spitsyn at oracle.com]

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 at oracle.com
> To: alexander.kulyakhtin at oracle.com, serviceability-dev at openjdk.java.net
> Cc: christian.tornqvist at 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
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160829/2e7bf6ff/attachment-0001.html>