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