RFR(M): 8078896: Add @modules as needed to the jdk_svc tests
Hi, Could I please have a review of this fix. bug: https://bugs.openjdk.java.net/browse/JDK-8078896 webrev: http://cr.openjdk.java.net/~ykantser/8078896 The push will be pushed to jdk9/dev. Thanks, Katja Alan's clarification from same change in hotspot (RFR: JDK-8075586: add @modules as needed to the open hotspot tests) ========== Just to put more context on this patch and similar patches that will be proposed for tests in other repos. The new @modules tag can be used for test selection (e.g. no point running a test that make use of types in module java.management if the runtime under test does not have the management module), and additionally to declare a dependency on JDK-internal APIs. If my test uses sun.misc.Unsafe for example then the runtime under test needs to export that API for the test to compile and run. Clearly the latter requires the module system in JDK 9 so consider this part as just preparing the tests for when that day comes. One other thing to say to say is that Alex has tooling to examine the test tree and create or change the @modules tag as needed. The intention is that this should run periodically to refresh the @modules tags. In this period before the module system then we can't expect everyone that is adding or changing tests to have internalized the module graph [1] and furthermore know which APIs are exported or not. In other words, the intention is not to needlessly burden anyone that adds or changes tests. Clearly making use of a new jtreg tag means everyone with a local copy of jtreg should grab a recent build. Finally, I hope in time that the TEST.groups files can be cleaned up to remove the needs_* groups, this is important for the group definitions in the jdk repo mostly (but there are some in the hotspot TEST.groups too). -Alan
On 05/05/2015 10:04, Yekaterina Kantserova wrote:
Hi,
Could I please have a review of this fix.
bug: https://bugs.openjdk.java.net/browse/JDK-8078896 webrev: http://cr.openjdk.java.net/~ykantser/8078896
The push will be pushed to jdk9/dev.
Thanks Katja, this looks good. One thing that we should as part of this is rev the requiredVersion in jdk/test/TEST.ROOT in case people are using older versions of jtreg. I think you did that as part of the patch for the hotspot repo. -Alan.
Alan, Thanks for the review! And for the catch - I'll fix it. // Katja On 05/05/2015 03:30 PM, Alan Bateman wrote:
Thanks Katja, this looks good. One thing that we should as part of this is rev the requiredVersion in jdk/test/TEST.ROOT in case people are using older versions of jtreg. I think you did that as part of the patch for the hotspot repo.
-Alan.
On 05/05/2015 02:04 AM, Yekaterina Kantserova wrote:
Hi,
Could I please have a review of this fix.
bug: https://bugs.openjdk.java.net/browse/JDK-8078896 webrev: http://cr.openjdk.java.net/~ykantser/8078896
com.sun.management has been moved to jdk.management module. The patch for JDK-8042901 is just integrated in jdk9/dev today. Most, if not all, test/com/sun/management tests need updates to require @modules jdk.management. There are several test/java/lang/management tests dependng on both java.lang.management and com.sun.management. For tests only using java.lang.management API, @modules java.management is adequate. test/com/sun/jdi/InterfaceMethodsTest.java test/com/sun/jdi/InterruptHangTest.java Since you have updated the end year of the copyright header in most tests, I spot a couple and so mention it to you. Otherwise looks good. Thanks for doing this. Mandy
On 05/05/2015 01:33 PM, Mandy Chung wrote:
On 05/05/2015 02:04 AM, Yekaterina Kantserova wrote:
Hi,
Could I please have a review of this fix.
bug: https://bugs.openjdk.java.net/browse/JDK-8078896 webrev: http://cr.openjdk.java.net/~ykantser/8078896
com.sun.management has been moved to jdk.management module. The patch for JDK-8042901 is just integrated in jdk9/dev today. Most, if not all, test/com/sun/management tests need updates to require @modules jdk.management.
There are several test/java/lang/management tests dependng on both java.lang.management and com.sun.management. For tests only using java.lang.management API, @modules java.management is adequate.
test/com/sun/jdi/InterfaceMethodsTest.java test/com/sun/jdi/InterruptHangTest.java Since you have updated the end year of the copyright header in most tests, I spot a couple and so mention it to you.
Otherwise looks good. Thanks for doing this.
About the test selection, one typical aspect of svc tests is to run a j* tool in a child process (e.g. jinfo, jstack, jstat, jstatd,jcmd, jps etc that are in jdk.jcmd module). I would expect all test/sun/tools/jcmd tests should have @modules jdk.jcmd and similiar for other sun/tools/j* tests. Are you thinking to come back in the next round of test selection update? Mandy
On 05/05/2015 22:00, Mandy Chung wrote:
:
About the test selection, one typical aspect of svc tests is to run a j* tool in a child process (e.g. jinfo, jstack, jstat, jstatd,jcmd, jps etc that are in jdk.jcmd module). I would expect all test/sun/tools/jcmd tests should have @modules jdk.jcmd and similiar for other sun/tools/j* tests. Are you thinking to come back in the next round of test selection update?
Alexander Kulyakhtin's tool hasn't been pushed to jdk9/dev yet but I expect it will be run regularly to keep the tests updated. For modules then we're mostly interested in the tests that make use of JDK-internal APIs of course, the test selection aspect is for later when jtreg has support for this. -Alan
Mandy, Thanks fro your review! Please see my comment inlined. On 05/05/2015 11:00 PM, Mandy Chung wrote:
com.sun.management has been moved to jdk.management module. The patch for JDK-8042901 is just integrated in jdk9/dev today. Most, if not all, test/com/sun/management tests need updates to require @modules jdk.management.
There are several test/java/lang/management tests dependng on both java.lang.management and com.sun.management. For tests only using java.lang.management API, @modules java.management is adequate.
test/com/sun/jdi/InterfaceMethodsTest.java The copyright header contains already 2015. test/com/sun/jdi/InterruptHangTest.java The test has been missing copyrights, I've updated it with the copyright header. Since you have updated the end year of the copyright header in most tests, I spot a couple and so mention it to you.
Otherwise looks good. Thanks for doing this.
The new webrev can be found here: http://cr.openjdk.java.net/~ykantser/8078896/webrev.02/
About the test selection, one typical aspect of svc tests is to run a j* tool in a child process (e.g. jinfo, jstack, jstat, jstatd,jcmd, jps etc that are in jdk.jcmd module). I would expect all test/sun/tools/jcmd tests should have @modules jdk.jcmd and similiar for other sun/tools/j* tests. Are you thinking to come back in the next round of test selection update?
As Alan pointed in his answer to this mail JTreg is not ready yet. I prefer to do it when it will be possible to test this change. Best regards, Katja
Mandy
On 05/06/2015 04:21 AM, Yekaterina Kantserova wrote:
The new webrev can be found here: http://cr.openjdk.java.net/~ykantser/8078896/webrev.02/
Looks good.
About the test selection, one typical aspect of svc tests is to run a j* tool in a child process (e.g. jinfo, jstack, jstat, jstatd,jcmd, jps etc that are in jdk.jcmd module). I would expect all test/sun/tools/jcmd tests should have @modules jdk.jcmd and similiar for other sun/tools/j* tests. Are you thinking to come back in the next round of test selection update?
As Alan pointed in his answer to this mail JTreg is not ready yet. I prefer to do it when it will be possible to test this change.
Right the test selection is yet to be supported by jtreg and it's okay to add those in the next round. As the tool analyzing the test dependencies doesn't detect that, these multi-process tests would need to be analyzed in a different way. Anyway, this patch looks good. Mandy
participants (3)
-
Alan Bateman
-
Mandy Chung
-
Yekaterina Kantserova