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

[Andrey Nazarov]

Thanks
On 11.12.2013 18:10, Vicente-Arturo Romero-Zaldivar wrote:
> 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/e52b0a29/attachment-0001.html