Jaroslav, First of all, thanks for the quick response and sorry for my slow one (your message didn't sort into the mail folder I expected, so didn't see it until now).
Secondly, all good comments; all fixed. Updated webrev: http://cr.openjdk.java.net/~miauno/8071908_8071909/webrev.02/ Thanks, Mikael On 2015-01-30 11:18, Jaroslav Bachorik wrote: > Hi Mikael, > > it's great to see this moving forward! > > Comments follow: > > * instead of throwing a RuntimeException from within the test classes > you could use o.testng.Assert.fail(...) method > > * all the newly introduced tests are missing @summary > > * test/serviceability/dcmd/compiler/CodelistTest.java > L43 - unused import > L68 - an unused, commented out, line > > * test/testlibrary/com/oracle/java/testlibrary/OutputAnalyzer.java > L436-438 - use Arrays.asList() > > * test/serviceability/dcmd/vm/UptimeTest.java > L44 - spurious wakeups may cause the test fail intermittently; should > make sure the wait was at least 'someUptime' seconds long > > > -JB- > > On 30.1.2015 10:44, Mikael Auno wrote: >> Hi, could I please some reviews for this test port? >> >> Issues: https://bugs.openjdk.java.net/browse/JDK-8071908 >> https://bugs.openjdk.java.net/browse/JDK-8071909 >> Webrev: http://cr.openjdk.java.net/~miauno/8071908_8071909/webrev.00/ >> >> Read on for the rationale on a few questions that might arise. >> >> * Why two issues? >> >> These changes are mainly a port of the Diagnostic Command (DCMD) tests >> and corresponding framework/utility classes from an internal (non-open) >> test framework to jtreg. The reason for the two issues is that the >> changes depend on a few modifications to testlibrary that are available >> in jdk/test but not yet in hotspot/test, as well as a small new addition >> to OutputAnalyzer, that are not specific to main subject (i.e. the DCMD >> tests). To keep the history as clean and coherent as possible, those >> changes will go in under JDK-8071909 while the new tests and >> DCMD-related additions to testlibrary go in under JDK-8071908. >> >> * Isn't there already utility classes for calling Diagnostic Commands? >> >> The main idea with the new utility classes in testlibrary is to provide >> a single interface to call Diagnostic Commands from tests, regardless of >> the transport used (e.g. jcmd or JMX). There are a few tests scattered >> around jdk/test and hotspot/test today that already utilize Diagnostic >> Commands in some way, but they either use different utility classes for >> this in different places or just do it directly in the test. Also, some >> of these utility classes or tests go through jcmd and some through JMX >> (most often without any real requirement for one transport over the >> other in the test). All this means that there are, today, numerous >> different implementations for calling Diagnostic Commands, and >> consequently a lot of code duplication. These utility classes are meant >> to replace all of these implementations, and with a single interface >> regardless of the transport at that. >> >> * You've missed this or that test using one of the existing utility >> classes! >> >> This is "by design". In order to keep the change at a more manageable >> size and to get the core of this change in sooner, we've chosen to do >> this transition in multiple steps. The first of those steps is what is >> in this review request; the core utility classes, the tests ported from >> the internal test framework and the adaption of the tests already in >> hotspot/test/serviceability/dcmd (since they happened to reside in the >> directory where we wanted to put the ported tests). When this is >> integrated and have gone a few rounds through nightly testing, the >> adaption of other tests in hotspot/test will follow, and after that >> jdk/test. >> >> Thanks, >> Mikael >> >
