Could you please take another look?
I have added more options, and fixed other things you have pointed out. I have
also picked up a couple more tests to cover the newly added methods.
> On Oct 10, 2016, at 2:54 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> On 07/10/2016 21:24, Alexandre (Shura) Iline wrote:
>> 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 ).
>> 2. It was suggested that a builder API could be introduced instead.
>> “toolbox” classes from langtools test library were suggested as a start .
>> 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
>> 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())
> would be clearer.
> Anyway, these are just some passing comments, I do agree that the tests using
> ProcessTools could be more readable.