RFR: 8073331: [TESTBUG] Test for VM option file feature (VM options specified in file)

[Dmitry Dmitriev]

Hi George,

Thank you for review! I will correct all typos.

About JVM_FAIL_1 - it means fail with 1 exit code, but probably it seems confusing and I will change it just to JVM_FAIL. 

Thanks,
Dmitry

----- Original Message -----
From: george.triantafillou at oracle.com
To: hotspot-runtime-dev at openjdk.java.net
Sent: Wednesday, September 23, 2015 2:28:05 AM GMT +03:00 Iraq
Subject: Re: RFR: 8073331: [TESTBUG] Test for VM option file feature (VM options specified in file)

Hi Dmitry,

In test/runtime/CommandLine/VMOptionsFile/TestVMOptionsFile.java:

89     /* VM Option file which not exist */
   Typo:  "which does not exist"

99     private static final int JVM_FAIL_1 = 1;
   Just a comment.  I'm curious why you chose to add "_1" to the 
variable name.

107     /* VM Options which passed to the JVM */
   Typo: "which are passed to"

317         /* Check that parameters are gotten from third VM Option 
file. Contain mix of the options */
   Change to "from third VM Option file which contains a mix of the options"

504          * The same as previous but without arguments from pseudo 
command line..
   Typo: Remove second period, change to "pseudo command line."

555         /* Pass VM option file which is not accessible(without read 
permissions) */
   Typo: Add a space before the open parenthesis: "which is not 
accessible (without"

Otherwise, this looks good.  I don't need to see a new webrev if you 
make these changes.

-George

On 9/22/2015 4:34 PM, Dmitry Dmitriev wrote:
> one more reviewer and sponsor. Thanks!
>
> Dmitry
>
> On 22.09.2015 19:13, Daniel D. Daugherty wrote:
>> Hi Dmitry,
>>
>> test/runtime/CommandLine/VMOptionsFile/TestVMOptionsFile.java
>>     I missed one typo:
>>
>>     -     * Make file non-readable by modifing it permissions.
>>     -     * If file support "posix" attributes, then modify it.
>>     +     * Make file non-readable by modifing its permissions.
>>     +     * If file supports "posix" attributes, then modify it.
>>
>>     Typo: 'modifing' -> 'modifying'
>>
>> test/runtime/CommandLine/VMOptionsFile/optionFILE_2
>>     No comments.
>>
>> test/runtime/CommandLine/VMOptionsFile/optionfile_quote
>>     No comments.
>>
>> Thumbs up. I don't need to see another webrev if you fix the above.
>>
>> Dan
>>
>>
>> On 9/22/15 8:07 AM, Dmitry Dmitriev wrote:
>>> Hello Dan,
>>>
>>> Thank you for comments! Here an updated webrev:
>>> webrev.01: http://cr.openjdk.java.net/~ddmitriev/8073331/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8073331/webrev.01/>
>>> webrev.01.vs.00: 
>>> http://cr.openjdk.java.net/~ddmitriev/8073331/webrev.01.vs.00/ 
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8073331/webrev.01.vs.00/>
>>> Note: 2 option files were changed, because I add tests for another 
>>> two properties.
>>>
>>> Thank you,
>>> Dmitry
>>>
>>> On 22.09.2015 0:12, Daniel D. Daugherty wrote:
>>>> On 9/15/15 9:08 AM, Dmitry Dmitriev wrote:
>>>>> Hello,
>>>>>
>>>>> Please review a new test for recently added feature: JDK-8061999 
>>>>> "Enhance VM option parsing to allow options to be specified in a 
>>>>> file".  Also, I need a sponsor for this fix.
>>>>>
>>>>> This test verifies new "-XX:VMOptionsFile" VM Options. Several 
>>>>> valid VM Options files are tested: with different number and types 
>>>>> of options, empty file, file with only tab and spaces, file which 
>>>>> contains properties with quotes. Also invalid situations are 
>>>>> tested: not existing file, folder instead of the file, file with 
>>>>> size greater than allowed, several VM option files and so on. 
>>>>> Also, test verifies "last argument wins" situation: when the same 
>>>>> option or property is defined several times in environment 
>>>>> variables, on command line and in VM Options file. In this case 
>>>>> option or property should get the last value from the most 
>>>>> priority source. Here a priority of the sources(from less priority 
>>>>> to the more priority):
>>>>> 1) Flags file
>>>>> 2) JAVA_TOOL_OPTIONS environment variables
>>>>> 3) Pseudo command line from launcher(which can include VM Options 
>>>>> file)
>>>>> 4) _JAVA_OPTIONS
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~ddmitriev/8073331/webrev.00/
>>>>
>>>> I was also expecting to see 
>>>> test/runtime/VMOptions/EnvironmentTest.java,
>>>> but perhaps that new test is coming via a different bug ID...
>>>>
>>>> I don't see the following test data files in the webrev:
>>>>
>>>>     optionfile_with_optionfile
>>>>     optionfile_with_same_optionfile
>>>>     optionfile_wo_read_perm
>>>>
>>>> Update: OK, I see now. These are generated by the test...
>>>>
>>>>
>>>> test/runtime/CommandLine/VMOptionsFile/TestVMOptionsFile.java
>>>>     L122      * Make file non-readable by modifing it permissions.:
>>>>         Typo: 'it' -> 'its'.
>>>>
>>>>     L123:      * If file support "posix" attributes,
>>>>         Typo: 'support' -> 'supports'
>>>>
>>>>     L132:         } else if (supportedAttr.contains("acl")) {
>>>> <snip>
>>>>     L145:             view.setAcl(acl);
>>>>         Wow! Java makes ACL code look easy. This is great evolution
>>>>         from POSIX 1003.6 APIs...
>>>>
>>>>     L157:         /* Create VM option file with following 
>>>> parametares "-XX:VMOptionFile=<absolute_path_to_the_VM_option_file> */
>>>>         Typo: 'parametares' -> 'parameters'.