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

[Alexandre (Shura) Iline]

Thank you, Mandy!

Comments inline.
> On Mar 6, 2017, at 6:00 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> 
>> 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.  

I see a point in doing that. At the same time I want to point out that there will be cases when the user will be forced to do a concatenation such as
.addExports(JAVA_BASE + “/“ + JDK_MISC, ALL_UNNAMED)
i.e. JAVA_BASE is a constant which is used multiple times in the code.

This not a deal breaker - I will change it in the next iteration.

> Also specifying the target explicitly makes the test clearer what it does.
> 
>  .addExports(“java.base/jdk.internal.reflect”, “ALL-UNNAMED”)

Makes sense.

I hope though that you are not against having a constant for “ALL-UNNAMED”, such as Task.ALL_UNNAMED. A lot of people, myself included prefer usage of constants over the copy-pasted value.

> 
> 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.
OK.
> 
> 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 
Yes.
> 
> classArgs(String… args) might be better to rename to mainArgs(String… args).
OK.
> 
> 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.

There will be a further discussion on this - Igor will be summarizing it in a separate e-mail. What he will be suggesting is 
.stderr().shouldContain(“IllegalAccessError”) 
or something similar.

> 
> 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”?

Sounds good to me.

> 
> 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?

Yes for filterStandardOption(…).
I meant  ignoreStandardOption(String, int)  as a public API. Some options are supported by JavaOptions, but there could be others which are not. But I agree that we can hide it for now - there may never be a case for it.

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

Yes.

> 
> TaskError - Alan suggests to be TaskException extends RuntimeException instead.

OK.

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

Makes sense.

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

Probably also make it private for now. Or remove - we can re-introduce it later.

> 
> I suggest the package name be “jdk.testlibrary.task” singular rather than plural.
ok

It will also need to go to the top-level test library, to allow HS team to use it.

Shura
> 
> Mandy
>