RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands
David Holmes
david.holmes at oracle.com
Thu Nov 16 02:51:50 UTC 2017
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 at 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 at 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 at 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 at 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
>>>
More information about the serviceability-dev
mailing list