> 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:
>
>> 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.
