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
