RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable

[Igor Ignatyev]

Andrey,

a few new comments:

1. 'TestDynamicIntVMOption' isn't specific for gc testing, so please 
move it to testlibrary

2. I'd like to have 'check*' methods in a separate auxiliary class in 
testlibrary. 'VMOptionValueChecker' as you have it before was good place.

3. I really really don't like spaces in brackets:
>  public TestDynamicIntVMOption( String name ) {
...
> checkInvalidValue( "-10" );
I don't think that it's fitted w/ our code-style.

4. please move test to some subdirectory in test/gc, e.g. 
test/gc/memory_pressure/ as it was before. or test/gc/memory_pressure/flags/

5. and the old one:
>>> 3. You use management packages, but not all JDK/JRE bundles have it, so
>>> you have to add your tests in some group (needs_compact3, I think). LevP
>>> should have more information on it.
LevP has confirmed that it's 'needs_compact3' group.
hint: test/TEST.groups -- the file w/ groups

Thanks
Igor

On 03/07/2014 04:35 PM, Jesper Wilhelmsson wrote:
> The webrev is uploaded here:
> http://cr.openjdk.java.net/~jwilhelm/7607562/
> /Jesper
>
> Andrey Zakharov skrev 7/3/14 11:23:
>> Hi
>> Thanks for comments.
>> I have completely rework this tests according to comments.
>> Also we have found that it will be easier to follow standard work-flow of
>> submitting patches to branch 9 and then back-porting it to 8 and 7.
>> So, here is webrev against http://hg.openjdk.java.net/jdk9/hs-gc/hotspot
>> I hope, Jesper will help me to upload it right place.
>>
>> Thanks.
>>
>>
>> On 05.03.2014 23:35, Lev wrote:
>>> Igor, you are right. You could check it in proposal:
>>> http://openjdk.java.net/jeps/161
>>>
>>> Best regards,
>>> Lev
>>>
>>>
>>> -------- Original message --------
>>> From: Igor Ignatyev
>>> Date:05/03/2014 13:12 (GMT+04:00)
>>> To: Andrey Zakharov ,Lev Priima
>>> Cc: hotspot-gc-dev at openjdk.java.net
>>> Subject: Re: RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio
>>> flags
>>> manageable
>>>
>>> Hi Andrey,
>>>
>>> 0. In all your tests you have:
>>> >   27  * @key gc
>>> it's redundant, since the tests are already in gc directory. please
>>> remove it.
>>> >   28  * @bug JDK-8028391
>>> according to convention, you have to use '8028391' instead of
>>> 'JDK-8028391'.
>>> $ grep -R "@bug" ./hotspot/test/ | wc -l
>>> 434
>>> $ grep -R "@bug" ./hotspot/test/ | grep -c JDK
>>> 0
>>>
>>> 1. VMOptionValueChecker has checkInvalidValue and checkValid, please
>>> normalize them:
>>>    checkInvalidValue/checkValidValuefO
>>>    or
>>>    checkInvalid/checkValid
>>>
>>> 2. I don't like that 'VMOptionValueChecker.getIntWritableValue' method
>>> do two things, could you please replace it w/ two methods:
>>> VMOptionValueChecker.getIntValue(name)
>>> VMOptionValueChecker.checkIsWriteable(name)
>>>
>>> 3. You use management packages, but not all JDK/JRE bundles have it, so
>>> you have to add your tests in some group (needs_compact3, I think). LevP
>>> should have more information on it.
>>>
>>> Lev, could you please help us here? com.sun.management.* and
>>> java.lang.management.* are available only in compact3, aren't they?
>>>
>>> 4. your tests look similar, can you create a super class for both tests?
>>> we will be able to reuse it in other tests for writable flags
>>>
>>> > class TestDynIntVMFlags  {
>>> > String name;
>>> > int originalValue;
>>> > TestDynIntVMFlags(name) {
>>> >   name = name;
>>> >   int originalValue = getIntValue(name)
>>> >   println(name + " = " + originalValue);
>>> > }
>>> >
>>> > void test() {
>>> >       checkIsWriteable(name);
>>> >       checkInvalidValue(name, "" + Integer.MIN_VALUE);
>>> >       checkInvalidValue(name, "" + Integer.MAX_VALUE);
>>> >       checkInvalidValue(name, "-10");
>>> >       checkInvalidValue(name, "190");
>>> >       checkValid(name, "0");
>>> >       extraTest();
>>> > }
>>> > void extraTest() { }
>>> > }
>>> > //// TestDynMinHeapFreeRatio.java
>>> > TestDynMinHeapFreeRatio extends TestDynIntVMFlags {
>>> > private TestDynMinHeapFreeRatio() { super("MinHeapFreeRatio"); }
>>> >   String MaxFreeRatioFlagName = "MaxHeapFreeRatio";
>>> >   void main() {
>>> >     new TestDynMinHeapFreeRatio().test();
>>> >   }
>>> >   void extraTest() {
>>> >     int maxHeapFreeValue = getIntValue(MaxFreeRatioFlagName);
>>> >     checkInvalidValue(name, "" + (maxHeapFreeValue + 1));
>>> >     checkValid(name, "" + (maxHeapFreeValue));
>>> >   }
>>> > }//// TestDynMaxHeapFreeRatio.java
>>> > TestDynMaxHeapFreeRatio extends TestDynIntVMFlags {
>>> > TestDynMaxHeapFreeRatio() { super("MaxHeapFreeRatio"); }
>>> >   String MaxFreeRatioFlagName = "MinHeapFreeRatio";
>>> >   void main() {
>>> >     new TestDynMaxHeapFreeRatio().test();
>>> >   }
>>> >   void extraTest() {
>>> >     int minHeapFreeValue = getIntValue(MinFreeRatioFlagName);
>>> >     checkInvalidValue(name, "" + (minHeapFreeValue - 1));
>>> >     checkValid(name, "" + (minHeapFreeValue));
>>> >   }
>>> > }
>>>
>>> Thanks,
>>> Igor
>>>
>>> On 02/20/2014 10:09 PM, Thomas Schatzl wrote:
>>> > Hi,
>>> >
>>> > On Thu, 2014-02-20 at 15:15 +0400, Andrey Zakharov wrote:
>>> >> Any comments are welcome!
>>> >> Thanks.
>>> >>
>>> >> On 14.02.2014 21:13, Jesper Wilhelmsson wrote:
>>> >>> Looks good.
>>> >>> /Jesper
>>> >>>
>>> >>> Andrey Zakharov skrev 14/2/14 16:33:
>>> >>>> Simple tests to check that MaxHeapFreeRatio and MinHeapFreeRatio
>>> >>>> flags are
>>> >>>> writable and managable in proper ways.
>>> >>>> webrev:
>>> http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.test.02/
>>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8028720
>>> >>>>
>>> >>>> Thanks.
>>> >>
>>> >
>>> >    some typos: "managable" -> "manageable"
>>> >
>>> > Otherwise looks good.
>>> >
>>> > Thomas
>>> >
>>> >
>>