Request for review for JDK-8178507 - co-locate nsk.regression.gc tests

[Alexander Harlap]

Thank you Igor and Leonid,

I fixed mentioned typos and unnecessary return (see 
http://cr.openjdk.java.net/~aharlap/8178507/webrev.02/)

Do I need more reviews?

Alex


On 6/26/2017 4:32 PM, Igor Ignatyev wrote:
> Hi Alexander,
>
> besides the small nits which Leonid mentioned, there is one in 
> http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html: 
> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html:> 
>
>>    28  * @summary Test verifies only that VM doesn’t crash but throw expected Error.
> I guess "doesn’t" is 'doesn't' w/ a fancy apostrophe. otherwise 
> looks good to me, Reviewed.
>
> -- Igor
>
>> On Jun 26, 2017, at 1:11 PM, Leonid Mesnik <leonid.mesnik at oracle.com 
>> <mailto:leonid.mesnik at oracle.com>> wrote:
>>
>> Hi
>>
>> New changes looks good for me. Please get review from Reviewer.
>>
>> The only 2 small nits which don’t require separate review from me:
>>
>> http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html 
>> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html> 
>> <http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html 
>> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html>>
>> typo in
>> 37 System.out.println("Hellow world!");
>>
>> http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html 
>> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html> 
>> <http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html 
>> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html>>
>> return is not needed in
>> 58 return;
>>
>> Thanks
>> Leonid
>>> On Jun 26, 2017, at 1:04 PM, Alexander Harlap 
>>> <alexander.harlap at oracle.com <mailto:alexander.harlap at oracle.com>> 
>>> wrote:
>>>
>>> Hi Leonid,
>>>
>>> I accommodated your suggestions.
>>>
>>> New version of changeset located at 
>>> http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/>
>>>
>>>
>>> Alex
>>>
>>>
>>> On 6/23/2017 6:18 PM, Leonid Mesnik wrote:
>>>> Hi
>>>>
>>>> Basically changes looks good. Below are some comments:
>>>>
>>>>> On Jun 22, 2017, at 9:16 AM, Alexander Harlap 
>>>>> <alexander.harlap at oracle.com <mailto:alexander.harlap at oracle.com>> 
>>>>> wrote:
>>>>>
>>>>> Please review change for JDK-8178507 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8178507> - co-locate 
>>>>> nsk.regression.gc tests
>>>>>
>>>>> JDK-8178507 <https://bugs.openjdk.java.net/browse/JDK-8178507> is 
>>>>> last remaining sub-task ofJDK-8178482 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8178482> - Co-locate 
>>>>> remaining GC tests
>>>>>
>>>>>
>>>>> Proposed change located at 
>>>>> http://cr.openjdk.java.net/~aharlap/8178507/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.00/>
>>>>>
>>>>> Co-located and converted to JTREG tests are:
>>>>>
>>>>>   nsk/regression/b4187687 => hotspot/test/gc/TestFullGCALot.java
>>>> The out variable is no used and return code is not checked in 
>>>> method “run”. Wouldn't it simpler just to move println into main 
>>>> and remove method ‘run’ completely?
>>>>>   nsk/regression/b4396719 => hotspot/test/gc/TestStackOverflow.java
>>>> The method ‘run’ always returns 0. It would be better to make it 
>>>> void or just remove it. Test never throws any exception. So it make 
>>>> a sense to write in comments that test verifies only that VM 
>>>> doesn’t crash but throw expected Error.
>>>>
>>>>>   nsk/regression/b4668531 => 
>>>>> hotspot/test/gc/TestMemoryInitialization.java
>>>> The variable buffer is “read-only”. It make a sense to make 
>>>> variable ‘buffer' public static member of class 
>>>> TestMemoryInitialization. So compiler could not optimize it usage 
>>>> during any optimization like escape analysis.
>>>>>   nsk/regression/b6186200 => 
>>>>> hotspot/test/gc/cslocker/TestCSLocker.java
>>>>>
>>>> Port looks good. It seems that test doesn’t verify that lock really 
>>>> happened. Could be this improved as a part of this fix or by filing 
>>>> separate RFE?
>>>>
>>>> Leonid
>>>>> Thank you,
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>
>>
>