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

[David Holmes]

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.

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

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