RFR: JDK-8215568: Refactor SA clhsdb tests to use ClhsdbLauncher
Jini George
jini.george at oracle.com
Tue Feb 5 02:27:00 UTC 2019
Thank you very much, David.
- Jini.
On 2/5/2019 6:40 AM, David Holmes wrote:
> Hi Jini,
>
> This looks fine to me - Reviewed.
>
> Thanks,
> David
>
> On 4/02/2019 11:39 pm, Jini George wrote:
>> Pinging again -- requesting for a Reviewer to take a look.
>>
>> The patch has been rebased again to include the changes of JDK-8217473
>> to rethrow SkippedException for the tests refactored to use
>> ClhsdbLauncher.
>>
>> webrev:
>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.04/index.html
>>
>> Thanks,
>> Jini.
>>
>> On 1/16/2019 9:59 AM, Jini George wrote:
>>> Ping!
>>>
>>> Need a Reviewer please.
>>>
>>> The patch rebased to the latest changes is at:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.03/index.html
>>>
>>> Thanks,
>>> Jini.
>>>
>>> On 1/10/2019 8:40 PM, Jini George wrote:
>>>> 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
More information about the serviceability-dev
mailing list