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.
http://cr.openjdk.java.net/~shurailine/8159523/webrev.02/ Shura > 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. > > 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.
