On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth <lkori...@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
