gnodet-bot commented on code in PR #13774:
URL: https://github.com/apache/maven/pull/13774#discussion_r4436252337
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -186,14 +191,122 @@ 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);
Review Comment:
⚠️ **`flatten()` public API doesn't propagate project** — The public
`flatten(Session, Node, PathScope)` method passes `null` as the project to the
private overload. This means if `maven.test.skip` is set only via project
properties (e.g. in a `<properties>` section of a POM, not via command-line
`-D` or system property), the filter will NOT be applied when callers invoke
`flatten()` directly.
The internal `resolve()` path correctly passes
`request.getProject().orElse(null)`, so that path is fine. But embedders and
extensions calling `DependencyResolver.flatten()` directly will miss
project-property-based test-skip.
Consider documenting this limitation in the Javadoc of `flatten()`, or if
the project is accessible from the `Session` at that point, propagate it.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -186,14 +191,122 @@ 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;
+ }
+
+ /**
+ * Determines whether the specified artifact refers to test classes (e.g.
test-jar).
+ * <p>
+ * Note: This method intentionally duplicates the logic in {@code
RepositoryUtils.isTestArtifact}
+ * because {@code maven-impl} is part of the Maven 4 API implementation
layer and does not depend
+ * on {@code maven-core} (which contains {@code RepositoryUtils}).
+ *
+ * @param artifact the artifact to check, may be {@code null}
+ * @return {@code true} if the artifact refers to test classes, {@code
false} otherwise
+ */
+ 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())));
+ }
+
+ /**
+ * Determines whether test execution and compilation are skipped (e.g. via
{@code -Dmaven.test.skip}).
+ * <p>
+ * Note: This method exists separately from {@code
TestSkipUtils.isTestSkip} because {@code maven-impl}
+ * operates on the Maven 4 API types ({@link InternalSession}, {@link
Project}) and cannot depend on
+ * {@code maven-core} where {@code TestSkipUtils} resides. Property
precedence order follows
+ * {@code userProperties > systemProperties > configProperties >
projectProperties}.
+ *
+ * @param session the internal session, may be {@code null}
+ * @param project the Maven project, may be {@code null}
+ * @return {@code true} if tests are skipped, {@code false} otherwise
+ */
+ 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 e) {
Review Comment:
⚠️ **Broad exception catch** — `catch (Exception e)` is overly wide here.
The only operations inside the `try` block are `Map` accessor calls
(`getUserProperties().get(...)`, `getSystemProperties().get(...)`) that return
`String`/`Object`. These do not declare checked exceptions, and the only
realistic failure mode is `UnsupportedOperationException` (immutable/view map).
Catching the root `Exception` type masks any unexpected failure — a
`NullPointerException` from a partially-initialized session, for example, would
be silently swallowed and return `false` (tests not skipped), causing a silent
behavior change.
Use `RuntimeException` or add `@SuppressWarnings` with an explicit
explanation if you intend broad catching:
```suggestion
} catch (RuntimeException e) {
LOGGER.debug("Failed to check test skip from session
properties", e);
}
```
--
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]
