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

[Igor Ignatyev]

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: 
>   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> 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/~aharlap/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/~aharlap/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> wrote:
>> 
>> Hi Leonid,
>> 
>> I accommodated your suggestions.
>> 
>> New version of changeset located at http://cr.openjdk.java.net/~aharlap/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> 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/
>>>> 
>>>> 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
>>>> 
>>>> 
>> 
>