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

[Jini George]

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