> On Mar 8, 2017, at 5:21 PM, Alexandre (Shura) Iline > <alexandre.il...@oracle.com> wrote: >> 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.
I am not against defining a constant variable (I see the benefit of doing that to avoid typo). However, Task is not the right class to define ALL_UNNAMED. Another alternative is to introduce a new Modules class to define those strings which seems overkill. A typo will be caught when the test is run and it isn’t too bad. >> >> 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. > That may be fine. > > 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. Sounds good. Add it when we identify a need for it. > >> >> 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 to remove it as a clean start. Mandy
