[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(L): 8071908, 8071909: Port internal Diagnostic Command tests and test framework to jtreg (pl
From:       Mikael Auno <mikael.auno () oracle ! com>
Date:       2015-01-30 20:19:23
Message-ID: 54CBE74B.7040104 () oracle ! com
[Download RAW message or body]

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
>>>>
>>>
>>
> 

[prev in list] [next in list] [prev in thread] [next in thread]