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

Tue Jun 16 19:29:53 UTC 2015

Hi Dmitry,

As mentioned in our private IM discussion, I’m concerned about the passing in external options (using ProcessTools.createJavaProcessBuilder(true,…)). The potential for test issues when passing in external args to this test is quite high, and the cost of running these command line tests with –Xcomp is very high and makes little sense.

Otherwise I think this looks good J

Thanks,

Christian

From: Dmitry Dmitriev [mailto:dmitry.dmitriev at oracle.com] 
Sent: Tuesday, June 16, 2015 11:59 AM
To: hotspot-dev at openjdk.java.net
Cc: David Holmes <david.holmes at oracle.com>; Gerard Ziemski <gerard.ziemski at oracle.com>; Christian Tornqvist <christian.tornqvist at oracle.com>
Subject: Re: RFR 8059557: Test set for "Validate JVM Command-Line Flag Arguments"

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/>  
 <http://cr.openjdk.java.net/%7Eddmitriev/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/>  
 <http://cr.openjdk.java.net/%7Eddmitriev/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/>  
 <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.00/> <http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.00/> 

JEP: https://bugs.openjdk.java.net/browse/JDK-8059557 

Thanks, 
Dmitry 