gnodet-bot commented on code in PR #13774:
URL: https://github.com/apache/maven/pull/13774#discussion_r4436187140
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -186,14 +187,100 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
@Override
public List<Node> flatten(@Nonnull Session s, @Nonnull Node node,
@Nullable PathScope scope)
throws DependencyResolverException {
+ return flatten(s, node, scope, null);
+ }
+
+ private List<Node> flatten(
+ @Nonnull Session s, @Nonnull Node node, @Nullable PathScope scope,
@Nullable Project project)
+ throws DependencyResolverException {
InternalSession session = InternalSession.from(s);
DependencyNode root = cast(AbstractNode.class, node,
"node").getDependencyNode();
List<DependencyNode> dependencies = session.getRepositorySystem()
- .flattenDependencyNodes(session.getSession(), root,
getScopeDependencyFilter(scope));
+ .flattenDependencyNodes(session.getSession(), root,
getDependencyFilter(session, scope, project));
dependencies.remove(root);
return map(dependencies, session::getNode);
}
+ private static DependencyFilter getDependencyFilter(InternalSession
session, PathScope scope, Project project) {
+ DependencyFilter scopeFilter = getScopeDependencyFilter(scope);
+ if (isTestSkip(session, project)) {
+ DependencyFilter testFilter = (node, parents) -> {
+ org.eclipse.aether.graph.Dependency dep = node.getDependency();
+ if (dep != null) {
+ return !(isTestArtifact(dep.getArtifact()) &&
"test".equals(dep.getScope()));
+ }
+ return true;
+ };
+ if (scopeFilter != null) {
+ return (node, parents) -> scopeFilter.accept(node, parents) &&
testFilter.accept(node, parents);
+ }
+ return testFilter;
+ }
+ return scopeFilter;
+ }
+
+ static boolean isTestArtifact(org.eclipse.aether.artifact.Artifact
artifact) {
+ return artifact != null
+ &&
("test-jar".equals(artifact.getProperty(ArtifactProperties.TYPE, ""))
+ || ("jar".equals(artifact.getExtension()) &&
"tests".equals(artifact.getClassifier())));
+ }
+
+ private static boolean isTestSkip(InternalSession session, Project
project) {
+ if (session != null) {
+ try {
+ if (session.getUserProperties() != null) {
+ String val =
session.getUserProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ if (session.getSystemProperties() != null) {
+ String val =
session.getSystemProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ } catch (Exception ignored) {
+ // Ignore if properties cannot be accessed
+ }
+ try {
+ RepositorySystemSession rSession = session.getSession();
+ if (rSession != null) {
+ Object val =
rSession.getConfigProperties().get("maven.test.skip");
+ if (val != null) {
+ String s = String.valueOf(val);
+ return Boolean.parseBoolean(s) || s.isEmpty();
+ }
+ val = rSession.getUserProperties().get("maven.test.skip");
+ if (val != null) {
+ String s = String.valueOf(val);
+ return Boolean.parseBoolean(s) || s.isEmpty();
+ }
+ val =
rSession.getSystemProperties().get("maven.test.skip");
+ if (val != null) {
+ String s = String.valueOf(val);
+ return Boolean.parseBoolean(s) || s.isEmpty();
+ }
+ }
+ } catch (Exception ignored) {
Review Comment:
⚠️ **Silent exception swallowing** — same issue as the first `catch` block:
failures from `rSession.getConfigProperties()`, `getUserProperties()`, or
`getSystemProperties()` are silently ignored. Log at `DEBUG` level at minimum.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/TestSkipUtils.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.internal;
+
+import org.apache.maven.RepositoryUtils;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.graph.DependencyFilter;
+
+/**
+ * Utility methods for checking test skip policy (e.g. {@code
-Dmaven.test.skip}).
+ */
+public final class TestSkipUtils {
+
+ private TestSkipUtils() {}
+
+ /**
+ * Determines whether test execution and compilation are skipped (e.g. via
{@code -Dmaven.test.skip}).
+ *
+ * @param session The Maven session, may be {@code null}.
+ * @param project The Maven project, may be {@code null}.
+ * @return {@code true} if tests are skipped, {@code false} otherwise.
+ */
+ public static boolean isTestSkip(MavenSession session, MavenProject
project) {
+ if (session != null) {
+ if (session.getUserProperties() != null) {
+ String val =
session.getUserProperties().getProperty("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ if (session.getSystemProperties() != null) {
+ String val =
session.getSystemProperties().getProperty("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ return isTestSkip(session.getRepositorySession(), project);
+ }
+ return isTestSkip((RepositorySystemSession) null, project);
+ }
+
+ /**
+ * Determines whether test execution and compilation are skipped (e.g. via
{@code -Dmaven.test.skip}).
+ *
+ * @param session The repository system session, may be {@code null}.
+ * @param project The Maven project, may be {@code null}.
+ * @return {@code true} if tests are skipped, {@code false} otherwise.
+ */
+ public static boolean isTestSkip(RepositorySystemSession session,
MavenProject project) {
+ if (session != null) {
+ Object val = session.getConfigProperties().get("maven.test.skip");
Review Comment:
⚠️ **Wrong property precedence** — Maven's precedence order is
`userProperties` > `systemProperties` > `configProperties`. Here
`configProperties` is checked first, which means a config-level setting cannot
be overridden by the user at the command line (e.g. `-Dmaven.test.skip=false`
in user properties would be ignored if config sets it to `true`).
Fix: check `userProperties` before `configProperties`:
```suggestion
Object val = session.getUserProperties().get("maven.test.skip");
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -186,14 +187,100 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
@Override
public List<Node> flatten(@Nonnull Session s, @Nonnull Node node,
@Nullable PathScope scope)
throws DependencyResolverException {
+ return flatten(s, node, scope, null);
+ }
+
+ private List<Node> flatten(
+ @Nonnull Session s, @Nonnull Node node, @Nullable PathScope scope,
@Nullable Project project)
+ throws DependencyResolverException {
InternalSession session = InternalSession.from(s);
DependencyNode root = cast(AbstractNode.class, node,
"node").getDependencyNode();
List<DependencyNode> dependencies = session.getRepositorySystem()
- .flattenDependencyNodes(session.getSession(), root,
getScopeDependencyFilter(scope));
+ .flattenDependencyNodes(session.getSession(), root,
getDependencyFilter(session, scope, project));
dependencies.remove(root);
return map(dependencies, session::getNode);
}
+ private static DependencyFilter getDependencyFilter(InternalSession
session, PathScope scope, Project project) {
+ DependencyFilter scopeFilter = getScopeDependencyFilter(scope);
+ if (isTestSkip(session, project)) {
+ DependencyFilter testFilter = (node, parents) -> {
+ org.eclipse.aether.graph.Dependency dep = node.getDependency();
+ if (dep != null) {
+ return !(isTestArtifact(dep.getArtifact()) &&
"test".equals(dep.getScope()));
+ }
+ return true;
+ };
+ if (scopeFilter != null) {
+ return (node, parents) -> scopeFilter.accept(node, parents) &&
testFilter.accept(node, parents);
+ }
+ return testFilter;
+ }
+ return scopeFilter;
+ }
+
+ static boolean isTestArtifact(org.eclipse.aether.artifact.Artifact
artifact) {
+ return artifact != null
+ &&
("test-jar".equals(artifact.getProperty(ArtifactProperties.TYPE, ""))
+ || ("jar".equals(artifact.getExtension()) &&
"tests".equals(artifact.getClassifier())));
+ }
+
+ private static boolean isTestSkip(InternalSession session, Project
project) {
+ if (session != null) {
+ try {
+ if (session.getUserProperties() != null) {
+ String val =
session.getUserProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ if (session.getSystemProperties() != null) {
+ String val =
session.getSystemProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ } catch (Exception ignored) {
Review Comment:
⚠️ **Silent exception swallowing** — `catch (Exception ignored)` hides any
failure here without logging. If `getUserProperties()` or
`getSystemProperties()` throws, the fallback silently continues and may return
`false` (tests not skipped) when the user intended to skip them. At minimum add
a `LOG.debug("Failed to check test skip from session properties", ignored)` so
failures are diagnosable.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -186,14 +187,100 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
@Override
public List<Node> flatten(@Nonnull Session s, @Nonnull Node node,
@Nullable PathScope scope)
throws DependencyResolverException {
+ return flatten(s, node, scope, null);
+ }
+
+ private List<Node> flatten(
+ @Nonnull Session s, @Nonnull Node node, @Nullable PathScope scope,
@Nullable Project project)
+ throws DependencyResolverException {
InternalSession session = InternalSession.from(s);
DependencyNode root = cast(AbstractNode.class, node,
"node").getDependencyNode();
List<DependencyNode> dependencies = session.getRepositorySystem()
- .flattenDependencyNodes(session.getSession(), root,
getScopeDependencyFilter(scope));
+ .flattenDependencyNodes(session.getSession(), root,
getDependencyFilter(session, scope, project));
dependencies.remove(root);
return map(dependencies, session::getNode);
}
+ private static DependencyFilter getDependencyFilter(InternalSession
session, PathScope scope, Project project) {
+ DependencyFilter scopeFilter = getScopeDependencyFilter(scope);
+ if (isTestSkip(session, project)) {
+ DependencyFilter testFilter = (node, parents) -> {
+ org.eclipse.aether.graph.Dependency dep = node.getDependency();
+ if (dep != null) {
+ return !(isTestArtifact(dep.getArtifact()) &&
"test".equals(dep.getScope()));
+ }
+ return true;
+ };
+ if (scopeFilter != null) {
+ return (node, parents) -> scopeFilter.accept(node, parents) &&
testFilter.accept(node, parents);
+ }
+ return testFilter;
+ }
+ return scopeFilter;
+ }
+
+ static boolean isTestArtifact(org.eclipse.aether.artifact.Artifact
artifact) {
+ return artifact != null
+ &&
("test-jar".equals(artifact.getProperty(ArtifactProperties.TYPE, ""))
+ || ("jar".equals(artifact.getExtension()) &&
"tests".equals(artifact.getClassifier())));
+ }
+
+ private static boolean isTestSkip(InternalSession session, Project
project) {
Review Comment:
💡 **Missing Javadoc** — `isTestSkip()` here duplicates logic from
`TestSkipUtils.isTestSkip()`. Please add Javadoc explaining why this separate
implementation exists (different session type, API layer, etc.) rather than
delegating. Similarly for `isTestArtifact()` (line 222), which duplicates
`RepositoryUtils.isTestArtifact()`.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -186,14 +187,100 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
@Override
public List<Node> flatten(@Nonnull Session s, @Nonnull Node node,
@Nullable PathScope scope)
throws DependencyResolverException {
+ return flatten(s, node, scope, null);
+ }
+
+ private List<Node> flatten(
+ @Nonnull Session s, @Nonnull Node node, @Nullable PathScope scope,
@Nullable Project project)
+ throws DependencyResolverException {
InternalSession session = InternalSession.from(s);
DependencyNode root = cast(AbstractNode.class, node,
"node").getDependencyNode();
List<DependencyNode> dependencies = session.getRepositorySystem()
- .flattenDependencyNodes(session.getSession(), root,
getScopeDependencyFilter(scope));
+ .flattenDependencyNodes(session.getSession(), root,
getDependencyFilter(session, scope, project));
dependencies.remove(root);
return map(dependencies, session::getNode);
}
+ private static DependencyFilter getDependencyFilter(InternalSession
session, PathScope scope, Project project) {
+ DependencyFilter scopeFilter = getScopeDependencyFilter(scope);
+ if (isTestSkip(session, project)) {
+ DependencyFilter testFilter = (node, parents) -> {
+ org.eclipse.aether.graph.Dependency dep = node.getDependency();
+ if (dep != null) {
+ return !(isTestArtifact(dep.getArtifact()) &&
"test".equals(dep.getScope()));
+ }
+ return true;
+ };
+ if (scopeFilter != null) {
+ return (node, parents) -> scopeFilter.accept(node, parents) &&
testFilter.accept(node, parents);
+ }
+ return testFilter;
+ }
+ return scopeFilter;
+ }
+
+ static boolean isTestArtifact(org.eclipse.aether.artifact.Artifact
artifact) {
+ return artifact != null
+ &&
("test-jar".equals(artifact.getProperty(ArtifactProperties.TYPE, ""))
+ || ("jar".equals(artifact.getExtension()) &&
"tests".equals(artifact.getClassifier())));
+ }
+
+ private static boolean isTestSkip(InternalSession session, Project
project) {
+ if (session != null) {
+ try {
+ if (session.getUserProperties() != null) {
+ String val =
session.getUserProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ if (session.getSystemProperties() != null) {
+ String val =
session.getSystemProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ } catch (Exception ignored) {
+ // Ignore if properties cannot be accessed
+ }
+ try {
+ RepositorySystemSession rSession = session.getSession();
+ if (rSession != null) {
+ Object val =
rSession.getConfigProperties().get("maven.test.skip");
+ if (val != null) {
+ String s = String.valueOf(val);
+ return Boolean.parseBoolean(s) || s.isEmpty();
+ }
+ val = rSession.getUserProperties().get("maven.test.skip");
+ if (val != null) {
+ String s = String.valueOf(val);
+ return Boolean.parseBoolean(s) || s.isEmpty();
+ }
+ val =
rSession.getSystemProperties().get("maven.test.skip");
+ if (val != null) {
+ String s = String.valueOf(val);
+ return Boolean.parseBoolean(s) || s.isEmpty();
+ }
+ }
+ } catch (Exception ignored) {
+ // Ignore
+ }
+ }
+ if (project != null) {
+ try {
+ if (project.getModel() != null &&
project.getModel().getProperties() != null) {
+ String val =
project.getModel().getProperties().get("maven.test.skip");
+ if (val != null) {
+ return Boolean.parseBoolean(val) || val.isEmpty();
+ }
+ }
+ } catch (Exception ignored) {
Review Comment:
⚠️ **Silent exception swallowing** — third `catch (Exception ignored)`
block. `project.getModel().getProperties()` throwing silently means this
fallback path also fails invisibly. Log at `DEBUG` level.
--
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]
