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

[coleen.phillimore at oracle.com]

On 4/28/20 11:46 AM, Stefan Karlsson wrote:
> 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");

Yes, both of these are better.  Less stuff to cut/paste for writing new 
tests.

thanks!
Coleen

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