RFR: 8244010: Simplify usages of ProcessTools.createJavaProcessBuilder in our tests

[coleen.phillimore at oracle.com]

I clicked on the runtime and serviceability tests and they look good to 
me.  This is definitely an improvement.

- pb = ProcessTools.createJavaProcessBuilder(runJava.toArray(new 
String[0]));
+ pb = ProcessTools.createJavaProcessBuilder(runJava);



On 4/28/20 9:54 AM, Stefan Karlsson wrote:
> Hi again,
>
> I realized that we probably want to give ProcessTools.executeTestJvm 
> the same treatment.
>
> Side-note: It's very awkard that createJavaProcessBuilder defaults to 
> not adding user-specifed flags, but executeTestJvm does. I think it 
> would be good to unify this as a separate RFE. I think *a lot* of 
> callers to createJavaProcessBuilder could be simplified by either 
> using executeTestJvm directly, or a simplified version of that.

I never knew there were two.  Why are they different?  It looks like 
there are more createJavaProcessBuilder calls than executeTestjvm ones 
though.  It would be good to have just one.

Thanks,
Coleen

>
> I'm running testing through mach5 and found a few things to fix, I 
> might find more when the testing has proceeded further.
>
> This is the current patch:
> https://cr.openjdk.java.net/~stefank/8244010/webrev.02.delta
> https://cr.openjdk.java.net/~stefank/8244010/webrev.02
>
> Thanks,
> StefanK
>
> On 2020-04-28 13:58, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to simplify usages of 
>> ProcessTools.createJavaProcessBuilder in our tests.
>>
>> https://cr.openjdk.java.net/~stefank/8244010/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8244010
>>
>> I saw all this code when reviewing changes to how we pass flags in 
>> our tests. There are a many places where arguments are converted and 
>> passed back and forth in String[] and Collections.
>>
>> For example:
>>   ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
>>           argsList.toArray(new String[argsList.size()]));
>>
>> If we add an overload the createJavaProcessBuilder, that takes a 
>> Collection<String> as an argument, then we can write the code above as:
>>    ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(argsList);
>>
>> Other places temporarily put the flags in a String[], where most 
>> calls simply lists the arguments in the call:
>>   String[] opts = {Xmx, "-XX:NativeMemoryTracking=detail", 
>> "-XX:+UseParallelGC", "-version"};
>>   ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(opts);
>>
>> And some places put the args in a temporary Collection:
>>   LinkedList<String> vmOptions = new LinkedList<>();
>>   vmOptions.add(gc);
>>   vmOptions.add("-Xmx" + minMaxHeap);
>>   vmOptions.add("-XX:+PrintFlagsFinal");
>>   vmOptions.add(VerifyHeapSize.class.getName());
>>
>>   ProcessBuilder pb = 
>> ProcessTools.createJavaProcessBuilder(vmOptions.toArray(new String[0]));
>>
>> I'd like to cleanup, simplify, and unify many of these usages.
>>
>> I've tested this by running all the changed tests locally.
>>
>> Thanks,
>> StefanK
>