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

Leonid Mesnik leonid.mesnik at oracle.com
Mon Jun 26 20:11:01 UTC 2017


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



More information about the hotspot-dev mailing list