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