[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8215568: Refactor SA clhsdb tests to use ClhsdbLauncher
From:       Jini George <jini.george () oracle ! com>
Date:       2019-01-16 4:41:51
Message-ID: 2744c4d7-a75b-a916-9b6a-e746cc6c3e50 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]