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

Wed May 27 18:19:58 UTC 2015

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”

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

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");”

1 d.
Line 67: "67 Asserts.assertGT(minHeapFreeRatio, 0, "MinHeapFreeRatio must be greater than 0”);”

Shouldn’t that be "Asserts.assertGTOE (>=)", not “Asserts.assertGT”?

1 e.
Line 68: “Asserts.assertLT(maxHeapFreeRatio, 100, "MaxHeapFreeRatio must be less than 100");”

Shouldn’t that be "Asserts.assertLTOE (<=)", not “ Asserts.assertLT”?

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?

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)) {“

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

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         }

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

3 b.

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

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.

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