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