P2ResolverImpl: Don't add implicit test deps to eclipse-test-plugin

sratz · sratz · commit 7fa6c2b2f0b4 · 2025-09-17T09:08:23.000+01:00
In c6edec4 / https://bugs.eclipse.org/bugs/show_bug.cgi?id=443396 implicit dependencies were added to every 'eclipse-test-plugin' project to work around a very special situation: When building the eclipse.platform.ui repository the test harness is part of the same reactor build as well and the tests do not explicitly depend on it. The dependency resolution was then failing because, quote [1], - Tycho sees that org.eclipse.ui.ide.application is part of the reactor, and hence filters all other versions of this bundle from the external target platform content. - The afterProjectsRead dependency resolution doesn't find a dependency between the eclipse-test-plugin module and the org.eclipse.ui.ide.application module, so it doesn't add it to the target platform for the eclipse-test-plugin module. (Adding only the "needed" reactor modules instead of all previously built modules is something which doesn't give us any benefits today, but it is a prerequisite for allowing parallel builds in the future.) - The resolution of the test runtime fails. However, this had the undesired side-effect that these artificial dependencies were added to every other 'eclipse-test-plugin' project as well, which does not need it. This caused every 'eclipse-test-plugin' to list all the UI hardness dependencies in their dependencies tree, even ending up in things like generated SBOMs, which is incorrect. With recent improvements to Tycho, especially with #2092, the quoted issue above is not an issue anymore: Instead of filtering out shadowed units from the external target platform completely, it can now still access them if needed. Therefore, this change reverts implicit adding of UI harness dependencies. This results in a slight change of behavior when running tests in 'eclipse.platform.ui': - Before this change, the *project / reactor' version of the test harness was used when executing tests that did not themselves depend on the workbench. - After this change, the *external target* version of the test harness is used + some warnings are logged. - If this turns out to be a problem, Eclipse side has to adapt, e.g. by adding <extraRequirements> to target-platform-configuration in eclipse.platform.releng.aggregator/eclipse-platform-parent/pom.xml, restoring the previous Tycho behavior - but only at the place where it is actually needed. [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=443396#c8 Fixes #5349.
diff --git a/tycho-core/src/main/java/org/eclipse/tycho/core/resolver/TestPluginPackagingInstallableUnitProvider.java b/tycho-core/src/main/java/org/eclipse/tycho/core/resolver/TestPluginPackagingInstallableUnitProvider.java
diff --git a/tycho-core/src/main/java/org/eclipse/tycho/p2resolver/P2ResolverImpl.java b/tycho-core/src/main/java/org/eclipse/tycho/p2resolver/P2ResolverImpl.java
@@ -30,7 +30,6 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.Consumer;
 
 import org.apache.felix.resolver.util.CopyOnWriteSet;
 import org.eclipse.core.runtime.IProgressMonitor;
@@ -53,7 +52,6 @@
 import org.eclipse.tycho.IArtifactFacade;
 import org.eclipse.tycho.IDependencyMetadata.DependencyMetadataType;
 import org.eclipse.tycho.IllegalArtifactReferenceException;
-import org.eclipse.tycho.PackagingType;
 import org.eclipse.tycho.ReactorProject;
 import org.eclipse.tycho.ReactorProjectIdentities;
 import org.eclipse.tycho.TargetEnvironment;
@@ -109,12 +107,6 @@ public Map<TargetEnvironment, P2ResolutionResult> resolveTargetDependencies(Targ
             ReactorProject project) {
         P2TargetPlatform targetPlatform = getTargetFromContext(context);
 
-        // Adding extra deps for testswould better be performed as part of tycho-core resolver rather
-        // than in this P2ResolverImpl
-        if (project != null && PackagingType.TYPE_ECLIPSE_TEST_PLUGIN.equals(project.getPackaging())) {
-            addDependenciesForTests(additionalRequirements::add);
-        }
-
         // we need a linked hashmap to maintain iteration-order, some of the code relies on it!
         Map<TargetEnvironment, P2ResolutionResult> results = new LinkedHashMap<>();
         Set<IInstallableUnit> usedTargetPlatformUnits = new LinkedHashSet<>();
@@ -466,19 +458,6 @@ private static void addArtifactFile(DefaultP2ResolutionResult result, IInstallab
         // throw new IllegalArgumentException();
     }
 
-    public static void addDependenciesForTests(Consumer<IRequirement> requirementsConsumer) {
-        /*
-         * In case the test harness bundles (cf. TestMojo.getTestDependencies()) are part of the
-         * reactor, the dependency resolution needs to identify them as a dependency of
-         * eclipse-test-plugin modules. Otherwise they will be missing in the final target platform
-         * of the module - they would be filtered from the external target platform, and not added
-         * from the reactor - and hence the test runtime resolution would fail (see bug 443396).
-         */
-        requirementsConsumer.accept(optionalGreedyRequirementTo("org.eclipse.equinox.launcher"));
-        requirementsConsumer.accept(optionalGreedyRequirementTo("org.eclipse.core.runtime"));
-        requirementsConsumer.accept(optionalGreedyRequirementTo("org.eclipse.ui.ide.application"));
-    }
-
     @Override
     public void setPomDependencies(PomDependencies pomDependencies) {
         this.pomDependencies = pomDependencies;
diff --git a/tycho-core/src/test/java/org/eclipse/tycho/p2resolver/P2ResolverTest.java b/tycho-core/src/test/java/org/eclipse/tycho/p2resolver/P2ResolverTest.java
@@ -24,6 +24,7 @@
 import static org.eclipse.tycho.test.util.InstallableUnitMatchers.unitWithId;
 import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.matchesRegex;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThrows;
@@ -198,18 +199,33 @@ public void testEclipseRepository() throws Exception {
     }
 
     @Test
-    public void testEclipseTestPluginAutomaticallyDependsOnTestHarnesses() throws Exception {
-        tpConfig.addP2Repository(resourceFile("repositories/e342").toURI());
-        addContextProject(resourceFile("resolver/bundle.uitestharness"), TYPE_ECLIPSE_PLUGIN);
+    public void testEclipseTestPluginTestHarnessesCanBeResolvedEvenIfHarnessIsPartOfReactorItself() throws Exception {
+        // https://github.com/eclipse-tycho/tycho/issues/5349
+
+        // Add a an external target platform that contains the test harness bundle 'org.eclipse.ui.ide.application' (stub)
+        tpConfig.addP2Repository(resourceFile("repositories/testHarnessExternalStub").toURI());
+
+        // Make the test harness itself part of the reactor as well by adding a project stub for 'org.eclipse.ui.ide.application'
+        this.reactorProjects.add(createReactorProject(resourceFile("resolver/bundle.uitestharness"),
+                TYPE_ECLIPSE_PLUGIN, "org.eclipse.ui.ide.application"));
 
         projectToResolve = createReactorProject(resourceFile("resolver/bundle.nodeps"), TYPE_ECLIPSE_TEST_PLUGIN,
                 "bundle.nodeps");
 
+        // Simulate surefire's adding of the additional test harness 'org.eclipse.ui.ide.application' dependency
+        impl.addAdditionalBundleDependency("org.eclipse.ui.ide.application");
+
         result = singleEnv(impl.resolveTargetDependencies(getTargetPlatform(), projectToResolve));
 
         assertEquals(2, result.getArtifacts().size());
-        assertThat(result.getArtifacts(), hasItem(withId("bundle.nodeps")));
-        assertThat(result.getArtifacts(), hasItem(withId("org.eclipse.ui.ide.application")));
+        assertThat(result.getArtifacts(), hasItem(withIdAndVersion("bundle.nodeps", "1.0.0.qualifier")));
+
+        // The test harness is satisfied from the shadowed bundle of the external target platform
+        assertThat(result.getArtifacts(), hasItem(
+                withIdAndVersion("org.eclipse.ui.ide.application", "1.0.0.bundleStubFromExternalTargetPlatform")));
+        // Since the bundle is also part of the reactor, we expect a warning message
+        logVerifier.expectWarning(matchesRegex(
+                "(?s).*org\\.eclipse\\.ui\\.ide\\.application 1\\.0\\.0\\.bundleStubFromExternalTargetPlatform.*shadowed.*"));
     }
 
     @Test
@@ -516,17 +532,17 @@ private static P2ResolutionResult.Entry getClassifiedArtifact(P2ResolutionResult
         return selectedEntry;
     }
 
-    static Matcher<Entry> withId(final String id) {
+    static Matcher<Entry> withIdAndVersion(final String id, final String version) {
         return new TypeSafeMatcher<>() {
 
             @Override
             protected boolean matchesSafely(Entry entry) {
-                return id.equals(entry.getId());
+                return id.equals(entry.getId()) && version.equals(entry.getVersion());
             }
 
             @Override
             public void describeTo(Description description) {
-                description.appendText("an artifact with ID " + id);
+                description.appendText("an artifact with ID " + id + " and version " + version);
             }
         };
     }
diff --git a/tycho-core/src/test/resources/repositories/testHarnessExternalStub/artifacts.xml b/tycho-core/src/test/resources/repositories/testHarnessExternalStub/artifacts.xml
@@ -0,0 +1,21 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<?artifactRepository version='1.1.0'?>
+<repository name='e342' type='org.eclipse.equinox.p2.artifact.repository.simpleRepository' version='1'>
+  <properties size='2'>
+    <property name='p2.timestamp' value='1308584189023'/>
+    <property name='p2.compressed' value='false'/>
+  </properties>
+  <mappings size='3'>
+    <rule filter='(&amp; (classifier=osgi.bundle))' output='${repoUrl}/plugins/${id}_${version}.jar'/>
+    <rule filter='(&amp; (classifier=binary))' output='${repoUrl}/binary/${id}_${version}'/>
+    <rule filter='(&amp; (classifier=org.eclipse.update.feature))' output='${repoUrl}/features/${id}_${version}.jar'/>
+  </mappings>
+  <artifacts size='1'>
+    <artifact classifier='osgi.bundle' id='org.eclipse.ui.ide.application' version='1.0.0.bundleStubFromExternalTargetPlatform'>
+      <properties size='3'>
+        <property name='artifact.size' value='394'/>
+        <property name='download.size' value='394'/>
+      </properties>
+    </artifact>
+  </artifacts>
+</repository>
diff --git a/tycho-core/src/test/resources/repositories/testHarnessExternalStub/content.xml b/tycho-core/src/test/resources/repositories/testHarnessExternalStub/content.xml
@@ -0,0 +1,37 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<?metadataRepository version='1.1.0'?>
+<repository name='e342' type='org.eclipse.equinox.internal.p2.metadata.repository.LocalMetadataRepository' version='1'>
+  <properties size='2'>
+    <property name='p2.timestamp' value='1308584189029'/>
+    <property name='p2.compressed' value='false'/>
+  </properties>
+  <units size='1'>
+    <unit id='org.eclipse.ui.ide.application' version='1.0.0.bundleStubFromExternalTargetPlatform'>
+      <update id='org.eclipse.core.runtime' range='[0.0.0,1.0.0.bundleStubFromExternalTargetPlatform)' severity='0'/>
+      <properties size='5'>
+        <property name='df_LT.providerName' value='Eclipse.org'/>
+        <property name='df_LT.pluginName' value='UI Test Harness Stub'/>
+        <property name='org.eclipse.equinox.p2.name' value='%pluginName'/>
+        <property name='org.eclipse.equinox.p2.provider' value='%providerName'/>
+        <property name='org.eclipse.equinox.p2.bundle.localization' value='plugin'/>
+      </properties>
+      <provides size='4'>
+        <provided namespace='org.eclipse.equinox.p2.iu' name='org.eclipse.ui.ide.application' version='1.0.0.bundleStubFromExternalTargetPlatform'/>
+        <provided namespace='osgi.bundle' name='org.eclipse.ui.ide.application' version='1.0.0.bundleStubFromExternalTargetPlatform'/>
+        <provided namespace='org.eclipse.equinox.p2.eclipse.type' name='bundle' version='1.0.0'/>
+        <provided namespace='org.eclipse.equinox.p2.localization' name='df_LT' version='1.0.0'/>
+      </provides>
+      <artifacts size='1'>
+        <artifact classifier='osgi.bundle' id='org.eclipse.ui.ide.application' version='1.0.0.bundleStubFromExternalTargetPlatform'/>
+      </artifacts>
+      <touchpoint id='org.eclipse.equinox.p2.osgi' version='1.0.0'/>
+      <touchpointData size='1'>
+        <instructions size='1'>
+          <instruction key='manifest'>
+            Manifest-Version: 1.0&#xA;Bundle-ManifestVersion: 2&#xA;Bundle-Name: Stub for the bundle containing the non-UI test harness&#xA;Bundle-SymbolicName: org.eclipse.osgi&#xA;Bundle-Version: 1.0.0.bundleStubFromExternalTargetPlatform&#xA;
+          </instruction>
+        </instructions>
+      </touchpointData>
+    </unit>
+  </units>
+</repository>
diff --git a/tycho-core/src/test/resources/repositories/testHarnessExternalStub/plugins/org.eclipse.ui.ide.application_1.0.0.bundleStubFromExternalTargetPlatform.jar b/tycho-core/src/test/resources/repositories/testHarnessExternalStub/plugins/org.eclipse.ui.ide.application_1.0.0.bundleStubFromExternalTargetPlatform.jar
diff --git a/tycho-core/src/test/resources/resolver/bundle.uitestharness/META-INF/MANIFEST.MF b/tycho-core/src/test/resources/resolver/bundle.uitestharness/META-INF/MANIFEST.MF
@@ -2,4 +2,4 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Stub for the bundle containing the UI test harness
 Bundle-SymbolicName: org.eclipse.ui.ide.application
-Bundle-Version: 1.0.0.qualifier
+Bundle-Version: 9999.0.0.qualifier
diff --git a/tycho-its/projects/sbom/example.testFragment/META-INF/MANIFEST.MF b/tycho-its/projects/sbom/example.testFragment/META-INF/MANIFEST.MF
diff --git a/tycho-its/projects/sbom/example.testFragment/build.properties b/tycho-its/projects/sbom/example.testFragment/build.properties
diff --git a/tycho-its/projects/sbom/example.testStandAlone/META-INF/MANIFEST.MF b/tycho-its/projects/sbom/example.testStandAlone/META-INF/MANIFEST.MF
@@ -1,8 +1,8 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
-Bundle-Name: Test-Fragment with SBOM
+Bundle-Name: Test-Bundle with SBOM
 Bundle-SymbolicName: example.test2
 Bundle-Version: 1.0.0.qualifier
-Fragment-Host: example.plugin
+Require-Bundle: example.plugin
 Automatic-Module-Name: example.test2
 Bundle-RequiredExecutionEnvironment: JavaSE-17
diff --git a/tycho-its/projects/sbom/example.testStandAlone/build.properties b/tycho-its/projects/sbom/example.testStandAlone/build.properties
diff --git a/tycho-its/projects/sbom/pom.xml b/tycho-its/projects/sbom/pom.xml
@@ -13,8 +13,8 @@
 	<modules>
 		<module>example.feature</module>
 		<module>example.plugin</module>
-		<module>example.test1</module>
-		<module>example.test2</module>
+		<module>example.testFragment</module>
+		<module>example.testStandAlone</module>
 		<module>product</module>
 		<module>repository</module>
 		<module>target-platform</module>
diff --git a/tycho-its/projects/surefire.bundleStart/implicit/ds.test/META-INF/MANIFEST.MF b/tycho-its/projects/surefire.bundleStart/implicit/ds.test/META-INF/MANIFEST.MF
@@ -9,3 +9,4 @@ Require-Bundle: org.junit;bundle-version="4.5.0",
  system.bundle
 Bundle-ActivationPolicy: lazy
 Bundle-Activator: bundle.Activator
+Require-Capability: osgi.extender; filter:="(&(osgi.extender=osgi.component)(version>=1.2)(!(version>=2.0)))"
diff --git a/tycho-its/src/test/java/org/eclipse/tycho/test/reactor/BomCreationTest.java b/tycho-its/src/test/java/org/eclipse/tycho/test/reactor/BomCreationTest.java
@@ -60,6 +60,46 @@ public void verifyBundle() throws Exception {
 		assertEquals(dependencies.size(), 7);
 	}
 
+	@Test
+	public void verifyTestFragment() throws Exception {
+		String bomPath = getBomPath("example.testFragment");
+		verifier.verifyFilePresent(bomPath);
+
+		Bom bom = getBom(bomPath);
+		List<Dependency> dependencies = bom.getDependencies();
+
+		// test fragment host bundle dependency
+		Dependency testBundle = getDependency(dependencies,
+				"pkg:maven/tycho-demo/example.test1@1.0.0-SNAPSHOT?type=eclipse-test-plugin");
+		verifyDependency(testBundle,
+				"pkg:p2/example.plugin@1.0.0.today?classifier=osgi.bundle&location=https://www.example.p2.repo/");
+		assertEquals(testBundle.getDependencies().size(), 1);
+
+		verifyBundleDependencies(dependencies);
+		verifyCommonDependencies(dependencies);
+		assertEquals(dependencies.size(), 8);
+	}
+
+	@Test
+	public void verifyTestStandAlone() throws Exception {
+		String bomPath = getBomPath("example.testStandAlone");
+		verifier.verifyFilePresent(bomPath);
+
+		Bom bom = getBom(bomPath);
+		List<Dependency> dependencies = bom.getDependencies();
+
+		// test bundle dependency
+		Dependency testBundle = getDependency(dependencies,
+				"pkg:maven/tycho-demo/example.test2@1.0.0-SNAPSHOT?type=eclipse-test-plugin");
+		verifyDependency(testBundle,
+				"pkg:p2/example.plugin@1.0.0.today?classifier=osgi.bundle&location=https://www.example.p2.repo/");
+		assertEquals(testBundle.getDependencies().size(), 1);
+
+		verifyBundleDependencies(dependencies);
+		verifyCommonDependencies(dependencies);
+		assertEquals(dependencies.size(), 8);
+	}
+
 	@Test
 	public void verifyFeature() throws Exception {
 		String bomPath = getBomPath("example.feature");
diff --git a/tycho-surefire/tycho-surefire-plugin/src/main/java/org/eclipse/tycho/surefire/AbstractEclipseTestMojo.java b/tycho-surefire/tycho-surefire-plugin/src/main/java/org/eclipse/tycho/surefire/AbstractEclipseTestMojo.java
@@ -890,7 +890,9 @@ protected List<ArtifactKey> getExtraDependencies() {
     private List<ArtifactKey> getTestDependencies() {
         ArrayList<ArtifactKey> result = new ArrayList<>();
 
-        // see also P2ResolverImpl.addDependenciesForTests()
+        // The test harness dependencies must be satisfiable from the external target platform
+        // See also https://github.com/eclipse-tycho/tycho/issues/5349 for a special situation
+        // where the harness bundles are part of the same reactor (e.g. when building eclipse.platform.ui)
         result.add(newBundleDependency("org.eclipse.osgi"));
         result.add(newBundleDependency(DefaultEquinoxInstallationDescription.EQUINOX_LAUNCHER));
         if (useUIHarness) {
