RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal

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

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

David Holmes
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
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal
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; [hidden email]
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
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal
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; [hidden email]
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; [hidden email]
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
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal
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/ 

Thanks,
Sharath


-----Original Message-----
From: Sharath Ballal
Sent: Tuesday, November 07, 2017 12:09 PM
To: David Holmes; [hidden email]
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; [hidden email]
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; [hidden email]
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
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Jini George
Hi Sharath,

Your changes look fine to me. One nit:

http://cr.openjdk.java.net/~sballal/8190198/webrev.03/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java.html

* You should be able to remove the following line:

26 import java.util.HashMap;

Thanks,
Jini (Not a (R)eviewer).


On 11/14/2017 11:02 AM, 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/
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: Sharath Ballal
> Sent: Tuesday, November 07, 2017 12:09 PM
> To: David Holmes; [hidden email]
> 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; [hidden email]
> 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; [hidden email]
> 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
>>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal
Thanks Jini.  I have made the change.


Thanks,
Sharath


-----Original Message-----
From: Jini George
Sent: Tuesday, November 14, 2017 3:14 PM
To: Sharath Ballal; David Holmes; [hidden email]
Subject: Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Hi Sharath,

Your changes look fine to me. One nit:

http://cr.openjdk.java.net/~sballal/8190198/webrev.03/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java.html

* You should be able to remove the following line:

26 import java.util.HashMap;

Thanks,
Jini (Not a (R)eviewer).


On 11/14/2017 11:02 AM, 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/
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: Sharath Ballal
> Sent: Tuesday, November 07, 2017 12:09 PM
> To: David Holmes; [hidden email]
> 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; [hidden email]
> 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; [hidden email]
> 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
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

David Holmes
In reply to this post by Sharath Ballal
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; [hidden email]
> 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; [hidden email]
> 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; [hidden email]
> 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
>>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal

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; [hidden email]
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; [hidden email]
> 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; [hidden email]
> 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; [hidden email]
> 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
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

David Holmes
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; [hidden email]
> 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; [hidden email]
>> 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; [hidden email]
>> 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; [hidden email]
>> 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
>>>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: JDK-8190198 - SA: Framework for writing 'jhsdb clhsdb' commands tests and testcases for some of the commands

Sharath Ballal
Thanks David.


Thanks,
Sharath


-----Original Message-----
From: David Holmes
Sent: Thursday, November 16, 2017 8:22 AM
To: Sharath Ballal; [hidden email]
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; [hidden email]
> 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; [hidden email]
>> 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; [hidden email]
>> 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; [hidden email]
>> 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
>>>