On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth <lkori...@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
