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; serviceability-dev at openjdk.java.net
>>> 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.