RFR: JDK-8215568: Refactor SA clhsdb tests to use ClhsdbLauncher

[David Holmes]

Hi Jini,

This looks fine to me - Reviewed.

Thanks,
David

On 4/02/2019 11:39 pm, Jini George wrote:
> Pinging again -- requesting for a Reviewer to take a look.
> 
> The patch has been rebased again to include the changes of JDK-8217473 
> to rethrow SkippedException for the tests refactored to use ClhsdbLauncher.
> 
> webrev:
> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.04/index.html
> 
> Thanks,
> Jini.
> 
> On 1/16/2019 9:59 AM, Jini George wrote:
>> Ping!
>>
>> Need a Reviewer please.
>>
>> The patch rebased to the latest changes is at:
>>
>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.03/index.html
>>
>> Thanks,
>> Jini.
>>
>> On 1/10/2019 8:40 PM, Jini George wrote:
>>> Gentle reminder -- Could I please let a Reviewer to take a look at this?
>>>
>>> Thanks,
>>> Jini.
>>>
>>> On 1/8/2019 10:36 PM, Jini George wrote:
>>>> Thank you so much for the great catch, JC! Yes, indeed, the test 
>>>> passed inspite of 'printmado' being an unrecognized command. I have 
>>>> filed https://bugs.openjdk.java.net/browse/JDK-8216352 to handle 
>>>> issues like these.
>>>>
>>>> I have the corrected webrev at:
>>>>
>>>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.02/index.html
>>>>
>>>> Could I get a Reviewer also to take a look at this ?
>>>>
>>>> Thank you,
>>>> Jini.
>>>>
>>>> On 1/8/2019 12:12 AM, JC Beyler wrote:
>>>>> Hi Jini,
>>>>>
>>>>> I saw this typo:
>>>>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java.udiff.html 
>>>>>
>>>>>
>>>>> +            List<String> cmds = List.of("printmado -a");
>>>>>
>>>>> Should it not be printmdo and not printmado? does printmado exist? 
>>>>> If it doesn't how does the test pass (my guess is that we do not 
>>>>> catch a "unexpected command" and that the hashmaps are not finding 
>>>>> the keys so they are not checking the expected/unexpected results; 
>>>>> if so perhaps a follow-up should fix that an unknown command fails 
>>>>> a test trying to do that / and perhaps if the key is not found, the 
>>>>> test fails as well?)
>>>>>
>>>>> Thanks!
>>>>> Jc
>>>>>
>>>>> On Tue, Jan 1, 2019 at 9:07 PM Jini George <jini.george at oracle.com 
>>>>> <mailto:jini.george at oracle.com>> wrote:
>>>>>
>>>>>     Thank you very much for the review, JC. My comments inline.
>>>>>
>>>>>
>>>>>      > I saw this potential issue with one of the test conversions:
>>>>>      >
>>>>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java.udiff.html 
>>>>>
>>>>>      >     - It seems like there is a missing "unexpected" strings 
>>>>> check
>>>>>     here no?
>>>>>      >        - The original test was doing
>>>>>      > -
>>>>>      > -                if (line.contains("missing reason for ")) {
>>>>>      > -                    unexpected = new
>>>>>     RuntimeException("Unexpected msg:
>>>>>      > missing reason for ");
>>>>>      > -                    break;
>>>>>      > -                }
>>>>>      >
>>>>>      > whereas the new test is not seemingly (though
>>>>>      >
>>>>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java.udiff.html 
>>>>>
>>>>>
>>>>>      > does do it so I think this is an oversight?).
>>>>>
>>>>>     Thank you very much for pointing this out! This was an 
>>>>> oversight. I
>>>>>     have
>>>>>     added the unexpected strings check.
>>>>>
>>>>>      >
>>>>>      >     - Also interesting is that the original test was trying to
>>>>>     find one
>>>>>      > of X:
>>>>>      > -                if (line.contains("VirtualCallData")  ||
>>>>>      > -                    line.contains("CounterData")      ||
>>>>>      > -                    line.contains("ReceiverTypeData")) {
>>>>>      > -                    knownProfileDataTypeFound = true;
>>>>>      > -                }
>>>>>      >
>>>>>      > whereas you are now wanting to find all of them. Is that
>>>>>     normal/wanted?
>>>>>
>>>>>     I was being extra cautious when I had written this test case in 
>>>>> the
>>>>>     beginning :-). As far as I have seen, the printmdo output does 
>>>>> contain
>>>>>     all of these. (The test passes with 50 repeated runs across all hs
>>>>>     tiers
>>>>>     with the changes -- so I believe it is OK).
>>>>>
>>>>>     I have the new webrev at:
>>>>>
>>>>>     http://cr.openjdk.java.net/~jgeorge/8215568/webrev.01/
>>>>>
>>>>>     I have additionally modified the copyright year to 2019.
>>>>>
>>>>>     Thank you,
>>>>>     Jini.
>>>>>
>>>>>
>>>>>      >
>>>>>      > The rest looked good to me, though I wish there were a way 
>>>>> to not
>>>>>     have
>>>>>      > to change all the strings to be regex friendly but I fail to 
>>>>> see
>>>>>     how to
>>>>>      > do that without writing a runCmdWithoutRegexMatcher, which 
>>>>> seems
>>>>>      > overkill :-)
>>>>>      > Jc
>>>>>      >
>>>>>      > On Tue, Dec 18, 2018 at 8:55 PM Jini George
>>>>>     <jini.george at oracle.com <mailto:jini.george at oracle.com>
>>>>>      > <mailto:jini.george at oracle.com 
>>>>> <mailto:jini.george at oracle.com>>>
>>>>>     wrote:
>>>>>      >
>>>>>      >     Hello!
>>>>>      >
>>>>>      >     Requesting reviews for:
>>>>>      >
>>>>>      > http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/
>>>>>      >
>>>>>      >     BugID: https://bugs.openjdk.java.net/browse/JDK-8215568
>>>>>      >
>>>>>      >     No new failures with the SA tests (hs-tiers 1-5) with these
>>>>>     changes.
>>>>>      >     The
>>>>>      >     changes here include:
>>>>>      >
>>>>>      >     * Refactoring the SA tests which test clhsdb commands to 
>>>>> use
>>>>>      >     ClhsdbLauncher for uniformity and ease of maintainence.
>>>>>      >     * Testing for regular expressions with shouldMatch 
>>>>> rather than
>>>>>      >     shouldContain.
>>>>>      >     * Minor cleanups.
>>>>>      >
>>>>>      >     Thank you,
>>>>>      >     Jini.
>>>>>      >
>>>>>      >
>>>>>      >
>>>>>      >
>>>>>      > --
>>>>>      >
>>>>>      > Thanks,
>>>>>      > Jc
>>>>>
>>>>>
>>>>>
>>>>> -- 
>>>>>
>>>>> Thanks,
>>>>> Jc