RFR 8059557: Test set for "Validate JVM Command-Line Flag Arguments"

Thu May 28 12:03:21 UTC 2015

Hi Gerard,

Thank you for reviewing the code! Please see my comment inline.

On 27.05.2015 21:19, Gerard Ziemski wrote:
> Nice code Dmitry!
>
> I have a bit of feedback, but I might be off on some of them, and some reflect my personal preference:
>
>
> 1.http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.02/test/runtime/CommandLine/OptionsValidation/TestJcmdOutput.java.html
>
> 1 a.
> Line 27, from: "value which not allowd by constraint. Also check that”
>             to: "value which is not allowed by constraint. Also check that”
Fixed!

>
> 1 b.
> Line 28, from: "jcmd not print error message to the target process output.”
>             to: "jcmd does not print an error message to the target process output."
Fixed!

> 1 c.
> Line 49, from: "System.out.println("Verify jcmd error message and that jcmd not write errors to the target process output");”
>             to: "System.out.println(“Verify jcmd error message and that jcmd does not write errors to the target process output");”
Fixed!

>
>
> 1 d.
> Line 67: "67 Asserts.assertGT(minHeapFreeRatio, 0, "MinHeapFreeRatio must be greater than 0”);”
>
> Shouldn’t that be "Asserts.assertGTOE (>=)", not “Asserts.assertGT”?
Actually, for this test MinHeapFreeRatio should be strictly greater than 
0, because otherwise I will be unable to violate constraint, i.e. when 
MinHeapFreeRatio/MaxHeapFreeRatio have valid values(in range) and 
MaxHeapFreeRatio < MinHeapFreeRatio.

> 1 e.
> Line 68: “Asserts.assertLT(maxHeapFreeRatio, 100, "MaxHeapFreeRatio must be less than 100");”
>
> Shouldn’t that be "Asserts.assertLTOE (<=)", not “ Asserts.assertLT”?
The same as above. MaxHeapFreeRatio must be strictly less than 100 to 
violate constraint.

>
> 2.http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.02/test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/DoubleJVMOption.java.html
>
> 2 a.
> Line 31: "private final double MAX_DOUBLE = 18446744073709551616.000;”
>
> Just curious: why can’t we use Double.MAX_VALUE? where does your value come from?
Fixed and removed mention of this old value! Actually, I reworked code 
for double option.

>
>
> 2 b.
> Line 126, from : "if (min == Double.MIN_VALUE && max == MAX_DOUBLE) {“
>              to : "if ((Double.equal(min, Double.MIN_VALUE) == 0) && (Double.equal(max, MAX_DOUBLE) == 0)) {“
Fixed! Use Double.compare for comparing doubles(I think you mean that 
method).

>
> 2 c.
> Line 127: "validValues.add(String.format("%f", -1.5));”
> Line 129: validValues.add(String.format("%f", 0.85));
>
> Just curious: why did you pick -1.5 and 0.85? I personally would have selected min/2.0 and max/2.0 and maybe -1.0 and 1.0
I pick values with fraction part. Actually, I reworked this part of code 
and slightly changed these values(also I define then via constants now).

> 2 d.
> We need to change from:
>
>   145         if (min != Double.MIN_VALUE) {
>   146             invalidValues.add(String.format("%f", min - 0.01));
>   147         }
>   148
>   149         if (max != MAX_DOUBLE) {
>   150             invalidValues.add(String.format("%f", max + 0.01));
>   151         }
>
>
> to:
>
>   145         if ((Double.equal(min, Double.MIN_VALUE) != 0) && (Double.isNaN(min-0.01) == false)) {
>   146             invalidValues.add(String.format("%f", min - 0.01));
>   147         }
>   148
>   149         if ((Double.equal(max, MAX_DOUBLE) != 0) && (Double.isNaN(max+0.01) == false)) {
>   150             invalidValues.add(String.format("%f", max + 0.01));
>   151         }
Fixed! Also, I changed max + 0.01 to max * 1.001 (for positive max), 
because max+0.01 is not work for huge max values.

>
>
> 3.http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.02/test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOption.java.html
>
>
> 3 a.
>
> Line 331, from : * @param valid Indicates, should JVM failed or not
>              to : * @param valid Indicates whether the JVM should fail or not
Fixed!

>
> 3 b.
>
> Line 361, from : } else if (returnCode == 1 && out.getOutput().isEmpty()) {
>              to : } else if ((returnCode == 1) && (out.getOutput().isEmpty() == true)) {
Fixed!

>
>
>
> 4.http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.02/test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java.html
>
> 4 a.
>
>   125     static private Map<String, JVMOption> getJVMOptions(Reader inputReader,
>   126             boolean withRanges, Predicate<String> acceptOrigin) throws IOException {
>
> I see that the implementation uses hardcoded positions, ex. "line.substring(0, 9)” Would it be hard to make the code character position independent by considering an implementation based on a “grep” or some other pattern recognition? If we ever tweak the output in ranges printout, even ever so slightly, this code will need to be updated.
I rewrite this code and not use StringTokenizer to parse line with option.

Regards,
Dmitry
>
>
> cheers
>
>
>> On May 26, 2015, at 1:24 PM, Gerard Ziemski<gerard.ziemski at oracle.com>  wrote:
>>
>>
>>
>>
>> -------- Forwarded Message --------
>> Subject:	Re: RFR 8059557: Test set for "Validate JVM Command-Line Flag Arguments"
>> Date:	Thu, 21 May 2015 23:57:11 +0300
>> From:	Dmitry Dmitriev<dmitry.dmitriev at oracle.com>
>> Organization:	Oracle Corporation
>> To:	hotspot-dev at openjdk.java.net, Gerard Ziemski<gerard.ziemski at oracle.com>
>>
>> Hello all,
>>
>> Recently I correct several typos, so here a new webrev for tests:http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.02/
>>
>> Thanks,
>> Dmitry
>>
>> On 18.05.2015 18:48, Dmitry Dmitriev wrote:
>>> Hello all,
>>>
>>> Please review test set for verifying functionality implemented by JEP 245 "Validate JVM Command-Line Flag Arguments"(JDK-8059557). Review request for this JEP can be found there:http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-May/018539.html
>>>
>>> I create 3 tests for verifying options with ranges. The tests mostly rely on common/optionsvalidation/JVMOptionsUtils.java. Class in this file contains functions to get options with ranges as list(by parsing new option "-XX:+PrintFlagsRanges" output), run command line test for list of options and other. The actual test code contained in common/optionsvalidation/JVMOption.java file - testCommandLine(), testDynamic(), testJcmd() and testAttach() methods. common/optionsvalidation/IntJVMOption.java and common/optionsvalidation/DoubleJVMOption.java source files contain classes derived from JVMOption class for integer and double JVM options correspondingly.
>>>
>>> Here are description of the tests:
>>> 1) hotspot/test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>
>>> This test get all options with ranges by parsing output of new option "-XX:+PrintFlagsRanges" and verify these options by starting Java and passing options in command line with valid and invalid values. Currently it verifies about 106 options which have ranges.
>>> Invalid values are values which out-of-range. In test used values "min-1" and "max+1".In this case Java should always exit with code 1 and print error message about out-of-range value(with one exception, if option is unsigned and passing negative value, then out-of-range error message is not printed because error occurred earlier).
>>> Valid values are values in range, e.g. min&max and also several additional values. In this case Java should successfully exit(exit code 0) or exit with error code 1 for other reasons(low memory with certain option value etc.). In any case for values in range Java should not print messages about out of range value.
>>> In any case Java should not crash.
>>> This test excluded from JPRT because it takes long time to execute and also fails - some options with value in valid range cause Java to crash(bugs are submitted).
>>>
>>> 2) hotspot/test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>
>>> This test get all writeable options with ranges by parsing output of new option "-XX:+PrintFlagsRanges" and verify these options by dynamically changing it's values to the valid and invalid values. Used 3 methods for that: DynamicVMOption isValidValue and isInvalidValue methods, Jcmd and by attach method. Currently 3 writeable options with ranges are verified by this test.
>>> This test pass in JPRT.
>>>
>>> 3) hotspot/test/runtime/CommandLine/OptionsValidation/TestJcmdOutput.java
>>>
>>> This test verified output of Jcmd when out-of-range value is set to the writeable option or value violates option constraint. Also this test verify that jcmd not write error message to the target process.
>>>   This test pass in JPRT.
>>>
>>>
>>> I am not write special tests for constraints for this JEP because there are exist test for that(e.g. test/runtime/CompressedOops/ObjectAlignment.java for ObjectAlignmentInBytes or hotspot/test/gc/arguments/TestHeapFreeRatio.java for MinHeapFreeRatio/MaxHeapFreeRatio).
>>>
>>> Webrev:http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.00/
>>>
>>> JEP:https://bugs.openjdk.java.net/browse/JDK-8059557  
>>>
>>> Thanks,
>>> Dmitry
>>>