Disable DetectVMInstallationsJob for JUnit Plug-in Test launches

[trancexpress]

This change disables the job org.eclipse.jdt.internal.launching.DetectVMInstallationsJob, via the respective system property, for JUnit Plug-in Test launches.

This is done to reduce differences between the CI environment and launching from Eclipse, simplifying debugging of tests failing in the CI environment.

Fixes: #2215

[github-actions]

Test Results

   798 files  ±0     798 suites  ±0   57m 58s ⏱️ +42s
 3 787 tests ±0   3 733 ✅ ±0   54 💤 ±0  0 ❌ ±0 
11 127 runs  ±0  10 964 ✅ ±0  163 💤 ±0  0 ❌ ±0 

Results for commit a15127b. ± Comparison against base commit 518aa04.

♻️ This comment has been updated with latest results.

[trancexpress]

@iloveeclipse can you request a Copilot review? I'm curious about the feedback, but I'm not a committer and so I cannot touch the reviewers section.

[Copilot]

Pull request overview

This PR disables the DetectVMInstallationsJob for JUnit Plug-in Test launches by adding the system property -DDetectVMInstallationsJob.disabled=true to the VM arguments. This change aims to reduce differences between CI and Eclipse IDE test execution environments, making it easier to debug tests that fail in CI.

Changes:

• Added constants for the VM installation detection property and its disabled value
• Modified VM arguments initialization to automatically include the disabled property if not already present

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[HannesWell]

This absolutely makes sense to me and the change looks good.
I just a two nitpicks (if at all) below.

But since we are already in the RC2 phase, this should wait until the next dev-cycle (therefore I don't approve this now to avoid confusion).

[laeubi]

I don't think this change is good in its current form.

This is done to reduce differences between the CI environment and launching from Eclipse

The CI environment is detected by the DetectVMInstallationsJob itself and I strongly believe it should possibly detect junit launches as well instead of expect it to happen by the launcher.

Not every Plugin Launch is launch an Eclipse IDE nor does it requires JDT to be on the classpath at all. Also there might be other launcher scenarios where this is then not picked up at all. Even worse, if we bake in project specific properties into PDE the become de-facto API and its very hard to change.

Also how many of these properties that might influence a launch should we include? What projects are eligible for such deep integration with PDE?

Just as some ideas for the DetectVMInstallationsJob:

1. It could look for the program arguments, a usual launch include these where I see multiple that could be used e.g. testLoaderClass, application, testpluginname:
   Command-line arguments: -os linux -ws gtk -arch x86_64 -consoleLog -version 3 -port 36681 -testLoaderClass org.eclipse.jdt.internal.junit4.runner.JUnit4TestLoader -loaderpluginname org.eclipse.jdt.junit4.runtime -classNames org.eclipse.jdt.core.tests.builder.MultiReleaseTests -application org.eclipse.pde.junit.runtime.uitestapplication -product org.eclipse.sdk.ide -data ... -dev ... -os linux -ws gtk -arch x86_64 -consoleLog -testpluginname org.eclipse.jdt.core.tests.builder
2. Looking at the stacktrace of the current thread, it will include NonUIThreadTestApplication or UITestApplication (possibly not only when launched from PDE!)
3. Even the System properties already contain something like eclipse.application=org.eclipse.pde.junit.runtime.uitestapplication

So I think there are many better alternatives that do not need a change in PDE or other tooling...

[trancexpress]

The CI environment is detected by the DetectVMInstallationsJob itself and I strongly believe it should possibly detect junit launches as well instead of expect it to happen by the launcher.

DetectVMInstallationsJob checks if ENV variable CI=true is set. It doesn't magically detect the CI environment, it uses hints set from outside. If you prefer, we can set the ENV variable instead of the property.

Not every Plugin Launch is launch an Eclipse IDE nor does it requires JDT to be on the classpath at all. Also there might be other launcher scenarios where this is then not picked up at all. Even worse, if we bake in project specific properties into PDE the become de-facto API and its very hard to change.

That is true, not every test we will run even belongs to Eclipse. Some tests will not care at all about the Eclipse environment.

Also how many of these properties that might influence a launch should we include? What projects are eligible for such deep integration with PDE?

Also a fair point. Though if PDE is not knowledgeable of DetectVMInstallationsJob, then DetectVMInstallationsJob must become knowledgeable of what PDE is doing. So the problem is only inverted.

Just as some ideas for the DetectVMInstallationsJob:

3. Even the System properties already contain something like `eclipse.application=org.eclipse.pde.junit.runtime.uitestapplication`

This is probably the most simple suggestion to make use of...

So I think there are many better alternatives that do not need a change in PDE or other tooling...

Actually your comment has made me wonder if we can disable the job at all.

We need this change strictly for Eclipse plug-in tests... not the launch type, but actual tests for Eclipse code. There could be any number of other cases that we would be influencing, that don't run in a similar environment.

E.g. in our product we disable the job with a preference. In other products the job might be a welcome change to Eclipse and tests can already be relying on the job.

So we need to also know we are running a test for Eclipse code... I'll have to discuss this with my team. Maybe its more simple to make tests more robust, if they have an issue with multiple JVMs installed (e.g. eclipse-jdt/eclipse.jdt.core#4860).

Or maybe there is some other hint we can use to detect a test for Eclipse code... org.eclipse namespace is likely not the best idea. I'm not sure.

[laeubi]

DetectVMInstallationsJob checks if ENV variable CI=true is set. It doesn't magically detect the CI environment, it uses hints set from outside.

I know as I added support from it, still it is something that most (all?) CI server set by default. So the job can simply adapt to other common ways (e.g. properties already set by such launches).

Also a fair point. Though if PDE is not knowledgeable of DetectVMInstallationsJob, then DetectVMInstallationsJob must become knowledgeable of what PDE is doing. So the problem is only inverted.

The point is that DetectVMInstallationsJob might care about where it is running, but PDE should not care much about what code it executes in a test. As said, tomorrow there will be WhatEverElseJob and JustARandomOtherJob and soon we add more and more "exceptions"... so for me its clear to have such things implemented at the one responsible for the thing.

E.g. in our product we disable the job with a preference. In other products the job might be a welcome change to Eclipse and tests can already be relying on the job.

Last time we have discussed about the topic (when the CI check was introduced) there where similar issues raised. Because of this the job here explicitly check if the property is unset and then only takes action. Another reason for me to implement it there, as it is much more flexible and can take other things into account where PDE can only set it unconditionally.

So user have always the choice to actively set the property to false in their test to enable it.

So we need to also know we are running a test for Eclipse code... I'll have to discuss this with my team. Maybe its more simple to make tests more robust, if they have an issue with multiple JVMs installed

That's for sure, apart from that a rule or junit extension might be used as well to specifically perform such setup/cleanup things in test requiring it specifically.