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

List:       openjdk-hotspot-runtime-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-02-02 15:40:01
Message-ID: 54CF9A51.8090309 () oracle ! com
[Download RAW message or body]

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

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