> On Mar 6, 2017, at 5:27 PM, Alexandre (Shura) Iline
> <alexandre.il...@oracle.com> wrote:
>
> Hi,
>
> Could you please review the suggested fox for:
> https://bugs.openjdk.java.net/browse/JDK-8159523
>
> There has been a bit of discussion on jigsaw-dev, but it perhaps make sense
> to include core-libs-dev.
>
> There have been some fixes since the review was published, so we are now at
> revision #4:
> http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/
>
I reviewed webrev.04 which is in a good shape. Here are my comments:
W.r.t. the JavaTask methods:
110 new JavaTask()
111 .addExports("java.base", "jdk.internal.reflect")
112 .vmOptions("-version")
113 .run();
I prefer it to use <module>/<package> to represent the source in the same way
as the command-line argument to `—-add-exports` option, as Alan suggests. Also
specifying the target explicitly makes the test clearer what it does.
.addExports(“java.base/jdk.internal.reflect”, “ALL-UNNAMED”)
Similiarly, module(String mid) takes the argument passed to `—-module`.
addReads(String source, String… targets)
I suggest to require a non-empty targets. i.e. ALL-UNNAMED must be specified.
`—-add-modules` is a repeating option and JavaTask::addModules does not support
that.
148 .addModules("jdk.naming.dns,jdk.compiler”)
This should be “addModules(“jdk.naming.dns”, “jdk.compiler”)”
JavaTask should be extended for the new `-—add-opens` option
classArgs(String… args) might be better to rename to mainArgs(String… args).
modulePath, upgradeModulePath, classPath - maybe just take Path arguments.
136 .shouldContain(Task.OutputKind.STDERR, "IllegalAccessError");
164 .shouldContain(Task.OutputKind.STDOUT, "specified more than
once”);
It may be useful to add “stdoutShouldContain” and “stderrShouldContain” method,
like what ProcessTools provides.
ignoreStandardOptions()
ignoreStandardModuleOptions()
It might be confusing what “standard options” are. They are the options
configured by jtreg and inherited from the test run.
What about “doNotInheritTestOptions” or “doNotInheritTestRuntimeOptions”?
It seems better for JavaTask constructor to take a boolean/enum flag. Instead
of a public JavaTask constructor, what about JavaTask.newTask(Enum) or
JavaTask.newTaskWithInheritTestRuntimeOptions() explicit factory methods?
Either way is fine with me.
public JavaTask ignoreStandardOption(String option, int parameterCount) {
public JavaTask filterStandardOption(Function<List<String>, List<String>>
filter) {
- should these methods be private?
what if classArgs and module are both called? The builder should detect that
and throw IAE to help troubleshooting since the main class should either be in
a named module or just class name.
TaskError - Alan suggests to be TaskException extends RuntimeException instead.
AbstractTask.java
System.out.println("Running " + pb.command().stream().map(s -> "\"" + s +
"\"").collect(Collectors.joining(",")));
- it would be useful to print the command-line that I can cut-n-paste to a
shell and rerun.
i.e. no need to wrap with double quotes except -classpath and --module-path
etc. Use space rather than “,” as separator.
Task.Mode.* is defined to launch via different mechanism. For java launcher,
it has only one way. For jar, jlink, jmod and other tools, we can now use
java.util.ToolProvider. It might be useful if we had a JlinkTask in the
future. We should either drop Task.Mode or update its comments.
Should we remove the use of JDKToolFinder since I assume jdk.testlibrary.task
will replace jdk.testlibrary.* helper classes in the future.
I suggest the package name be “jdk.testlibrary.task” singular rather than
plural.
Mandy
