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>> 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
