<i18n dev> RFR: 8315097: Rename createJavaProcessBuilder [v6]

Roger Riggs rriggs at openjdk.org
Tue Oct 24 19:42:43 UTC 2023


On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

>> This pull request renames `createJavaProcessBuilder` to `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to `createTestJavaProcessBuilder`. Both are implemented through a private `createJavaProcessBuilder`. It also updates the java doc.
>> 
>> This is so that it should be harder to by mistake use `createLimitedTestJavaProcessBuilder` that is problematic because it will not forward JVM flags to the tested JVM.
>
> Leo Korinth has updated the pull request incrementally with six additional commits since the last revision:
> 
>  - static import
>  - copyright
>  - whitespace
>  - whitespace
>  - sed
>  - fix test/lib/jdk/test/lib/process/ProcessTools.java

test/lib/jdk/test/lib/process/ProcessTools.java line 506:

> 504:      */
> 505:     public static ProcessBuilder createTestJavaProcessBuilder(List<String> command) {
> 506:         return createTestJavaProcessBuilder(command.toArray(String[]::new));

The javadoc shoul d describe all of the options being added to the ProcessBuilder.
They were inadequated described previously and still are.
The other options (seem to be from the code), test.noclasspath, java.class.path, and test.thread.factory.
The description of test.thread.factory and addTestThreadFactoryArgs method seems inadequately described.

test/lib/jdk/test/lib/process/ProcessTools.java line 527:

> 525:      * Create ProcessBuilder using the java launcher from the jdk to
> 526:      * be tested.
> 527:      *

As above, should described the limited options that are added to the ProcessBuilder, the same as for `reateTestJavaProcessBuilder(...)`

test/lib/jdk/test/lib/process/ProcessTools.java line 549:

> 547:      * Create ProcessBuilder using the java launcher from the jdk to
> 548:      * be tested.
> 549:      *

As above, should described the limited options that are added to the ProcessBuilder, the same as for reateTestJavaProcessBuilder(...)

test/lib/jdk/test/lib/process/ProcessTools.java line 599:

> 597:      */
> 598:     public static OutputAnalyzer executeTestJvm(String... cmds) throws Exception {
> 599:         ProcessBuilder pb = createTestJavaProcessBuilder(cmds);

This should also describe *all* of the options being set in the ProcessBuilder before executing the process.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370728371
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370729609
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370729925
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370730637


More information about the i18n-dev mailing list