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

[Stefan Karlsson]

On 2020-04-29 07:12, David Holmes wrote:
> Hi Stefan,
>
> On 28/04/2020 11:54 pm, 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.
>
> createJavaProcessBuilder is the primitive and over time we have added 
> higher-level APIs to abstract away the boiler-plate that deal with the 
> externally passed flags, gives you an OutputAnalyzer etc.
Right, and what I'm saying is that I think a lot of the code could 
benefit from using the higher-level APIs. Though I'm not sure the once 
we have are an exact match to what we need, so we might have to add some 
more, or tweak some of the old once.

>
> The underlying java.lang.ProcessBuilder supports both List<String> and 
> String[] for passing arguments, but returns the command as 
> List<String>, so this has also affected the ProcessTools API. As 
> Leonid suggests adding in a List<String> overload suffices rather than 
> Collection<String>.

Yes, and I answered Leonid's review and said that I would change it to 
List<String>. I've tested that version with tier1-5.

>
>
>> 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
>
> One thing I noticed, as you copied it, is that this comment to all the 
> createJavaProcessBuilder methods seems wrong:
>
> 271      * with any platform specific arguments prepended
>
> as we only add the other arguments if we pass "true" for 
> addTestVmAndJavaOptions. ??

I saw that, but thought that the @param addTestVmAndJavaOptions 
disambiguated this. I'll remove that part from all the comments.

>
> Also regarding the String[]/List conversions ... as ProcessBuilder 
> supports both I really don't see why we are doing:
>
>  334         return new ProcessBuilder(args.toArray(new 
> String[args.size()]));

I'll fix that as well.

>
>
> and the "reporting" section could just iterate the args rather than 
> creating a StringBuilder.

I'm unsure about that. I can't tell if this is done to ensure that the 
reporting is atomic. I'll leave this for someone else to fix.

Thanks,
StefanK

>
> Cheers,
> David
>
>> 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
>>