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

[Stefan Karlsson]

On 2020-04-28 16:10, coleen.phillimore at oracle.com wrote:
>
> 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);

Thanks. See below:

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

One big difference is that one provides a ProcessBuilder and the other 
hides it and returns an OutputAnalyzer:

     public static OutputAnalyzer executeTestJvm(String... cmds) throws 
Exception {
         ProcessBuilder pb = createJavaProcessBuilder(true, cmds);
         return executeProcess(pb);
     }

There is a difference between executeProcess and new 
OutputAnalyzer(pb.start()), but if we ignore that and focus on what the 
test code would look like, we could often completely get rid of the 
notion of a ProcessBuilder from our tests.

If we take test/hotspot/jtreg/runtime//logging/ClassResolutionTest.java 
as an example. We could change:

   ProcessBuilder pb = 
ProcessTools.createJavaProcessBuilder("-Xlog:class+resolve=debug",
ClassResolutionTestMain.class.getName());
   OutputAnalyzer o = new OutputAnalyzer(pb.start());
   o.shouldContain("[class,resolve] 
ClassResolutionTest$ClassResolutionTestMain$Thing1Handler 
ClassResolutionTest$ClassResolutionTestMain$Thing1");
   o.shouldContain("[class,resolve] resolve JVM_CONSTANT_MethodHandle");

To:

   OutputAnalyzer o = 
ProcessTools.executeTestJvm("-Xlog:class+resolve=debug",
ClassResolutionTestMain.class.getName());
   o.shouldContain("[class,resolve] 
ClassResolutionTest$ClassResolutionTestMain$Thing1Handler 
ClassResolutionTest$ClassResolutionTestMain$Thing1");
   o.shouldContain("[class,resolve] resolve JVM_CONSTANT_MethodHandle");

Or even get rid of the short lived variables (Not sure if it's 
preferable, but it's doable):

   ProcessTools.executeTestJvm("-Xlog:class+resolve=debug",
ClassResolutionTestMain.class.getName())
               .shouldContain("[class,resolve] 
ClassResolutionTest$ClassResolutionTestMain$Thing1Handler 
ClassResolutionTest$ClassResolutionTestMain$Thing1");
               .shouldContain("[class,resolve] resolve 
JVM_CONSTANT_MethodHandle");

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