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.geo...@oracle.com
<mailto:jini.geo...@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.geo...@oracle.com <mailto:jini.geo...@oracle.com>
> <mailto:jini.geo...@oracle.com
<mailto:jini.geo...@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
