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

David Holmes david.holmes at oracle.com
Fri Jun 19 04:09:58 UTC 2015 

Hi Dmitry,

I have no further comments.

Thanks,
David

On 18/06/2015 5:29 AM, Dmitry Dmitriev wrote:
> Hi Gerard,
>
> Can you please look at small enhancement that I made by Christian
> suggestion - ran test JVM with the same type as current(for all cases,
> not only for "-client" as in 06 webrev). Tested by JPRT. Sorry for
> confusion. Thank you!
>
> Webrev:
> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.07/>http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.07/
> Webrev incremental(from 06):
> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.07.incremental.from06/>http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.07.incremental.from06/
>
> Regards,
> Dmitry
>
> On 17.06.2015 18:07, Gerard Ziemski wrote:
>> hi Dmitry,
>>
>> Please consider this reviewed (with small "r").
>>
>> Just one quick question: is there an issue filed to followup on why
>> the test hangs with CICompiler and to enable it back, once we figure
>> it out?
>>
>>
>> cheers
>>
>> On 6/16/2015 10:58 AM, Dmitry Dmitriev wrote:
>>> Hello,
>>>
>>> Here a updated version of test set for Command Line Option
>>> Validation. The main changes are: added new options types - int and
>>> uint, added @modules, test all options with range in
>>> TestOptionsWithRanges.java test.
>>>
>>> Tested: JPRT
>>>
>>> Webrev: http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.05/
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.05/>
>>> Webrev incremental(from 04):
>>> http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.05.incremental/
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.05.incremental/>
>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8059557
>>>
>>> Thanks,
>>> Dmitry
>>>
>>> On 03.06.2015 16:47, Dmitry Dmitriev wrote:
>>>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>

More information about the hotspot-dev mailing list