RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Chris Plummer]

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