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

[Vicente-Arturo Romero-Zaldivar]

On 11/12/13 12:58, Andrey Nazarov wrote:
> Vicente, could you be so kind to sponsor my fix?

Sure I can do it,

Vicente

>
> --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/4caaef76/attachment.html