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

[Andrey Nazarov]

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/915c2dd1/attachment.html