gnodet commented on code in PR #13067: URL: https://github.com/apache/maven/pull/13067#discussion_r3952079394
########## impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutorTest.java: ########## @@ -0,0 +1,261 @@ +/* + * 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.lifecycle.internal.concurrent; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; + +import org.apache.maven.api.Lifecycle; +import org.apache.maven.execution.DefaultMavenExecutionRequest; +import org.apache.maven.execution.DefaultMavenExecutionResult; +import org.apache.maven.execution.MavenExecutionRequest; +import org.apache.maven.execution.MavenSession; +import org.apache.maven.execution.ProjectDependencyGraph; +import org.apache.maven.execution.ProjectExecutionEvent; +import org.apache.maven.execution.ProjectExecutionListener; +import org.apache.maven.internal.impl.DefaultLifecycleRegistry; +import org.apache.maven.internal.transformation.TransformerManager; +import org.apache.maven.lifecycle.LifecycleExecutionException; +import org.apache.maven.lifecycle.internal.LifecycleTask; +import org.apache.maven.lifecycle.internal.ReactorBuildStatus; +import org.apache.maven.lifecycle.internal.ReactorContext; +import org.apache.maven.lifecycle.internal.TaskSegment; +import org.apache.maven.lifecycle.internal.stub.ExecutionEventCatapultStub; +import org.apache.maven.project.MavenProject; +import org.eclipse.aether.DefaultRepositorySystemSession; +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.deployment.DeployRequest; +import org.eclipse.aether.installation.InstallRequest; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class BuildPlanExecutorTest { + + /** + * A build step that throws an {@link Error} must be reported as a build failure, the same way the + * single threaded builder reports it. Otherwise the build ends with no exception at all and Maven + * prints BUILD SUCCESS while nothing was built. + */ + @Test + void errorThrownByBuildStepIsRecordedAsBuildFailure() throws Exception { + Error thrown = new NoClassDefFoundError("some/Class"); + MavenProject project = newProject(); + MavenSession session = newSession(project); + + execute(session, project, event -> { + throw thrown; + }); + + List<Throwable> exceptions = session.getResult().getExceptions(); + assertEquals(1, exceptions.size(), "expected the error to be recorded, but got: " + exceptions); + assertSame(thrown, exceptions.get(0)); + assertTrue(session.getResult().getBuildSummary(project) instanceof org.apache.maven.execution.BuildFailure); + } + + /** + * The same for an exception, which already worked. This pins the existing behaviour so the widened + * catch does not change it. + */ + @Test + void exceptionThrownByBuildStepIsRecordedAsBuildFailure() throws Exception { Review Comment: **Missing coverage:** `exceptionThrownByBuildStepIsRecordedAsBuildFailure` uses `IllegalStateException` (a RuntimeException), so it tests the *fatal* path — the build will halt just like for an `Error`. There is no test that throws a checked exception and verifies that the reactor is **not** halted (i.e. the soft-failure path: `isFatal` returns `false`, event fires, blacklisting happens). That path existed before this PR and the widened `catch (Throwable)` should not affect it, but having it pinned would prevent a future regression in `isFatal` from silently breaking `REACTOR_FAIL_AT_END` for ordinary plugin failures. ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java: ########## @@ -578,6 +578,18 @@ private void executeStep(BuildStep step) throws IOException, LifecycleExecutionE step.status.compareAndSet(SCHEDULED, EXECUTED); } + /** + * Tells whether any of the failures collected for a project must halt the build. Several failures are + * reported through a wrapper, and a wrapper is always a checked exception, so an {@link Error} among + * them can only be seen by looking at the failures themselves. + * + * @param failures The failures collected for a single project + * @return {@code true} if the build must be halted + */ + private static boolean isFatal(List<Throwable> failures) { + return failures.stream().anyMatch(t -> t instanceof RuntimeException || !(t instanceof Exception)); Review Comment: **Nit / Documentation gap:** The name `isFatal` and the predicate `t instanceof RuntimeException || !(t instanceof Exception)` are correct, but the Javadoc only explains *what* this method does, not *why* `RuntimeException` is treated as fatal alongside `Error`. A reader unfamiliar with the original design intent in `handleBuildError` will be confused: checked exceptions are "soft" failures, RuntimeExceptions and Errors are "hard" ones. Worth a single sentence in the `@return` tag: ```suggestion private static boolean isFatal(List<Throwable> failures) { // RuntimeExceptions are treated as system errors on par with Errors: // both indicate the JVM or framework is in an unexpected state and // further build steps are unlikely to succeed. return failures.stream().anyMatch(t -> t instanceof RuntimeException || !(t instanceof Exception)); } ``` -- 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]
