RFR: 8356102: TestJcmdOutput, JcmdWithNMTDisabled and DumpSharedDictionary hs/tier1 tests fail on static-jdk

[Jiangli Zhou]

On Fri, 9 May 2025 22:50:48 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>>> This looks rather hackish to me - individual tests should not need to know about how to make jdkToolFinder work. Can this be hidden away in `ProcessTools.createLimitedTestJavaProcessBuilder` and other `ProcessTools` code as needed?
>> 
>> I agree, this feels like the wrong place for this kind of logic. The tests should not have to care whether they run on a static JDK.
>
>> > This looks rather hackish to me - individual tests should not need to know about how to make jdkToolFinder work. Can this be hidden away in `ProcessTools.createLimitedTestJavaProcessBuilder` and other `ProcessTools` code as needed?
>> 
>> I agree, this feels like the wrong place for this kind of logic. The tests should not have to care whether they run on a static JDK.
> 
> @dholmes-ora @tstuefe Thanks for looking at this. Setting up the `-Dtest.jdk` and `-Dcompile.jdk` in `ProcessTools` seems reasonable on the first thought. I went ahead and changed to move setting `-Dtest.jdk` and `-Dcompile.jdk` into `ProcessTools.createJavaProcessBuilder()`, in my local branch. All three tests pass on static-jdk in my local testing, without explicitly setting `-Dtest.jdk` and `-Dcompile.jdk` in test code when launching child process. However, thinking more on this I realize that some tests may want to set to a different `test.jdk` or `compile.jdk` intentionally, and we need to be careful to not cause any unexpected side-effects. Of course, if we put `-Dtest.jdk=` and `-Dcompile.jdk=` command-line options immediately after the launcher (that's the case with my new local change), the settings from test can override the default settings in `ProcessTools.createJavaProcessBuilder()`. There would be no unexpected behavior. Any other thoughts, in case I missed something?

> @jianglizhou FWIW I couldn't find any test that explicitly sets `-Dcompile.jdk= ...` it seems these are things expected to only be set via jtreg for a test component to use.
> 
> The problem we face here is that tests that exec a VM programmatically do not pass through these jtreg settings (reasonably so as the individual tests need not have any knowledge of their existence). We have defined `createJavaProcessBuilder` to pass through flags passed via jtreg (as opposed to `createLimitedJavaProcessBuilder`) so I think it is quite reasonable to include these properties as well.
> 
> However, the tests at hand use `createLimitedJavaProcessbuilder` which intentionally does not pass on any flags, so we are muddying the waters if we decide that it should pass on some flags after all.

Thanks for the thoughts, @dholmes-ora. I didn't notice the difference between `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder` before reading your comments above. Thanks for pointing out to those. Both methods calls the underlying `createJavaProcessBuilder`, and `createTestJavaProcessBuilder` includes additional arguments from `test.vm.opts` and `test.java.opts` from jtreg environment (comparing to `createLimitedTestJavaProcessBuilder`). The underlying `createJavaProcessBuilder` already adds `-Dtest.thread.factory=...` to the child process' command-line argument if the system property is defined. It seems also ok to add `-Dcompile.jdk=...` then. `-Dcompile.jdk=...` doesn't affect the VM and Java options, so it seems fine for `createLimitedTestJavaProcessBuilder` to also include it.

In my local change, I included `-Dtest.jdk= ...` as well. It's probably better to leave `-Dtest.jdk= ...` out. I'll update my PR based on the discussion.

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

PR Comment: https://git.openjdk.org/jdk/pull/25018#issuecomment-2870784488