Thanks David.
Thanks, Sharath -----Original Message----- From: David Holmes Sent: Thursday, November 16, 2017 8:22 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 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; [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 >>>
