gnodet-bot commented on code in PR #13774: URL: https://github.com/apache/maven/pull/13774#discussion_r4436056374
########## impl/maven-core/src/main/java/org/apache/maven/internal/TestSkipUtils.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.execution.MavenSession; +import org.apache.maven.project.MavenProject; +import org.eclipse.aether.RepositorySystemSession; + +/** + * Utility methods for checking test skip policy (e.g. {@code -Dmaven.test.skip}). + */ +public final class TestSkipUtils { + Review Comment: ⚠️ **Missing unit tests** — `TestSkipUtils` is a new utility class with non-trivial branching logic (user props → system props → repo session config props → repo session user/system props → project props). This multi-level fallback chain should be tested directly with a `TestSkipUtilsTest`, covering at least: - `maven.test.skip=true` in user properties - `maven.test.skip=true` in system properties - `maven.test.skip` in repository session config properties - `maven.test.skip` set on the project - all inputs null (should return false) - empty string value (should return true per `val.isEmpty()`) The integration-level test in `LifecycleDependencyResolverTest` exercises one path but does not cover the full decision tree. ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleDependencyResolver.java: ########## @@ -259,6 +260,18 @@ private Set<Artifact> getDependencies( resolutionFilter = AndDependencyFilter.newInstance(resolutionFilter, new ReactorDependencyFilter(projectArtifacts)); + if (TestSkipUtils.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())); Review Comment: ⚠️ **DRY violation** — this `testArtifactFilter` lambda is character-for-character identical to the one added in `DefaultProjectDependenciesResolver.java`. Extract it as a static factory method in `TestSkipUtils` (or `RepositoryUtils`) to avoid silent drift between the two paths: ```java // In TestSkipUtils (or RepositoryUtils): public static DependencyFilter testArtifactFilter() { return (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; }; } ``` Then both callers become `TestSkipUtils.testArtifactFilter()` (or similar), and the filter logic lives in exactly one place. -- 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]
