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
