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

[Igor Ignatyev]

> On Jun 26, 2017, at 2:54 PM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> 
> Thank you Igor and Leonid,
> 
> I fixed mentioned typos and unnecessary return (see http://cr.openjdk.java.net/~aharlap/8178507/webrev.02/ <http://cr.openjdk.java.net/~aharlap/8178507/webrev.02/>)
> 
perfect.
> Do I need more reviews?
> 
no, you can go ahead and integrate it.

-- Igor
> 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 <https://bugs.openjdk.java.net/browse/JDK-8178507>> - co-locate nsk.regression.gc tests
>>>>>> 
>>>>>> JDK-8178507 <https://bugs.openjdk.java.net/browse/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 <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
>>>>>> 
>>>>>> 
>>>> 
>>> 
>> 
>