Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v2]
On Thu, 16 Nov 2023 18:09:20 GMT, Leonid Mesnik wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review feedback > > test/jdk/jdk/modules/incubator/DefaultImage.java line 113: > >> 111: PrintStream ps = new PrintStream(baos); >> 112: >> 113: ProcessBuilder pb = >> ProcessTools.createLimitedTestJavaProcessBuilder(opts); > > Shouldn't the test be marked as flagless? Fixed. thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/1#discussion_r1396154431
Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v2]
On Wed, 15 Nov 2023 02:39:56 GMT, Mandy Chung wrote: >> This PR includes test fixes for the following issues: >> >> 8319567: Update java/lang/invoke tests to support vm flags >> 8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java >> to accept vm flags >> 8319672: Several classloader tests ignore VM flags >> 8319676: A couple of jdk/modules/incubator/ tests ignore VM flags >> 8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as >> flagless >> >> It converts the test to use `ProcessTools::createTestJavaProcessBuilder` or >> `createNativeTestJavaProcessBuilder` so that the test will support VM flags >> passed to jtreg. A couple tests that ignore VM flags should use >> `ProcessTools::createLimtiedTestJavaProcessBuilder` and marks the test with >> `@requires vm.flagless`. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Changes requested by lmesnik (Reviewer). test/jdk/jdk/modules/incubator/DefaultImage.java line 113: > 111: PrintStream ps = new PrintStream(baos); > 112: > 113: ProcessBuilder pb = > ProcessTools.createLimitedTestJavaProcessBuilder(opts); Shouldn't the test be marked as flagless? - PR Review: https://git.openjdk.org/jdk/pull/1#pullrequestreview-1735130788 PR Review Comment: https://git.openjdk.org/jdk/pull/1#discussion_r1396146084
Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v2]
On Wed, 15 Nov 2023 02:39:56 GMT, Mandy Chung wrote: >> This PR includes test fixes for the following issues: >> >> 8319567: Update java/lang/invoke tests to support vm flags >> 8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java >> to accept vm flags >> 8319672: Several classloader tests ignore VM flags >> 8319676: A couple of jdk/modules/incubator/ tests ignore VM flags >> 8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as >> flagless >> >> It converts the test to use `ProcessTools::createTestJavaProcessBuilder` or >> `createNativeTestJavaProcessBuilder` so that the test will support VM flags >> passed to jtreg. A couple tests that ignore VM flags should use >> `ProcessTools::createLimtiedTestJavaProcessBuilder` and marks the test with >> `@requires vm.flagless`. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/1#pullrequestreview-1733180278
Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v2]
> This PR includes test fixes for the following issues: > > 8319567: Update java/lang/invoke tests to support vm flags > 8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java > to accept vm flags > 8319672: Several classloader tests ignore VM flags > 8319676: A couple of jdk/modules/incubator/ tests ignore VM flags > 8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as > flagless > > It converts the test to use `ProcessTools::createTestJavaProcessBuilder` or > `createNativeTestJavaProcessBuilder` so that the test will support VM flags > passed to jtreg. A couple tests that ignore VM flags should use > `ProcessTools::createLimtiedTestJavaProcessBuilder` and marks the test with > `@requires vm.flagless`. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/1/files - new: https://git.openjdk.org/jdk/pull/1/files/ee3643f1..e69f048b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=1=01 - incr: https://webrevs.openjdk.org/?repo=jdk=1=00-01 Stats: 6 lines in 3 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/1.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/1/head:pull/1 PR: https://git.openjdk.org/jdk/pull/1
Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v2]
On Wed, 15 Nov 2023 00:17:56 GMT, Jorn Vernee wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review feedback
>
> test/jdk/java/lang/ClassLoader/getResource/GetResource.java line 166:
>
>> 164: Map env = pb.environment();
>> 165: String value = env.remove("CLASSPATH");
>> 166:
>
> Looking into the implementation, it seems that the `CLASSPATH` environment
> variable is only cleared when `test.noclasspath` == true:
>
>
> if (noCP) {
> // clear CLASSPATH from the env
> pb.environment().remove("CLASSPATH");
> }
>
>
> (This seems to be contrary to the doc comment on
> `createTestJavaProcessBuilder` though, which says that _unless_
> `test.noclasspath` is true, the env. var will be cleared).
>
> Should this test be run with `-Dtest.noclasspath=true`?
I updated the test to remove `CLASSPATH` env var. We should file an issue for
the ProcessTools javadoc.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/1#discussion_r1393564565
