search

eclipse-jdt / eclipse.jdt.debug

Eclipse Public License 2.0
16 stars 46 forks source link

Don't try to update compliance if JDT model tests are running #458

Closed iloveeclipse closed 1 month ago

iloveeclipse commented 2 months ago

The tests need predictable defaults, not based on current JVM. Check if TEST_LIB variable is known and don't update in that case. (TEST_LIB classpath variable is contributed by org.eclipse.jdt.core.tests.model)

Fixes https://github.com/eclipse-jdt/eclipse.jdt.core/issues/84

iloveeclipse commented 1 month ago

I will prefer it handled in initializeVMs where we should set updateCompliance as false for testing condition.

Done, please check. To test if the patch works, run JavaSearchMultipleProjectsTests - testMethodOccurences() will fail without the patch (at least it does so consistently for me on this PR: https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2551).

stephan-herrmann commented 1 month ago

A related property is -Djdt.default.test.compliance=1.8 it is set from test.xml and pom.xml.

iloveeclipse commented 1 month ago

A related property is -Djdt.default.test.compliance=1.8 it is set from test.xml and pom.xml.

As I've mentioned on the ticket, I'm not in favour of using system property here, because it would always need a manual update of every test launch configuration in the IDE. This would also only help during automated builds but not help contributors running tests locally.

stephan-herrmann commented 1 month ago

A related property is -Djdt.default.test.compliance=1.8 it is set from test.xml and pom.xml.

As I've mentioned on the ticket, I'm not in favour of using system property here, because it would always need a manual update of every test launch configuration in the IDE. This would also only help during automated builds but not help contributors running tests locally.

The property is already there, and at some point at least it was necessary for successful test runs (and one test even explicitly checks if it is set).

But perhaps we're lucky and after your change we can actually remove this property :)

iloveeclipse commented 1 month ago

The property is already there

It is not there in the IDE!