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

[Vicente-Arturo Romero-Zaldivar]

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/9cbfff78/attachment.html