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

[Alan Bateman]

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.

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?) 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.

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.

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

-Alan.