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

[Vicente-Arturo Romero-Zaldivar]

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/82256f9c/attachment.html