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; [email protected]
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; [email protected]
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; [email protected]
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
