On 4/30/20 2:07 AM, Stefan Karlsson wrote:
Hi all,
Please review this patch to make it less likely that we accidentally
add or fail to add test.java.opts and test.vm.opts to our spawned test
JVMs.
https://cr.openjdk.java.net/~stefank/8244078/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8244078
ProcessTools.createJavaProcessBuilder(cmd) creates a ProcessBuilder
*without* test.java.opts and test.vm.opts. There is a
(addTestVmAndJavaOptions, cmd) overload that allows the caller to
opt-in to the addition of these flags. The created ProcessBuilder is
then used to start the JVM, and almost always fed into an OutputAnalyzer.
There's another function executeTestJvm, that both creates a
ProcessBuilder and then feeds it into an OutputAnalyzer (plus a bit
more). This function uses createJavaProcessBuilder(true, cmd), and
thereby adds the test.java.opts and test.vm.opts flags.
This means that one has to know about this difference when reading
code using createJavaProcessBuilder(cmd) and executeTestJvm(cmd), and
when creating (copying) code using these functions.
It has been suggested that createJavaProcessBuilder is intended to be
a lower-level, building block API and that it's not confusing that
they have different behavior. I don't really agree, but I'm buying
into the notion of lower-level vs higher-level APIs here. So, my
proposal is to remove the addTestVmAndJavaOptions feature from
createJavaProcessBuilder, and instead create a new function called
createTestJvm that adds test.java.opts and test.vm.opts. The name is
intentionally similar to executeTestJvm, and in fact, the
executeTestJvm implementation will use createTestJvm:
public static OutputAnalyzer executeTestJvm(String... cmds)
throws Exception {
- ProcessBuilder pb = createJavaProcessBuilder(true, cmds);
+ ProcessBuilder pb = createTestJvm(cmds);
return executeProcess(pb);
}
The rest of the patch is mainly:
- leaving createJavaProcessBuilder(cmd) as is
- replacing createJavaProcessBuilder(false, cmd) with
createJavaProcessBuilder(cmd)
- replacing createJavaProcessBuilder(true, cmd) with createTestJvm(cmd)
There was one odd thing in jdi that requires extra scrutiny:
https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udiff.html
Yes, that did look a odd at first glance, but I think that fact that
your removed addTestVmAndJavaOptions() and everything still built is
proof enough that it was just bit rotted code and not needed.
Chris
I've run this through tier1-3, and are currently running this through
higher tiers.
Thanks,
StefanK
