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 >
