RFR: 8315097: Rename createJavaProcessBuilder [v3]

[Mark Sheppard]

On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

>> Rename createJavaProcessBuilder so that it is not used by mistake instead of createTestJvm.
>> 
>> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"`
>> 
>> Then I have manually modified ProcessTools.java. In that file I have moved one version of createJavaProcessBuilder so that it is close to the other version. Then I have added a javadoc comment in bold telling:
>> 
>>    /**
>>      * Create ProcessBuilder using the java launcher from the jdk to
>>      * be tested.
>>      *
>>      * <p><b> Please observe that you likely should use
>>      * createTestJvm() instead of this method because createTestJvm()
>>      * will add JVM options from "test.vm.opts" and "test.java.opts"
>>      * </b> and this method will not do that.
>>      *
>>      * @param command Arguments to pass to the java command.
>>      * @return The ProcessBuilder instance representing the java command.
>>      */
>> 
>> 
>> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a better name I could do a rename of the method. I kind of like that it is long and clumsy, that makes it harder to use...
>> 
>> I have run tier 1 testing, and I have started more exhaustive testing.
>
> Leo Korinth has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix static import

> From talking to other HotSpot devs it is quite clear that the different names lead to mistakes when writing (copy-n-pasting) tests, so I'm happy that we try to figure out some way to make it more obvious when we prepend the extra test options and when we don't.
> 
> I agree with @msheppar that `createTestJvm` isn't a good name and I wouldn't be opposed to changing that name instead of `createJavaProcessBuilder`. However, if we do rename that function I strongly urge us to also rename the corresponding `executeTestJvm` function.
> 
> I also think it is too obscure that functions with _Test_ in the name prepend the extra test options, and those without _Test_ don't, so I'd like to get rid of that convention.
> 
> I wouldn't be opposed to a change that:
> 
>     * Keeps the `createJavaProcessBuilder` name
> 
>     * Renames `createTestJvm` to `createJavaProcessBuilderPrependTestOpts`
> 
>     * Renames `executeTestJvm` to `executeJavaPrependTestOpts`
> 
>     * Removes `createTestJava`
> 
>     * Removes `executeTestJava`

I think @stefank made a reasonable suggestion which was endorsed by @AlanBateman 

which would remove the misconception hurdle

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1707391042