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

[Jini George]

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