Hi Sergey, Thank you very much for the review. I've submitted JDK-8165017 as you have suggested.
Best regards, Alexander ----- Original Message ----- From: serguei.spit...@oracle.com To: alexander.kulyakh...@oracle.com Cc: christian.tornqv...@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.spit...@oracle.com To: alexander.kulyakh...@oracle.com , serviceability-dev@openjdk.java.net Cc: christian.tornqv...@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
