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

[Ioi Lam]

Hi Stefan,

For a future RFE, I would suggesting adding a new function

     ProcessTools.createJavaProcessBuilder(Object... args)

Every argument must be a String, a String[], a List<String>, or NULL.

You can simplified code like this:

    static ProcessBuilder exec(String... args) throws Exception {
         List<String> argsList = new ArrayList<>();
         Collections.addAll(argsList, args);
         Collections.addAll(argsList, "-Xmn8m");
         if (cond) {
             Collections.addAll(argsList, "-Dtest.classes=" + 
System.getProperty("test.classes","."));
         }
         Collections.addAll(argsList, ClassUnloadTestMain.class.getName());
         return ProcessTools.createJavaProcessBuilder(argsList);
     }

to this:

     static ProcessBuilder exec(String... args) throws Exception {
         return ProcessTools.createJavaProcessBuilder(
                   args,
                   "-Xmn8m",
(cond) ? "-Dtest.classes=" + System.getProperty("test.classes",".") : NULL,
ClassUnloadTestMain.class.getName());
     }

We can probably allow higher-level dimensions like Object[][], as long 
as the eventual element type is a String.

What do you think?

- Ioi



On 4/28/20 6: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'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
>