[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for som
From:       David Holmes <david.holmes () oracle ! com>
Date:       2017-10-30 5:44:31
Message-ID: 6824f2b2-0afb-24ea-5996-0942272570c3 () oracle ! com
[Download RAW message or body]

Hi Sharath,

I think you and Jini need to coordinate your current proposed changes. :)

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. ??

Do you require the input commands to include the "quit"? Is there a 
reason the launcher couldn't handle that itself?

Can the launcher internalize the management of the LingeredApp?

Can the launcher also handle the shouldSAAttach check?

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.

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.

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, 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?

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
> 
[prev in list] [next in list] [prev in thread] [next in thread]