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

[David Holmes]

One follow up at end ...

On 29/04/2020 4:29 pm, Stefan Karlsson wrote:
> 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.

Good point on atomicity - that's important for our test logs.

Thanks,
David

> 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
>>>
>