> On Oct 11, 2016, at 1:35 PM, Alexandre (Shura) Iline
> <alexandre.il...@oracle.com> wrote:
>
>
>> 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.
Actually —add-exports is implemented incorrectly. I see the point now.
Thanks.
Shura
>
> 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.
>
