[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for som
From: Sharath Ballal <sharath.ballal () oracle ! com>
Date: 2017-11-16 3:53:03
Message-ID: df727f63-32a9-4b4e-9f4a-a977e0d82436 () default
[Download RAW message or body]
Thanks David.
Thanks,
Sharath
-----Original Message-----
From: David Holmes
Sent: Thursday, November 16, 2017 8:22 AM
To: Sharath Ballal; serviceability-dev@openjdk.java.net
Subject: Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands \
tests and testcases for some of the commands
That looks fine.
Thanks,
David
On 15/11/2017 7:58 PM, Sharath Ballal wrote:
>
> Thanks David. Here is the revised webrev after addressing the comments:
>
> http://cr.openjdk.java.net/~sballal/8190198/webrev.04/
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: David Holmes
> Sent: Wednesday, November 15, 2017 7:26 AM
> To: Sharath Ballal; serviceability-dev@openjdk.java.net
> Subject: Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb
> clhsdb' commands tests and testcases for some of the commands
>
> Hi Sharath,
>
> This is looking very good.
>
> A few comments below.
>
> On 14/11/2017 3:32 PM, Sharath Ballal wrote:
> > My changes with using the outputAnalyzer were creating timeouts. This was seen \
> > with testcases creating more output. As per the documentation of Process class \
> > this is expected as I was creating the outputAnalyzer after doing \
> > Process.waitFor().
> > " Because some native platforms only provide limited buffer size for standard \
> > input and output streams, failure to promptly write the input stream or read the \
> > output stream of the process may cause the process to block, or even deadlock."
> > Hence I made changes to create the outputAnalyzer before Process.waitFor(). \
> > outputAnalyzer takes care of creating threads and reading the process output and \
> > error and hence we should not have the same problem. The tests are passing on \
> > Mach5.
> > Here is the latest webrev:
> > http://cr.openjdk.java.net/~sballal/8190198/webrev.03/
>
> General comments:
>
> Please add @bug to each test header.
>
> I would not expect you to need this in each test?
>
> * @modules java.base
>
> Style nit: you're using an unusual indentation for code like:
>
> 57 List<String> cmds = List.of(
> 58 "flags", "flags -nd",
> 59 "flags UseJVMCICompiler", "flags MaxFDLimit",
> 60 "flags MaxJavaStackTraceDepth");
>
> and
>
> 63 expStrMap.put("flags", List.of(
> 64 "UseJVMCICompiler = true",
> 65 "MaxFDLimit = false",
>
> but it's readable so I won't insist on any changes.
>
> ---
>
> test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java
>
> The @param names don't match the actual args on run/runCmd.
>
> 78 private String runCmd(List<String> commands,
> 79 Map<String, List<String>> expectedStrMap,
> 80 Map<String, List<String>> unExpectedStrMap)
>
> Indent is wrong on 79 and 80: Map should line up with List on 78.
>
> 144 public String run(long lingeredAppPid, List<String> commands,
> 145 Map<String, List<String>>
> expectedStrMap,
> 146 Map<String, List<String>>
> UnExpectedStrMap)
>
> Indent is wrong.
>
> ---
>
> test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java
> test/hotspot/jtreg/serviceability/sa/ClhsdbPstack.java
>
> You should use @requires to exclude execution on OS X rather than a Platform check \
> in the test.
> Thanks,
> David
>
>
> > Thanks,
> > Sharath
> >
> >
> > -----Original Message-----
> > From: Sharath Ballal
> > Sent: Tuesday, November 07, 2017 12:09 PM
> > To: David Holmes; serviceability-dev@openjdk.java.net
> > Subject: RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb
> > clhsdb' commands tests and testcases for some of the commands
> >
> > I have made changes to use the outputAnalyzer (thanks Jini).
> > Latest webrev is :
> > http://cr.openjdk.java.net/~sballal/8190198/webrev.02/
> >
> > Thanks,
> > Sharath
> >
> >
> > -----Original Message-----
> > From: Sharath Ballal
> > Sent: Sunday, November 05, 2017 10:04 PM
> > To: David Holmes; serviceability-dev@openjdk.java.net
> > Subject: RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb
> > clhsdb' commands tests and testcases for some of the commands
> >
> > Thanks David for the comments. Reply inline.
> > The new webrev is at
> > http://cr.openjdk.java.net/~sballal/8190198/webrev.01/
> >
> >
> > Thanks,
> > Sharath
> >
> >
> > -----Original Message-----
> > From: David Holmes
> > Sent: Monday, October 30, 2017 11:15 AM
> > To: Sharath Ballal; serviceability-dev@openjdk.java.net
> > Subject: Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb
> > clhsdb' commands tests and testcases for some of the commands
> >
> > Hi Sharath,
> >
> > I think you and Jini need to coordinate your current proposed changes. :) \
> > [Sharath Ballal] Jini is aware of these changes. She will modify the testcases \
> > later to use the new Launcher.
> > I have a few comments.
> >
> > On 30/10/2017 2:29 PM, Sharath Ballal wrote:
> > > Hello,
> > >
> > > This webrev has changes for a framework for running the 'jhsdb clhsdb'
> > > commands and verifying the output. It also has sanity tests for 8
> > > of
> >
> > I can't help but think the launcher should be able to utilize OutputAnalyzer. ??
> > [Sharath Ballal] clhsdb is an interactive command line tool. After launching \
> > clhsdb, we get a prompt. Further commands are typed on the prompt, finally we \
> > quit the tool. Example: => sudo \
> > /home/sharath/java/src/java10/hs_8189061/build/linux-x86_64-normal-server-fastdebug/images/jdk/bin/jhsdb \
> > clhsdb --pid 8306 Attaching to process 8306, please wait... hsdb>
> > hsdb>
> > hsdb> flags
> > ....
> > ......
> > ZombieALotInterval = 5 0
> > hashCode = 5 0
> > hsdb>
> > hsdb>
> >
> > My understanding is that OutputAnalyzer does not work with such cases (an \
> > earlier clhsdb testcase also does not use outputAnalyzer, \
> > open/test/jdk/sun/tools/jhsdb/BasicLauncherTest.java). I tried creating \
> > OutputAnalyzer after running the commands as shown below but the testcase times \
> > out. OutputAnalyzer outputAnalyzer = new OutputAnalyzer(toolProcess);
> > toolProcess.waitFor();
> > output = outputAnalyzer.getOutput();
> >
> > Do you require the input commands to include the "quit"? Is there a reason the \
> > launcher couldn't handle that itself? [Sharath Ballal] Launcher can do it. I \
> > have made the changes.
> > Can the launcher internalize the management of the LingeredApp?
> > [Sharath Ballal] LingeredApp can be derived and the subclass can add more \
> > specific things as per the testcase. For examples pls see \
> > DeadlockDetectionTest.java and LingeredAppWithDeadlock.java and other similar \
> > classes in the same directory. Hence I have left it up to the user to create an \
> > instance of LingeredApp and pass the pid as an argument.
> > Can the launcher also handle the shouldSAAttach check?
> > [Sharath Ballal] Yes. I have made that change.
> >
> > I can imagine the test logic reducing to a very simple:
> >
> > println(Starting test of ...
> > List<String> cmds = List.of( ...);
> > List<String> expected = List.of(...); List<String> unexpected =
> > Listd.of(...); ClhsdbLauncher.run(cmds, expected, unexpected); //
> > static method println("test PASSED");
> >
> > I don't see why the test classes should be subclasses of the Clhsdblauncher class \
> > - the tests are not launchers themselves. The launcher class is just a utility \
> > class that should have public rather than protected methods. [Sharath Ballal] I \
> > have done this change.
> > I'm not sure the approach of sending a set of commands, and having a set of \
> > expected outputs is the right/best way to test this. I would expect to see a \
> > check of the expected outcome for each command ie send a command, check the \
> > result, send the next command, etc. Or if you can put/detect delimiters in the \
> > output you could still send all the commands and then bulk process the output. \
> > But the present approach allows for the commands to actually do the wrong things, \
> > as long as the expected output appears somewhere. [Sharath Ballal] OK. I have \
> > done these changes.
> > It also unclear what output corresponds to what command and why. Eg in the \
> > longConstant test it is not obvious why you expect to see \
> > markOopDesc::epoch_mask_in_place, [Sharath Ballal] \
> > markOopDesc::epoch_mask_in_place is one of the longConstants that is printed by \
> > longConstant command.
> > or the difference in expected output
> > between:
> >
> > 57 "longConstant jtreg::test 6\n",
> > 58 "longConstant jtreg::test\n",
> >
> > I'm guessing the first silently declares and sets a variable, while the second \
> > reads it back - is that right? [Sharath Ballal] Yes.
> > I have provided a way to specify the expected/unexpected output per command and \
> > check it separately.
> > Thanks,
> > David
> >
> > > the clhsdb commands.
> > >
> > > Pls review the changes.
> > >
> > > Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190198
> > >
> > > Webrev: http://cr.openjdk.java.net/~sballal/8190198/webrev.00/
> > >
> > > Thanks,
> > >
> > > Sharath
> > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic