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.
