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

[David Holmes]

Hi Stefan,

On 30/04/2020 7:07 pm, 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)

This all looks good to me. I think this high/low API makes things much 
clearer.

An observation from compiler/runtime/cr8015436/Driver8015436.java - can:

oa = ProcessTools.executeProcess(ProcessTools.createTestJvm(...));

be replaced with

oa = executeTestJvm(...);

?

---

test/hotspot/jtreg/gc/arguments/GCArguments.java

Isn't the String[] <-> List<String> conversion already handled in 
ProcessTools?

This looks like an area where GC added its own helper utilities early on 
and they aren't really needed any more. Future RFE?

---

test/hotspot/jtreg/gc/g1/mixedgc/TestLogging.java
test/hotspot/jtreg/gc/nvdimm/*

Just observation - these tests also use a code pattern that looks like 
it could use executeTestJvm instead.

---

test/lib/jdk/test/lib/process/ProcessTools.java

This looks a bit odd:

    /**
      * @see #executeTestJvm(String...)
      * @param cmds User specified arguments.
      * @return The output from the process.
      */
     public static OutputAnalyzer executeTestJava(String... cmds) throws 
Exception {
         return executeTestJvm(cmds);
     }

I assume this exists because this was the name used in the JDK test 
library originally and many (most?) of the JDK tests call it rather than 
executeTestJvm. There are a couple of hotspot callers that should 
probably be converted over:

./jtreg/compiler/jvmci/errors/TestInvalidTieredStopAtLevel.java
./jtreg/compiler/jvmci/TestValidateModules.java

otherwise another future RFE to switch over to single API.

> 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 

As there are no callers of the addTestVmAndJavaOptions(boolean) API 
these changes seem fine. I can understand why the ability to select 
whether or not to add the test harness args was made available, but it 
seems no test is concerned about using it, so it can go.

Thanks,
David
-----

> 
> I've run this through tier1-3, and are currently running this through 
> higher tiers.
> 
> Thanks,
> StefanK