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 <> wrote:
> On 07/10/2016 21:24, Alexandre (Shura) Iline wrote:
>> Hi,
>> Please review a fix for
>> 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:
> 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.

Reply via email to