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

[Jini George]

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