RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

> On Oct 10, 2016, at 2:54 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
> 
> On 07/10/2016 21:24, Alexandre (Shura) Iline wrote:
> 
>> Hi,
>> 
>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>> 
>> To recap what has happened in the past.
>> 
>> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to support some filtering java and VM options. That implementation came out really overloaded (check executeModularTest(...) in [1]).
>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>> 
>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to realize that there are many questions which may cause design discussions. So, instead, I have rolled back to one class (JavaTask) and the superclasses. If the design is acceptable, more tasks could be added as needed.
>> 
>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01/
>> 
> At a high-level then the approach looks reasonable, just lots of details to get agreement on before doing a big conversion.
> 
> One thing that jumps out is that JavaTask doesn't seem to handle repeated options, I'll assume that will be fixed.

Do you think there are many cases in the test code with the repeated options? the current implementation allows lists for --add-modules, —add-exports, I would also do it where it makes sense for other options. For negative testing then there is vmOptions, which could contain anything.

Not that I had anything against supporting multiple options. Just trying to better understand the use case.

> 
> In the examples then I suspect that "mainClass" be be clearer than "className". Also "module" might work better than "moduleName". One thing you could look at for the module method is an overload that takes 2 parameters, the module name and main class. Another suggestion is methods such as modulepath (modulePath?)

Then classPath also, I think. It was “classpath” in the original source, so I added “modulepath”.

> should have an overload that takes a Path for the common cases where tests just specify one path element - if have those then a lot of calls to toString go away.

The other idea I had is to use varargs for the paths: modulePath(String... modulePath) and modulePath(Path... modulePath). That would allow to avoid the concatenation in the test code.

> 
> One discussion point is whether it should support all options. I note the usage in one of the examples:
> 
> .vmOptions("--upgrade-module-path", UPGRADE_MODS_DIRS.toString())
> .modulepath(MODS_DIR.toString())
> 
> where
> 
> .upgradeModulePath(UPGRADE_MODS)
> .modulePath(MODS_DIRS)
> 
> would be clearer.

I will add all the missing options.

Shura

> 
> Anyway, these are just some passing comments, I do agree that the tests using ProcessTools could be more readable.
> 
> -Alan.