Thanks everyone for the reviews. This has now been pushed. Mikael
On 2015-01-30 21:19, Mikael Auno wrote: > Thanks Jaroslav! > > I'll leave this open for further comments if someone else has them until > Monday afternoon (CET). If there are no more comments by then I'll get > it pushed at that time. > > Mikael > > On 2015-01-30 20:48, Jaroslav Bachorik wrote: >> Nice! For me it's good to go. >> >> -JB- >> >> On 30.1.2015 20:32, Mikael Auno wrote: >>> 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 >>>>> >>>> >>> >> >
