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

[Jini George]

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