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

[JC Beyler]

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> 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>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190107/ee2a97c5/attachment.html>