RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

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

RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Jini George
Hello,

We have been working on writing sanity tests for the various jhsdb
clhsdb commands of the SA to improve the robustness of the SA. As a part
of this, here is a webrev for sanity tests for 3 of the clhsdb commands:

Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/

I would like to request for reviews for this simple addition.

Thank you,
Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

David Holmes
Hi Jini,

OutputAnalyzer has a bunch of methods that deal with checking return
values and output contents, and throwing exceptions (not assertion
failures!) so that you don't need to manually do all that. It also
prints diagnostics to aid with debugging when things do not contain what
is expected.

Looking at the individual tests:

TestIntConstant.java

  92
Asserts.assertTrue(output.contains("CollectedHeap::G1CollectedHeap 2"));
   93         Asserts.assertTrue(output.contains("RUNNABLE 2"));
   94         Asserts.assertTrue(output.contains("_thread_uninitialized
0"));
   95         Asserts.assertTrue(output.contains("intConstant
_temp_constant 45"));

Apart from  the last line this expected output looks very strange. If
this indeed what we expect from issuing those commands we should
document/comment what corresponds to what.

Plus this assumes G1 is being used but Lingered App is started using the
test run vmOptions AFAICS so it would use whatever GC were passed
through, wouldn't it?

---

TestType.java

The expected output is not quite so strange, but still could do with
some commentary.

   90         Asserts.assertTrue(output.contains("type G1CollectedHeap
CollectedHeap"));

Same comment about assuming G1.

Thanks,
David
------

On 30/10/2017 1:44 PM, Jini George wrote:

> Hello,
>
> We have been working on writing sanity tests for the various jhsdb
> clhsdb commands of the SA to improve the robustness of the SA. As a part
> of this, here is a webrev for sanity tests for 3 of the clhsdb commands:
>
> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>
> I would like to request for reviews for this simple addition.
>
> Thank you,
> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

David Holmes
Correction. Having now looked at Sharath's proposal (please see my
comments there) I understand the test structure better.

On 30/10/2017 3:05 PM, David Holmes wrote:

> Hi Jini,
>
> OutputAnalyzer has a bunch of methods that deal with checking return
> values and output contents, and throwing exceptions (not assertion
> failures!) so that you don't need to manually do all that. It also
> prints diagnostics to aid with debugging when things do not contain what
> is expected.
>
> Looking at the individual tests:
>
> TestIntConstant.java
>
>   92 Asserts.assertTrue(output.contains("CollectedHeap::G1CollectedHeap
> 2"));
>    93         Asserts.assertTrue(output.contains("RUNNABLE 2"));
>    94         Asserts.assertTrue(output.contains("_thread_uninitialized
> 0"));
>    95         Asserts.assertTrue(output.contains("intConstant
> _temp_constant 45"));
>
> Apart from  the last line this expected output looks very strange. If
> this indeed what we expect from issuing those commands we should
> document/comment what corresponds to what.
>
> Plus this assumes G1 is being used but Lingered App is started using the
> test run vmOptions AFAICS so it would use whatever GC were passed
> through, wouldn't it?

Okay the above are just examples of "int constants" that will be printed
when you dump all the "int constants". The G1 constant doesn't indicate
(I presume) that G1 is the active GC.

As I said to Sharath it would be good to validate the results of each
command separately rather than collectively.

Thanks,
David

> ---
>
> TestType.java
>
> The expected output is not quite so strange, but still could do with
> some commentary.
>
>    90         Asserts.assertTrue(output.contains("type G1CollectedHeap
> CollectedHeap"));
>
> Same comment about assuming G1.
>
> Thanks,
> David
> ------
>
> On 30/10/2017 1:44 PM, Jini George wrote:
>> Hello,
>>
>> We have been working on writing sanity tests for the various jhsdb
>> clhsdb commands of the SA to improve the robustness of the SA. As a
>> part of this, here is a webrev for sanity tests for 3 of the clhsdb
>> commands:
>>
>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>
>> I would like to request for reviews for this simple addition.
>>
>> Thank you,
>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Jini George
Thank you for the quick review, David. My comments inline:

On 10/30/2017 11:18 AM, David Holmes wrote:

>> Plus this assumes G1 is being used but Lingered App is started using
>> the test run vmOptions AFAICS so it would use whatever GC were passed
>> through, wouldn't it?
>
> Okay the above are just examples of "int constants" that will be printed
> when you dump all the "int constants". The G1 constant doesn't indicate
> (I presume) that G1 is the active GC.

Yes, the int constants contain compile time values and there are entries
for all the GC types.

> As I said to Sharath it would be good to validate the results of each
> command separately rather than collectively.

Will do.

Thanks!
- Jini.



> Thanks,
> David
>
>> ---
>>
>> TestType.java
>>
>> The expected output is not quite so strange, but still could do with
>> some commentary.
>>
>>    90         Asserts.assertTrue(output.contains("type G1CollectedHeap
>> CollectedHeap"));
>>
>> Same comment about assuming G1.
>>
>> Thanks,
>> David
>> ------
>>
>> On 30/10/2017 1:44 PM, Jini George wrote:
>>> Hello,
>>>
>>> We have been working on writing sanity tests for the various jhsdb
>>> clhsdb commands of the SA to improve the robustness of the SA. As a
>>> part of this, here is a webrev for sanity tests for 3 of the clhsdb
>>> commands:
>>>
>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>
>>> I would like to request for reviews for this simple addition.
>>>
>>> Thank you,
>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Jini George
Here is the updated webrev:

http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/

I have made changes to validate the test results of each command
separately, done away with the asserts and have added some more comments.

Thank you,
Jini.

On 10/31/2017 12:32 PM, Jini George wrote:

> Thank you for the quick review, David. My comments inline:
>
> On 10/30/2017 11:18 AM, David Holmes wrote:
>
>>> Plus this assumes G1 is being used but Lingered App is started using
>>> the test run vmOptions AFAICS so it would use whatever GC were passed
>>> through, wouldn't it?
>>
>> Okay the above are just examples of "int constants" that will be
>> printed when you dump all the "int constants". The G1 constant doesn't
>> indicate (I presume) that G1 is the active GC.
>
> Yes, the int constants contain compile time values and there are entries
> for all the GC types.
>
>> As I said to Sharath it would be good to validate the results of each
>> command separately rather than collectively.
>
> Will do.
>
> Thanks!
> - Jini.
>
>
>
>> Thanks,
>> David
>>
>>> ---
>>>
>>> TestType.java
>>>
>>> The expected output is not quite so strange, but still could do with
>>> some commentary.
>>>
>>>    90         Asserts.assertTrue(output.contains("type
>>> G1CollectedHeap CollectedHeap"));
>>>
>>> Same comment about assuming G1.
>>>
>>> Thanks,
>>> David
>>> ------
>>>
>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>> Hello,
>>>>
>>>> We have been working on writing sanity tests for the various jhsdb
>>>> clhsdb commands of the SA to improve the robustness of the SA. As a
>>>> part of this, here is a webrev for sanity tests for 3 of the clhsdb
>>>> commands:
>>>>
>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>
>>>> I would like to request for reviews for this simple addition.
>>>>
>>>> Thank you,
>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

David Holmes
Hi Jini,

On 3/11/2017 8:51 PM, Jini George wrote:
> Here is the updated webrev:
>
> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>
> I have made changes to validate the test results of each command
> separately, done away with the asserts and have added some more comments.

This looks much better to me - thanks. This fragment:

           int exitValue = p.exitValue();
           OutputAnalyzer output = new OutputAnalyzer(p);
           System.out.println(output.getOutput());

           if (exitValue != 0) {
               throw new Error("clhsdb wasn't run successfully.");
           }

can be reduced to:

OutputAnalyzer output = new OutputAnalyzer(p);
output.shouldHaveExitValue(0);

There's probably no need to print getOutput() as if anything goes wrong
it will be printed by OutputAnalyzer anyway. But that's your choice.

It may also be advisable to ensure the process is destroyed if you get
an exception here:

          try {
              p.waitFor();
          } catch (InterruptedException ie) {
+            p.destroyForcibly();
              throw new Error("Problem awaiting the child process: " +
ie, ie);
          }

similar to how ProcessTools.executeProcess manages things.

Thanks,
David
-----

> Thank you,
> Jini.
>
> On 10/31/2017 12:32 PM, Jini George wrote:
>> Thank you for the quick review, David. My comments inline:
>>
>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>
>>>> Plus this assumes G1 is being used but Lingered App is started using
>>>> the test run vmOptions AFAICS so it would use whatever GC were
>>>> passed through, wouldn't it?
>>>
>>> Okay the above are just examples of "int constants" that will be
>>> printed when you dump all the "int constants". The G1 constant
>>> doesn't indicate (I presume) that G1 is the active GC.
>>
>> Yes, the int constants contain compile time values and there are
>> entries for all the GC types.
>>
>>> As I said to Sharath it would be good to validate the results of each
>>> command separately rather than collectively.
>>
>> Will do.
>>
>> Thanks!
>> - Jini.
>>
>>
>>
>>> Thanks,
>>> David
>>>
>>>> ---
>>>>
>>>> TestType.java
>>>>
>>>> The expected output is not quite so strange, but still could do with
>>>> some commentary.
>>>>
>>>>    90         Asserts.assertTrue(output.contains("type
>>>> G1CollectedHeap CollectedHeap"));
>>>>
>>>> Same comment about assuming G1.
>>>>
>>>> Thanks,
>>>> David
>>>> ------
>>>>
>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>> Hello,
>>>>>
>>>>> We have been working on writing sanity tests for the various jhsdb
>>>>> clhsdb commands of the SA to improve the robustness of the SA. As a
>>>>> part of this, here is a webrev for sanity tests for 3 of the clhsdb
>>>>> commands:
>>>>>
>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>
>>>>> I would like to request for reviews for this simple addition.
>>>>>
>>>>> Thank you,
>>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Jini George
Thank you very much, David. Here is the revised webrev:

http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/

Have reduced the exitValue checking fragment and included
p.destroyForcibly(). Also placed the creation of OutputAnalyzer before
waiting for the jhsdb process exit, to avoid jhsdb hangs due to the
output stream buffer not being consumed. (Thanks, Sharath!)

Thanks,
Jini.

On 11/6/2017 12:58 PM, David Holmes wrote:

> Hi Jini,
>
> On 3/11/2017 8:51 PM, Jini George wrote:
>> Here is the updated webrev:
>>
>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>>
>> I have made changes to validate the test results of each command
>> separately, done away with the asserts and have added some more comments.
>
> This looks much better to me - thanks. This fragment:
>
>            int exitValue = p.exitValue();
>            OutputAnalyzer output = new OutputAnalyzer(p);
>            System.out.println(output.getOutput());
>
>            if (exitValue != 0) {
>                throw new Error("clhsdb wasn't run successfully.");
>            }
>
> can be reduced to:
>
> OutputAnalyzer output = new OutputAnalyzer(p);
> output.shouldHaveExitValue(0);
>
> There's probably no need to print getOutput() as if anything goes wrong
> it will be printed by OutputAnalyzer anyway. But that's your choice.
>
> It may also be advisable to ensure the process is destroyed if you get
> an exception here:
>
>           try {
>               p.waitFor();
>           } catch (InterruptedException ie) {
> +            p.destroyForcibly();
>               throw new Error("Problem awaiting the child process: " +
> ie, ie);
>           }
>
> similar to how ProcessTools.executeProcess manages things.
>
> Thanks,
> David
> -----
>
>> Thank you,
>> Jini.
>>
>> On 10/31/2017 12:32 PM, Jini George wrote:
>>> Thank you for the quick review, David. My comments inline:
>>>
>>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>>
>>>>> Plus this assumes G1 is being used but Lingered App is started
>>>>> using the test run vmOptions AFAICS so it would use whatever GC
>>>>> were passed through, wouldn't it?
>>>>
>>>> Okay the above are just examples of "int constants" that will be
>>>> printed when you dump all the "int constants". The G1 constant
>>>> doesn't indicate (I presume) that G1 is the active GC.
>>>
>>> Yes, the int constants contain compile time values and there are
>>> entries for all the GC types.
>>>
>>>> As I said to Sharath it would be good to validate the results of
>>>> each command separately rather than collectively.
>>>
>>> Will do.
>>>
>>> Thanks!
>>> - Jini.
>>>
>>>
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> ---
>>>>>
>>>>> TestType.java
>>>>>
>>>>> The expected output is not quite so strange, but still could do
>>>>> with some commentary.
>>>>>
>>>>>    90         Asserts.assertTrue(output.contains("type
>>>>> G1CollectedHeap CollectedHeap"));
>>>>>
>>>>> Same comment about assuming G1.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> ------
>>>>>
>>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>>> Hello,
>>>>>>
>>>>>> We have been working on writing sanity tests for the various jhsdb
>>>>>> clhsdb commands of the SA to improve the robustness of the SA. As
>>>>>> a part of this, here is a webrev for sanity tests for 3 of the
>>>>>> clhsdb commands:
>>>>>>
>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>>
>>>>>> I would like to request for reviews for this simple addition.
>>>>>>
>>>>>> Thank you,
>>>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

RE: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Sharath Ballal
Hi Jini,

Code looks good.  Some nits.

TestUniverse.java:

These lines can be removed
   26 import java.io.File;
   33 import jdk.test.lib.process.ProcessTools;
   84         int exitValue = p.exitValue();

TestType.java:

These lines can be removed
   32 import jdk.test.lib.process.ProcessTools;
   86         int exitValue = p.exitValue();

You may want to add few more strings in the defaultOutputStrings.

TestIntConstant.java

These lines can be removed
   32 import jdk.test.lib.process.ProcessTools;
   89         int exitValue = p.exitValue();

You may want to add few more strings in the defaultOutputStrings.

Thanks,
Sharath (not a reviewer)


-----Original Message-----
From: Jini George
Sent: Tuesday, November 14, 2017 2:19 PM
To: David Holmes; [hidden email]
Subject: Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Thank you very much, David. Here is the revised webrev:

http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/

Have reduced the exitValue checking fragment and included p.destroyForcibly(). Also placed the creation of OutputAnalyzer before waiting for the jhsdb process exit, to avoid jhsdb hangs due to the output stream buffer not being consumed. (Thanks, Sharath!)

Thanks,
Jini.

On 11/6/2017 12:58 PM, David Holmes wrote:

> Hi Jini,
>
> On 3/11/2017 8:51 PM, Jini George wrote:
>> Here is the updated webrev:
>>
>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>>
>> I have made changes to validate the test results of each command
>> separately, done away with the asserts and have added some more comments.
>
> This looks much better to me - thanks. This fragment:
>
>            int exitValue = p.exitValue();
>            OutputAnalyzer output = new OutputAnalyzer(p);
>            System.out.println(output.getOutput());
>
>            if (exitValue != 0) {
>                throw new Error("clhsdb wasn't run successfully.");
>            }
>
> can be reduced to:
>
> OutputAnalyzer output = new OutputAnalyzer(p);
> output.shouldHaveExitValue(0);
>
> There's probably no need to print getOutput() as if anything goes
> wrong it will be printed by OutputAnalyzer anyway. But that's your choice.
>
> It may also be advisable to ensure the process is destroyed if you get
> an exception here:
>
>           try {
>               p.waitFor();
>           } catch (InterruptedException ie) {
> +            p.destroyForcibly();
>               throw new Error("Problem awaiting the child process: " +
> ie, ie);
>           }
>
> similar to how ProcessTools.executeProcess manages things.
>
> Thanks,
> David
> -----
>
>> Thank you,
>> Jini.
>>
>> On 10/31/2017 12:32 PM, Jini George wrote:
>>> Thank you for the quick review, David. My comments inline:
>>>
>>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>>
>>>>> Plus this assumes G1 is being used but Lingered App is started
>>>>> using the test run vmOptions AFAICS so it would use whatever GC
>>>>> were passed through, wouldn't it?
>>>>
>>>> Okay the above are just examples of "int constants" that will be
>>>> printed when you dump all the "int constants". The G1 constant
>>>> doesn't indicate (I presume) that G1 is the active GC.
>>>
>>> Yes, the int constants contain compile time values and there are
>>> entries for all the GC types.
>>>
>>>> As I said to Sharath it would be good to validate the results of
>>>> each command separately rather than collectively.
>>>
>>> Will do.
>>>
>>> Thanks!
>>> - Jini.
>>>
>>>
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> ---
>>>>>
>>>>> TestType.java
>>>>>
>>>>> The expected output is not quite so strange, but still could do
>>>>> with some commentary.
>>>>>
>>>>>    90         Asserts.assertTrue(output.contains("type
>>>>> G1CollectedHeap CollectedHeap"));
>>>>>
>>>>> Same comment about assuming G1.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> ------
>>>>>
>>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>>> Hello,
>>>>>>
>>>>>> We have been working on writing sanity tests for the various
>>>>>> jhsdb clhsdb commands of the SA to improve the robustness of the
>>>>>> SA. As a part of this, here is a webrev for sanity tests for 3 of
>>>>>> the clhsdb commands:
>>>>>>
>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>>
>>>>>> I would like to request for reviews for this simple addition.
>>>>>>
>>>>>> Thank you,
>>>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

David Holmes
Hi Jini,

In addition to Sharath's comments please add @bug and @summary items to
each test header.

Thanks,
David
-----

On 15/11/2017 2:31 AM, Sharath Ballal wrote:

> Hi Jini,
>
> Code looks good.  Some nits.
>
> TestUniverse.java:
>
> These lines can be removed
>     26 import java.io.File;
>     33 import jdk.test.lib.process.ProcessTools;
>     84         int exitValue = p.exitValue();
>
> TestType.java:
>
> These lines can be removed
>     32 import jdk.test.lib.process.ProcessTools;
>     86         int exitValue = p.exitValue();
>
> You may want to add few more strings in the defaultOutputStrings.
>
> TestIntConstant.java
>
> These lines can be removed
>     32 import jdk.test.lib.process.ProcessTools;
>     89         int exitValue = p.exitValue();
>
> You may want to add few more strings in the defaultOutputStrings.
>
> Thanks,
> Sharath (not a reviewer)
>
>
> -----Original Message-----
> From: Jini George
> Sent: Tuesday, November 14, 2017 2:19 PM
> To: David Holmes; [hidden email]
> Subject: Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type
>
> Thank you very much, David. Here is the revised webrev:
>
> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/
>
> Have reduced the exitValue checking fragment and included p.destroyForcibly(). Also placed the creation of OutputAnalyzer before waiting for the jhsdb process exit, to avoid jhsdb hangs due to the output stream buffer not being consumed. (Thanks, Sharath!)
>
> Thanks,
> Jini.
>
> On 11/6/2017 12:58 PM, David Holmes wrote:
>> Hi Jini,
>>
>> On 3/11/2017 8:51 PM, Jini George wrote:
>>> Here is the updated webrev:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>>>
>>> I have made changes to validate the test results of each command
>>> separately, done away with the asserts and have added some more comments.
>>
>> This looks much better to me - thanks. This fragment:
>>
>>             int exitValue = p.exitValue();
>>             OutputAnalyzer output = new OutputAnalyzer(p);
>>             System.out.println(output.getOutput());
>>
>>             if (exitValue != 0) {
>>                 throw new Error("clhsdb wasn't run successfully.");
>>             }
>>
>> can be reduced to:
>>
>> OutputAnalyzer output = new OutputAnalyzer(p);
>> output.shouldHaveExitValue(0);
>>
>> There's probably no need to print getOutput() as if anything goes
>> wrong it will be printed by OutputAnalyzer anyway. But that's your choice.
>>
>> It may also be advisable to ensure the process is destroyed if you get
>> an exception here:
>>
>>            try {
>>                p.waitFor();
>>            } catch (InterruptedException ie) {
>> +            p.destroyForcibly();
>>                throw new Error("Problem awaiting the child process: " +
>> ie, ie);
>>            }
>>
>> similar to how ProcessTools.executeProcess manages things.
>>
>> Thanks,
>> David
>> -----
>>
>>> Thank you,
>>> Jini.
>>>
>>> On 10/31/2017 12:32 PM, Jini George wrote:
>>>> Thank you for the quick review, David. My comments inline:
>>>>
>>>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>>>
>>>>>> Plus this assumes G1 is being used but Lingered App is started
>>>>>> using the test run vmOptions AFAICS so it would use whatever GC
>>>>>> were passed through, wouldn't it?
>>>>>
>>>>> Okay the above are just examples of "int constants" that will be
>>>>> printed when you dump all the "int constants". The G1 constant
>>>>> doesn't indicate (I presume) that G1 is the active GC.
>>>>
>>>> Yes, the int constants contain compile time values and there are
>>>> entries for all the GC types.
>>>>
>>>>> As I said to Sharath it would be good to validate the results of
>>>>> each command separately rather than collectively.
>>>>
>>>> Will do.
>>>>
>>>> Thanks!
>>>> - Jini.
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> ---
>>>>>>
>>>>>> TestType.java
>>>>>>
>>>>>> The expected output is not quite so strange, but still could do
>>>>>> with some commentary.
>>>>>>
>>>>>>     90         Asserts.assertTrue(output.contains("type
>>>>>> G1CollectedHeap CollectedHeap"));
>>>>>>
>>>>>> Same comment about assuming G1.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> ------
>>>>>>
>>>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> We have been working on writing sanity tests for the various
>>>>>>> jhsdb clhsdb commands of the SA to improve the robustness of the
>>>>>>> SA. As a part of this, here is a webrev for sanity tests for 3 of
>>>>>>> the clhsdb commands:
>>>>>>>
>>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>>>
>>>>>>> I would like to request for reviews for this simple addition.
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Jini George
Thank you, Sharath and David. The revised webrev after addressing the
comments are at:

http://cr.openjdk.java.net/~jgeorge/8190307/webrev.03/

Thanks,
Jini.

On 11/15/2017 7:00 AM, David Holmes wrote:

> Hi Jini,
>
> In addition to Sharath's comments please add @bug and @summary items to
> each test header.
>
> Thanks,
> David
> -----
>
> On 15/11/2017 2:31 AM, Sharath Ballal wrote:
>> Hi Jini,
>>
>> Code looks good.  Some nits.
>>
>> TestUniverse.java:
>>
>> These lines can be removed
>>     26 import java.io.File;
>>     33 import jdk.test.lib.process.ProcessTools;
>>     84         int exitValue = p.exitValue();
>>
>> TestType.java:
>>
>> These lines can be removed
>>     32 import jdk.test.lib.process.ProcessTools;
>>     86         int exitValue = p.exitValue();
>>
>> You may want to add few more strings in the defaultOutputStrings.
>>
>> TestIntConstant.java
>>
>> These lines can be removed
>>     32 import jdk.test.lib.process.ProcessTools;
>>     89         int exitValue = p.exitValue();
>>
>> You may want to add few more strings in the defaultOutputStrings.
>>
>> Thanks,
>> Sharath (not a reviewer)
>>
>>
>> -----Original Message-----
>> From: Jini George
>> Sent: Tuesday, November 14, 2017 2:19 PM
>> To: David Holmes; [hidden email]
>> Subject: Re: RFR: (small): JDK-8190307: SA: Sanity tests for the
>> clhsdb commands: universe, intconstant, type
>>
>> Thank you very much, David. Here is the revised webrev:
>>
>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/
>>
>> Have reduced the exitValue checking fragment and included
>> p.destroyForcibly(). Also placed the creation of OutputAnalyzer before
>> waiting for the jhsdb process exit, to avoid jhsdb hangs due to the
>> output stream buffer not being consumed. (Thanks, Sharath!)
>>
>> Thanks,
>> Jini.
>>
>> On 11/6/2017 12:58 PM, David Holmes wrote:
>>> Hi Jini,
>>>
>>> On 3/11/2017 8:51 PM, Jini George wrote:
>>>> Here is the updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>>>>
>>>> I have made changes to validate the test results of each command
>>>> separately, done away with the asserts and have added some more
>>>> comments.
>>>
>>> This looks much better to me - thanks. This fragment:
>>>
>>>             int exitValue = p.exitValue();
>>>             OutputAnalyzer output = new OutputAnalyzer(p);
>>>             System.out.println(output.getOutput());
>>>
>>>             if (exitValue != 0) {
>>>                 throw new Error("clhsdb wasn't run successfully.");
>>>             }
>>>
>>> can be reduced to:
>>>
>>> OutputAnalyzer output = new OutputAnalyzer(p);
>>> output.shouldHaveExitValue(0);
>>>
>>> There's probably no need to print getOutput() as if anything goes
>>> wrong it will be printed by OutputAnalyzer anyway. But that's your
>>> choice.
>>>
>>> It may also be advisable to ensure the process is destroyed if you get
>>> an exception here:
>>>
>>>            try {
>>>                p.waitFor();
>>>            } catch (InterruptedException ie) {
>>> +            p.destroyForcibly();
>>>                throw new Error("Problem awaiting the child process: " +
>>> ie, ie);
>>>            }
>>>
>>> similar to how ProcessTools.executeProcess manages things.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Thank you,
>>>> Jini.
>>>>
>>>> On 10/31/2017 12:32 PM, Jini George wrote:
>>>>> Thank you for the quick review, David. My comments inline:
>>>>>
>>>>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>>>>
>>>>>>> Plus this assumes G1 is being used but Lingered App is started
>>>>>>> using the test run vmOptions AFAICS so it would use whatever GC
>>>>>>> were passed through, wouldn't it?
>>>>>>
>>>>>> Okay the above are just examples of "int constants" that will be
>>>>>> printed when you dump all the "int constants". The G1 constant
>>>>>> doesn't indicate (I presume) that G1 is the active GC.
>>>>>
>>>>> Yes, the int constants contain compile time values and there are
>>>>> entries for all the GC types.
>>>>>
>>>>>> As I said to Sharath it would be good to validate the results of
>>>>>> each command separately rather than collectively.
>>>>>
>>>>> Will do.
>>>>>
>>>>> Thanks!
>>>>> - Jini.
>>>>>
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> TestType.java
>>>>>>>
>>>>>>> The expected output is not quite so strange, but still could do
>>>>>>> with some commentary.
>>>>>>>
>>>>>>>     90         Asserts.assertTrue(output.contains("type
>>>>>>> G1CollectedHeap CollectedHeap"));
>>>>>>>
>>>>>>> Same comment about assuming G1.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> ------
>>>>>>>
>>>>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> We have been working on writing sanity tests for the various
>>>>>>>> jhsdb clhsdb commands of the SA to improve the robustness of the
>>>>>>>> SA. As a part of this, here is a webrev for sanity tests for 3 of
>>>>>>>> the clhsdb commands:
>>>>>>>>
>>>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>>>>
>>>>>>>> I would like to request for reviews for this simple addition.
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

David Holmes
On 15/11/2017 3:27 PM, Jini George wrote:
> Thank you, Sharath and David. The revised webrev after addressing the
> comments are at:
>
> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.03/

Please put all the informational tags first:

@test
@bug
@summary

You didn't delete the

    int exitValue = p.exitValue();

lines.

No need to see an updated webrev with these fixes.

Thanks,
David


> Thanks,
> Jini.
>
> On 11/15/2017 7:00 AM, David Holmes wrote:
>> Hi Jini,
>>
>> In addition to Sharath's comments please add @bug and @summary items
>> to each test header.
>>
>> Thanks,
>> David
>> -----
>>
>> On 15/11/2017 2:31 AM, Sharath Ballal wrote:
>>> Hi Jini,
>>>
>>> Code looks good.  Some nits.
>>>
>>> TestUniverse.java:
>>>
>>> These lines can be removed
>>>     26 import java.io.File;
>>>     33 import jdk.test.lib.process.ProcessTools;
>>>     84         int exitValue = p.exitValue();
>>>
>>> TestType.java:
>>>
>>> These lines can be removed
>>>     32 import jdk.test.lib.process.ProcessTools;
>>>     86         int exitValue = p.exitValue();
>>>
>>> You may want to add few more strings in the defaultOutputStrings.
>>>
>>> TestIntConstant.java
>>>
>>> These lines can be removed
>>>     32 import jdk.test.lib.process.ProcessTools;
>>>     89         int exitValue = p.exitValue();
>>>
>>> You may want to add few more strings in the defaultOutputStrings.
>>>
>>> Thanks,
>>> Sharath (not a reviewer)
>>>
>>>
>>> -----Original Message-----
>>> From: Jini George
>>> Sent: Tuesday, November 14, 2017 2:19 PM
>>> To: David Holmes; [hidden email]
>>> Subject: Re: RFR: (small): JDK-8190307: SA: Sanity tests for the
>>> clhsdb commands: universe, intconstant, type
>>>
>>> Thank you very much, David. Here is the revised webrev:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/
>>>
>>> Have reduced the exitValue checking fragment and included
>>> p.destroyForcibly(). Also placed the creation of OutputAnalyzer
>>> before waiting for the jhsdb process exit, to avoid jhsdb hangs due
>>> to the output stream buffer not being consumed. (Thanks, Sharath!)
>>>
>>> Thanks,
>>> Jini.
>>>
>>> On 11/6/2017 12:58 PM, David Holmes wrote:
>>>> Hi Jini,
>>>>
>>>> On 3/11/2017 8:51 PM, Jini George wrote:
>>>>> Here is the updated webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>>>>>
>>>>> I have made changes to validate the test results of each command
>>>>> separately, done away with the asserts and have added some more
>>>>> comments.
>>>>
>>>> This looks much better to me - thanks. This fragment:
>>>>
>>>>             int exitValue = p.exitValue();
>>>>             OutputAnalyzer output = new OutputAnalyzer(p);
>>>>             System.out.println(output.getOutput());
>>>>
>>>>             if (exitValue != 0) {
>>>>                 throw new Error("clhsdb wasn't run successfully.");
>>>>             }
>>>>
>>>> can be reduced to:
>>>>
>>>> OutputAnalyzer output = new OutputAnalyzer(p);
>>>> output.shouldHaveExitValue(0);
>>>>
>>>> There's probably no need to print getOutput() as if anything goes
>>>> wrong it will be printed by OutputAnalyzer anyway. But that's your
>>>> choice.
>>>>
>>>> It may also be advisable to ensure the process is destroyed if you get
>>>> an exception here:
>>>>
>>>>            try {
>>>>                p.waitFor();
>>>>            } catch (InterruptedException ie) {
>>>> +            p.destroyForcibly();
>>>>                throw new Error("Problem awaiting the child process: " +
>>>> ie, ie);
>>>>            }
>>>>
>>>> similar to how ProcessTools.executeProcess manages things.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Thank you,
>>>>> Jini.
>>>>>
>>>>> On 10/31/2017 12:32 PM, Jini George wrote:
>>>>>> Thank you for the quick review, David. My comments inline:
>>>>>>
>>>>>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>>>>>
>>>>>>>> Plus this assumes G1 is being used but Lingered App is started
>>>>>>>> using the test run vmOptions AFAICS so it would use whatever GC
>>>>>>>> were passed through, wouldn't it?
>>>>>>>
>>>>>>> Okay the above are just examples of "int constants" that will be
>>>>>>> printed when you dump all the "int constants". The G1 constant
>>>>>>> doesn't indicate (I presume) that G1 is the active GC.
>>>>>>
>>>>>> Yes, the int constants contain compile time values and there are
>>>>>> entries for all the GC types.
>>>>>>
>>>>>>> As I said to Sharath it would be good to validate the results of
>>>>>>> each command separately rather than collectively.
>>>>>>
>>>>>> Will do.
>>>>>>
>>>>>> Thanks!
>>>>>> - Jini.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> TestType.java
>>>>>>>>
>>>>>>>> The expected output is not quite so strange, but still could do
>>>>>>>> with some commentary.
>>>>>>>>
>>>>>>>>     90         Asserts.assertTrue(output.contains("type
>>>>>>>> G1CollectedHeap CollectedHeap"));
>>>>>>>>
>>>>>>>> Same comment about assuming G1.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> ------
>>>>>>>>
>>>>>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> We have been working on writing sanity tests for the various
>>>>>>>>> jhsdb clhsdb commands of the SA to improve the robustness of the
>>>>>>>>> SA. As a part of this, here is a webrev for sanity tests for 3 of
>>>>>>>>> the clhsdb commands:
>>>>>>>>>
>>>>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>>>>>
>>>>>>>>> I would like to request for reviews for this simple addition.
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Jini.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: (small): JDK-8190307: SA: Sanity tests for the clhsdb commands: universe, intconstant, type

Jini George
Thank you very much, David.

- Jini.

On 11/16/2017 8:16 AM, David Holmes wrote:

> On 15/11/2017 3:27 PM, Jini George wrote:
>> Thank you, Sharath and David. The revised webrev after addressing the
>> comments are at:
>>
>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.03/
>
> Please put all the informational tags first:
>
> @test
> @bug
> @summary
>
> You didn't delete the
>
>     int exitValue = p.exitValue();
>
> lines.
>
> No need to see an updated webrev with these fixes.
>
> Thanks,
> David
>
>
>> Thanks,
>> Jini.
>>
>> On 11/15/2017 7:00 AM, David Holmes wrote:
>>> Hi Jini,
>>>
>>> In addition to Sharath's comments please add @bug and @summary items
>>> to each test header.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>> On 15/11/2017 2:31 AM, Sharath Ballal wrote:
>>>> Hi Jini,
>>>>
>>>> Code looks good.  Some nits.
>>>>
>>>> TestUniverse.java:
>>>>
>>>> These lines can be removed
>>>>     26 import java.io.File;
>>>>     33 import jdk.test.lib.process.ProcessTools;
>>>>     84         int exitValue = p.exitValue();
>>>>
>>>> TestType.java:
>>>>
>>>> These lines can be removed
>>>>     32 import jdk.test.lib.process.ProcessTools;
>>>>     86         int exitValue = p.exitValue();
>>>>
>>>> You may want to add few more strings in the defaultOutputStrings.
>>>>
>>>> TestIntConstant.java
>>>>
>>>> These lines can be removed
>>>>     32 import jdk.test.lib.process.ProcessTools;
>>>>     89         int exitValue = p.exitValue();
>>>>
>>>> You may want to add few more strings in the defaultOutputStrings.
>>>>
>>>> Thanks,
>>>> Sharath (not a reviewer)
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Jini George
>>>> Sent: Tuesday, November 14, 2017 2:19 PM
>>>> To: David Holmes; [hidden email]
>>>> Subject: Re: RFR: (small): JDK-8190307: SA: Sanity tests for the
>>>> clhsdb commands: universe, intconstant, type
>>>>
>>>> Thank you very much, David. Here is the revised webrev:
>>>>
>>>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.02/
>>>>
>>>> Have reduced the exitValue checking fragment and included
>>>> p.destroyForcibly(). Also placed the creation of OutputAnalyzer
>>>> before waiting for the jhsdb process exit, to avoid jhsdb hangs due
>>>> to the output stream buffer not being consumed. (Thanks, Sharath!)
>>>>
>>>> Thanks,
>>>> Jini.
>>>>
>>>> On 11/6/2017 12:58 PM, David Holmes wrote:
>>>>> Hi Jini,
>>>>>
>>>>> On 3/11/2017 8:51 PM, Jini George wrote:
>>>>>> Here is the updated webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jgeorge/8190307/webrev.01/
>>>>>>
>>>>>> I have made changes to validate the test results of each command
>>>>>> separately, done away with the asserts and have added some more
>>>>>> comments.
>>>>>
>>>>> This looks much better to me - thanks. This fragment:
>>>>>
>>>>>             int exitValue = p.exitValue();
>>>>>             OutputAnalyzer output = new OutputAnalyzer(p);
>>>>>             System.out.println(output.getOutput());
>>>>>
>>>>>             if (exitValue != 0) {
>>>>>                 throw new Error("clhsdb wasn't run successfully.");
>>>>>             }
>>>>>
>>>>> can be reduced to:
>>>>>
>>>>> OutputAnalyzer output = new OutputAnalyzer(p);
>>>>> output.shouldHaveExitValue(0);
>>>>>
>>>>> There's probably no need to print getOutput() as if anything goes
>>>>> wrong it will be printed by OutputAnalyzer anyway. But that's your
>>>>> choice.
>>>>>
>>>>> It may also be advisable to ensure the process is destroyed if you get
>>>>> an exception here:
>>>>>
>>>>>            try {
>>>>>                p.waitFor();
>>>>>            } catch (InterruptedException ie) {
>>>>> +            p.destroyForcibly();
>>>>>                throw new Error("Problem awaiting the child process:
>>>>> " +
>>>>> ie, ie);
>>>>>            }
>>>>>
>>>>> similar to how ProcessTools.executeProcess manages things.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thank you,
>>>>>> Jini.
>>>>>>
>>>>>> On 10/31/2017 12:32 PM, Jini George wrote:
>>>>>>> Thank you for the quick review, David. My comments inline:
>>>>>>>
>>>>>>> On 10/30/2017 11:18 AM, David Holmes wrote:
>>>>>>>
>>>>>>>>> Plus this assumes G1 is being used but Lingered App is started
>>>>>>>>> using the test run vmOptions AFAICS so it would use whatever GC
>>>>>>>>> were passed through, wouldn't it?
>>>>>>>>
>>>>>>>> Okay the above are just examples of "int constants" that will be
>>>>>>>> printed when you dump all the "int constants". The G1 constant
>>>>>>>> doesn't indicate (I presume) that G1 is the active GC.
>>>>>>>
>>>>>>> Yes, the int constants contain compile time values and there are
>>>>>>> entries for all the GC types.
>>>>>>>
>>>>>>>> As I said to Sharath it would be good to validate the results of
>>>>>>>> each command separately rather than collectively.
>>>>>>>
>>>>>>> Will do.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> - Jini.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> TestType.java
>>>>>>>>>
>>>>>>>>> The expected output is not quite so strange, but still could do
>>>>>>>>> with some commentary.
>>>>>>>>>
>>>>>>>>>     90         Asserts.assertTrue(output.contains("type
>>>>>>>>> G1CollectedHeap CollectedHeap"));
>>>>>>>>>
>>>>>>>>> Same comment about assuming G1.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> ------
>>>>>>>>>
>>>>>>>>> On 30/10/2017 1:44 PM, Jini George wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> We have been working on writing sanity tests for the various
>>>>>>>>>> jhsdb clhsdb commands of the SA to improve the robustness of the
>>>>>>>>>> SA. As a part of this, here is a webrev for sanity tests for 3 of
>>>>>>>>>> the clhsdb commands:
>>>>>>>>>>
>>>>>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8190307
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8190307/webrev.00/
>>>>>>>>>>
>>>>>>>>>> I would like to request for reviews for this simple addition.
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Jini.