Agreed and Reviewed!
On Wed, May 25, 2016 at 11:57 AM, Andrey Nazarov <andrey.x.naza...@oracle.com> wrote: > Hi Martin, > > See my comments inline. >> On 25 May 2016, at 20:48, Martin Buchholz <marti...@google.com> wrote: >> >> Hi Andrey, >> >> Looking at http://openjdk.java.net/jtreg/vmoptions.html, >> I see we have both test.vm.opts and test.tool.vm.opts >> and the latter is supposed to take care of adding "-J". >> >> + VM_OPTIONS.stream().map(opt -> "-J" + opt).forEach(commands::add); >> + JAVA_OPTIONS.stream().map(opt -> "-J" + opt).forEach(commands::add); > I considered this option for Basic.java test. But it doesn’t make code > clearer.The test requires both test.tool.vm.opts and test.vm.opts for jar > tool and java invocation. > For CLICompatibility.java test.tool.vm.opts is enough since test invokes > only jar tool. >> >> --- >> >> Maybe splitting on "\\s+" would be better. > In theory yes. I’ll update. >> >> --- >> >> I think we should have test library methods to return the List<String> >> for java subprocesses, one that could try hard to get the option >> tokenization correct. > Agree. But I think it should be separate patch with changes in tests that use > process builder with such option. Better we need some framework which will > pass options for us. It’s common problem not passing through VM options. >> >> --- >> >> >> On Wed, May 25, 2016 at 9:07 AM, Andrey Nazarov >> <andrey.x.naza...@oracle.com> wrote: >>> Some jar tests start VMs without passing vmoptions from jtreg. >>> >>> This fix pass jtreg's vmoptions and javaoptions to processes(java, jar, >>> javac) started by tests. >>> >>> webrev: http://cr.openjdk.java.net/~anazarov/8157850/webrev.01/ >>> >>> jbs: https://bugs.openjdk.java.net/browse/JDK-8157850 >>> >>> --Andrey >>> >
