gnodet-bot commented on code in PR #13774:
URL: https://github.com/apache/maven/pull/13774#discussion_r4435792293
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleDependencyResolver.java:
##########
@@ -259,6 +259,17 @@ private Set<Artifact> getDependencies(
resolutionFilter =
AndDependencyFilter.newInstance(resolutionFilter, new
ReactorDependencyFilter(projectArtifacts));
+ if (RepositoryUtils.isTestSkip(session, project)) {
+ DependencyFilter testArtifactFilter = (node, parents) -> {
+ if (node.getDependency() != null) {
+ return
!RepositoryUtils.isTestArtifact(node.getDependency().getArtifact());
Review Comment:
⚠️ **Scope-blind filter** — The filter excludes test-jar artifacts
regardless of their dependency scope. A dependency declared as
`<type>test-jar</type><scope>compile</scope>` (used for shared test utilities
published as regular dependencies) would be silently dropped when
`maven.test.skip` is set, breaking the build in a different way.
The filter should restrict to `test`-scoped artifacts only:
```suggestion
if (RepositoryUtils.isTestSkip(session, project)) {
DependencyFilter testArtifactFilter = (node, parents) -> {
if (node.getDependency() != null) {
org.eclipse.aether.graph.Dependency dep =
node.getDependency();
return
!(RepositoryUtils.isTestArtifact(dep.getArtifact())
&& "test".equals(dep.getScope()));
}
return true;
};
collectionFilter =
AndDependencyFilter.newInstance(collectionFilter, testArtifactFilter);
resolutionFilter =
AndDependencyFilter.newInstance(resolutionFilter, testArtifactFilter);
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/RepositoryUtils.java:
##########
@@ -396,4 +398,81 @@ private static boolean repositoryEquals(RemoteRepository
r1, RemoteRepository r2
&& policyEquals(r1.getPolicy(false), r2.getPolicy(false))
&& policyEquals(r1.getPolicy(true), r2.getPolicy(true));
}
+
+ /**
+ * Determines whether the specified artifact refers to test classes (e.g.
test-jar).
+ *
+ * @param artifact The artifact to check, may be {@code null}.
+ * @return {@code true} if the artifact refers to test classes, {@code
false} otherwise.
+ */
+ public static boolean isTestArtifact(Artifact artifact) {
+ return artifact != null
+ &&
("test-jar".equals(artifact.getProperty(ArtifactProperties.TYPE, ""))
+ || ("jar".equals(artifact.getExtension()) &&
"tests".equals(artifact.getClassifier())));
+ }
+
+ /**
+ * Determines whether the specified artifact refers to test classes (e.g.
test-jar).
+ *
+ * @param artifact The artifact to check, may be {@code null}.
+ * @return {@code true} if the artifact refers to test classes, {@code
false} otherwise.
+ */
+ public static boolean isTestArtifact(org.apache.maven.artifact.Artifact
artifact) {
+ return artifact != null
+ && ("test-jar".equals(artifact.getType())
+ || ("jar".equals(artifact.getType()) &&
"tests".equals(artifact.getClassifier())));
+ }
Review Comment:
🚫 **Dead code** — The `isTestArtifact(org.apache.maven.artifact.Artifact)`
overload has no callers. Both filter lambdas in this PR receive
`org.eclipse.aether.artifact.Artifact` via
`DependencyNode.getDependency().getArtifact()`, and the `ReactorReader`
refactor delegates to the Aether overload above. There is no existing call site
for this legacy-type overload either. Remove it to avoid dead public API.
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleDependencyResolverTest.java:
##########
@@ -85,7 +87,7 @@ void testCachedReactorProjectDependencies() throws Exception {
* (lib) has a {@code null} artifact file — i.e. the sibling has not yet
packaged its jar.
*
* <p>This guards the null-file code path introduced by the fix in
- * <a href="https://github.com/apache/maven/pull/13728";>#13728</a>. The
concurrent-builder
+ * <a href=https://github.com/apache/maven/pull/13728>#13728</a>. The
concurrent-builder
Review Comment:
🔍 **Unintended change** — The existing `href` attribute lost its quotes
(`href="..."` → `href=...`). While technically valid HTML5, this is
inconsistent with the rest of the codebase and is an unrelated modification to
pre-existing code.
```suggestion
* <a href="https://github.com/apache/maven/pull/13728";>#13728</a>. The
concurrent-builder
```
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -78,10 +79,20 @@ public DependencyResolutionResult
resolve(DependencyResolutionRequest request)
final DefaultDependencyResolutionResult result = new
DefaultDependencyResolutionResult();
final MavenProject project = request.getMavenProject();
- final DependencyFilter filter = request.getResolutionFilter();
+ DependencyFilter filter = request.getResolutionFilter();
RepositorySystemSession session = request.getRepositorySession();
ArtifactTypeRegistry stereotypes = session.getArtifactTypeRegistry();
+ if (RepositoryUtils.isTestSkip(session, project)) {
+ DependencyFilter testArtifactFilter = (node, parents) -> {
+ if (node.getDependency() != null) {
+ return
!RepositoryUtils.isTestArtifact(node.getDependency().getArtifact());
+ }
+ return true;
+ };
+ filter = AndDependencyFilter.newInstance(filter,
testArtifactFilter);
Review Comment:
⚠️ **Same scope-blind filter** — same issue as in
`LifecycleDependencyResolver`: the filter drops all test-jar artifacts
regardless of scope. Restrict to `test`-scoped dependencies:
```suggestion
if (RepositoryUtils.isTestSkip(session, project)) {
DependencyFilter testArtifactFilter = (node, parents) -> {
if (node.getDependency() != null) {
org.eclipse.aether.graph.Dependency dep =
node.getDependency();
return
!(RepositoryUtils.isTestArtifact(dep.getArtifact())
&& "test".equals(dep.getScope()));
}
return true;
};
filter = AndDependencyFilter.newInstance(filter,
testArtifactFilter);
}
```
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleDependencyResolverTest.java:
##########
@@ -129,4 +131,44 @@ void
testReactorDependencyWithNullArtifactFileDoesNotThrow() throws Exception {
.getFile(),
"resolved lib artifact file should remain null when lib has
not yet packaged its jar");
}
+
+ @Test
+ void testTestJarNotResolvedWhenTestSkipped() throws Exception {
+ File pom = new
File("src/test/projects/lifecycle-dependency-resolver-test-jar/pom.xml");
+ Properties userProps = new Properties();
+ userProps.setProperty("maven.test.skip", "true");
+
+ MavenSession session = createMavenSession(pom, new Properties(), true);
+ session.getUserProperties().putAll(userProps);
+
+ MavenProject common = session.getProjects().get(1);
+ MavenProject core = session.getProjects().get(2);
+
+ common.getArtifact().setFile(new File("common.jar"));
+
+ Collection<String> scopesToResolve = Collections.singletonList("test");
+
+ resolver.resolveProjectDependencies(core, null, scopesToResolve,
session, false, Collections.emptySet());
+
+
assertNull(core.getArtifactMap().get("org.apache.maven.its.mng8074:mng8074-common:test-jar:tests"));
+ }
+
+ @Test
+ void testTestJarResolutionFailsWhenTestNotSkipped() throws Exception {
+ File pom = new
File("src/test/projects/lifecycle-dependency-resolver-test-jar/pom.xml");
+
+ MavenSession session = createMavenSession(pom, new Properties(), true);
+
+ MavenProject common = session.getProjects().get(1);
+ MavenProject core = session.getProjects().get(2);
+
+ common.getArtifact().setFile(new File("common.jar"));
+
+ Collection<String> scopesToResolve = Collections.singletonList("test");
+
+ assertThrows(
+ LifecycleExecutionException.class,
+ () -> resolver.resolveProjectDependencies(
+ core, null, scopesToResolve, session, false,
Collections.emptySet()));
+ }
Review Comment:
⚠️ **Test doesn't exercise the fix** —
`testTestJarResolutionFailsWhenTestNotSkipped` passes identically before and
after this PR. It sets no `maven.test.skip` property, so `isTestSkip()` returns
`false`, the new filter is never applied, and the test simply confirms the
pre-existing behavior (resolution fails when a test-jar isn't available in the
reactor).
Replace this with a meaningful counterpart: verify that the test-jar **is**
resolved (and present in the artifact map) when `maven.test.skip` is _not_ set
and the test-jar file exists. That would establish the expected baseline and
make the two tests a proper before/after pair.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
