RFR for JDK-8028708. Tests Tests should pass through VM options - accepted

[Andrey Nazarov]

Vicente, could you be so kind to sponsor my fix?

--Andrey
On 11.12.2013 16:29, Vicente-Arturo Romero-Zaldivar wrote:
> Hi Andrey,
>
> The webrev.02 is OK for me,
>
> Thanks,
> Vicente
>
>
> On 11/12/13 12:00, Andrey Nazarov wrote:
>> Hi Vicente,
>>
>> I've updated files as you suggested.
>> http://cr.openjdk.java.net/~anazarov/JDK-8028708/webrev.02 
>> <http://cr.openjdk.java.net/%7Eanazarov/JDK-8028708/webrev.02>
>>
>> --Thanks,
>> Andrey.
>>
>> On 11.12.2013 14:16, Vicente-Arturo Romero-Zaldivar wrote:
>>> Hi Andrey,
>>>
>>> Thanks for the updated patch, some comments:
>>>
>>> I don't see the need for this:
>>>
>>> public static final String testVMOptions = System.getProperty("test.vm.opts", "");
>>> public static final String testToolVMOptions = System.getProperty("test.tool.vm.opts","");
>>> public static final String testJavaOptions = System.getProperty("test.java.opts", "");
>>>
>>> public static final List<String> testVMOpts = splitOptions(testVMOptions);
>>> public static final List<String> testToolVMOpts = splitOptions(testToolVMOptions);
>>> public static final List<String> testJavaOpts = splitOptions(testJavaOptions);
>>>
>>>
>>> There are two fields with basically the same information one field 
>>> has a String value and the other one has the same split String 
>>> value. I think that what is used is only the split String value so I 
>>> don't see the need for this duplication.
>>>
>>> Also if you look for javac jtreg tests using ToolBox you will see 
>>> that you can do with ToolBox the same that you are doing with 
>>> "List<String> cmd" and ProcessBuilder, also ToolBox adds some 
>>> features like checking for an expected result, success or error. So 
>>> I think that if you are using ToolBox for solving this issue you 
>>> should use it for what it's intended for. We also prefer tests to be 
>>> as homogeneous as possible.
>>>
>>> Thanks,
>>> Vicente.
>>>
>>> On 10/12/13 13:57, Andrey Nazarov wrote:
>>>> Hi everyone,
>>>>
>>>> I fixed path to java binary and introduced shared utility method.
>>>> http://cr.openjdk.java.net/~anazarov/JDK-8028708/webrev.01/ 
>>>> <http://cr.openjdk.java.net/%7Eanazarov/JDK-8028708/webrev.01/>
>>>>
>>>> On 09.12.2013 23:14, Vicente-Arturo Romero-Zaldivar wrote:
>>>>> On 09/12/13 16:32, Jonathan Gibbons wrote:
>>>>>> On 12/09/2013 08:03 AM, Andrey Nazarov wrote:
>>>>>>> Hi everyone,
>>>>>>>
>>>>>>> I'm fixing following tests.
>>>>>>> tools/javac/api/ToolProvider/HelloWorldTest.java
>>>>>>> tools/javac/api/ToolProvider/ToolProviderTest1.java
>>>>>>> tools/javac/api/ToolProvider/ToolProviderTest2.java
>>>>>>>
>>>>>>> Tests should pass through VM options.
>>>>>>>
>>>>>>> Root cause:
>>>>>>> VM options are not passed to java executed by java programs.
>>>>>>>
>>>>>>> SUGGESTED FIX:
>>>>>>> Add System.getProperty("test.vm.opts","") and 
>>>>>>> System.getProperty("test.java.opts","") to invocations of the 
>>>>>>> java cmd for *.java tests.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~anazarov/JDK-8028708/webrev.00/ 
>>>>>>> <http://cr.openjdk.java.net/%7Eanazarov/JDK-8028708/webrev.00/>
>>>>>>>
>>>>>>> --Thanks,
>>>>>>> Andrey
>>>>>>
>>>>>> The fix is OK, but in general it is not important to be able to 
>>>>>> run javac tests under a variety of externally provided JVM 
>>>>>> options.  Do you have a particular reason for the fix?
>>>> Issue JDK-8028708 was assigned to me, so I just provide fix.
>>>>>>
>>>>>> Since you've written the same code 3 times in ToolBox.java, you 
>>>>>> might want to use a shared utility method instead.
>>>>>>
>>>>>> -- Jon
>>>>>
>>>>> Hi Andrey,
>>>>>
>>>>> I can also see that the tests need to have a path to bin/java this 
>>>>> is also provided by Toolbox through the static field javaBinary.
>>>>>
>>>>> Vicente
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20131211/fa327705/attachment-0001.html