RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Mandy Chung]

> On Mar 6, 2017, at 5:27 PM, Alexandre (Shura) Iline <alexandre.iline at 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