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

Wed Jun 3 13:47:05 UTC 2015

Hello David,

Thank you for pointing on style/grammar mistakes! I corrected all of them.
Here a link to the updated 
webrev:http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.04/ 
<http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.04/>

About "options validation" package - I agree that this looks odd, but if 
you don't mind I prefer to leave it. It slightly simplify calling static 
methods by using static import and also I use package-private access level.

Regards,
Dmitry

On 02.06.2015 7:48, David Holmes wrote:
> Hi Dmitry,
>
> On 28/05/2015 11:32 PM, Dmitry Dmitriev wrote:
>> Hello all,
>>
>> Here is a 3 version of the tests taking into account feedback from
>> Christian, David and Gerard.
>>
>> I limit number of options in TestOptionsWithRanges.java to 15. This
>> include options of different types(intx, uintx etc.) and with different
>> combination of min/max range. TestOptionsWithRangesDynamic.java leaved
>> as is, because amount of manageable numeric options is very small and
>> currently only 3 of them have range. Also, I improve code for double
>> option.
>>
>> Webrev: http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.03/
>> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.03/>
>
> Only a few style/grammar nits (the downside of writing so many doc 
> comments :) ).
>
> Meta-question: is there a specific reason to use the package "options 
> validation"? It looks odd to me to have 
> OptionsValidation/common/optionsvalidation/ in the paths.
>
> General doc-comment comments:
> - @param/@return descriptions should start with lower-case (you 
> currently mix-n-match)
> - @throws description should start with "if", so:
>      @throws IOException if an error occurred reading the data
>
>
> General Java-style comments:
> - access modifiers (public, private, protected) always come first
>
> ---
>
> test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/IntJVMOption.java 
>
>
> 265                  * passed value is negative then error will be 
> catched earlier for
>
> catched -> caught
>
> ---
>
> test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOption.java 
>
>
> 327      * @param param Tested parameter passed to the java
>
> "the java" is not a noun - suggest "the JVM" ?
>
> Maybe change Java to JVM to avoid use of "java" as a noun everywhere.
>
>  350             failedMessage(name, fullOptionString, valid, "JVM 
> output reports about fatal error. JVM exit with code " + returnCode + 
> "!");
>
> The message isn't grammatically correct: about -> a; exit -> exited
>
> Similarly "JVM exit" -> "JVM exited"
>
>  377                 failedMessage(name, fullOptionString, valid, "JVM 
> output not contains "
>
> not contains -> does not contain
>
> 400      * Return list of strings which contain options with valid 
> values which used
>
> which used -> which can be used
>
>  416      * Return list of strings which contain options with invalid 
> values which
>  417      * used for testing on command line
>
> Ditto
>
> ---
>
> test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java 
>
>
> 101      * Add dependency for option depending on it's type. E.g. ran 
> java in
>
> ran java -> run the JVM
>
> 119      * @param withRanges true if needed options with defined ranges
>
> I'm not sure what this means. Occurs elswhere too.
>
> 121      * "product", "diagnostic" etc. Accept option only if 
> acceptOrigin return
>
> return -> returns (or even return -> evaluates). Occurs elsewhere too.
>
> 260      * methods. Tested only writeable options.
>
> Suggestion: Only tests writeable options. Occurs elsewhere too.
>
> 264      * @throws Exception
>
> When? Why? Occurs elsewhere too.
>
> Thanks,
> David
> -----
>
>> Thanks,
>> Dmitry
>>
>>
>> On 21.05.2015 23:57, Dmitry Dmitriev wrote:
>>> Hello all,
>>>
>>> Recently I correct several typos, so here a new webrev for tests:
>>> http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.02/
>>> <http://cr.openjdk.java.net/%7Eddmitriev/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/
>>>> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.00/>
>>>>
>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8059557
>>>>
>>>> Thanks,
>>>> Dmitry
>>>>
>>>
>>