RFR 8059557: Test set for "Validate JVM Command-Line Flag Arguments"
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Tue Jun 16 20:29:48 UTC 2015
Hi Christian,
Thank you for review. Yes, I agree with your concern. I run tests with
"-Xcomp" and it takes a lot of time. I rewrite the logic and remove
passing in external options. Also in this case I add special case for
client mode, i.e. if test ran in client mode, then also verify options
with client mode.
Webrev: http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.06/
<http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.06/>
Webrev incremental(from 04):
http://cr.openjdk.java.net/~ddmitriev/8059557/webrev.06.incremental/
<http://cr.openjdk.java.net/%7Eddmitriev/8059557/webrev.06.incremental/>
Thanks,
Dmitry
On 16.06.2015 22:29, Christian Tornqvist wrote:
>
> 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
>
More information about the hotspot-dev
mailing list