Copilot commented on code in PR #3489: URL: https://github.com/apache/maven-surefire/pull/3489#discussion_r4346265981
########## surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire3463TestNgExtensionFailureIT.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.surefire.its.jiras; + +import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase; +import org.junit.jupiter.api.Test; Review Comment: This IT extends `SurefireJUnit4IntegrationTestCase` but uses JUnit Jupiter’s `@Test`. If the `surefire-its` test runtime is JUnit 4-based (as implied by the base class name), this method may not execute at all. Use `org.junit.Test` here (and make the test method `public` if required by the runner), or switch to the JUnit Jupiter IT base class that matches the annotation. ########## surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire3463TestNgExtensionFailureIT.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.surefire.its.jiras; + +import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase; +import org.junit.jupiter.api.Test; + +/** + * A TestNG extension failure during JUnit Platform discovery should retain its cause message. + */ +public class Surefire3463TestNgExtensionFailureIT extends SurefireJUnit4IntegrationTestCase { + + @Test + void testNgExtensionFailureIsReported() { Review Comment: This IT extends `SurefireJUnit4IntegrationTestCase` but uses JUnit Jupiter’s `@Test`. If the `surefire-its` test runtime is JUnit 4-based (as implied by the base class name), this method may not execute at all. Use `org.junit.Test` here (and make the test method `public` if required by the runner), or switch to the JUnit Jupiter IT base class that matches the annotation. ########## surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/LauncherAdapter.java: ########## @@ -57,15 +66,73 @@ void execute(LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... void executeWithoutCancellationToken( LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) { - delegate.execute(discoveryRequest, listeners); + try { + delegate.execute(discoveryRequest, listeners); + } catch (JUnitException e) { + throw withRootCauseMessage(e); + } } private void executeWithCancellationToken( LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) { Method executeMethod = requireNonNull(EXECUTE_METHOD_WITH_LAUNCHER_EXECUTION_REQUEST_PARAMETER); Object executionRequest = createExecutionRequest(discoveryRequest, listeners); - ReflectionUtils.invokeMethodWithArray(delegate, executeMethod, executionRequest); + try { + ReflectionUtils.invokeMethodWithArray(delegate, executeMethod, executionRequest); + } catch (JUnitException e) { + throw withRootCauseMessage(e); + } catch (SurefireReflectionException e) { + JUnitException junitException = findJUnitException(e); + if (junitException != null) { + throw withRootCauseMessage(junitException); + } + throw e; + } + } + + static JUnitException findJUnitException(Throwable failure) { + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + Throwable cause = failure; + while (cause != null && visited.add(cause)) { + if (cause instanceof JUnitException) { + return (JUnitException) cause; + } + cause = cause.getCause(); + } + return null; + } + + private static JUnitException withRootCauseMessage(JUnitException failure) { + Throwable rootCause = failure; + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + visited.add(rootCause); + while (rootCause.getCause() != null && visited.add(rootCause.getCause())) { + rootCause = rootCause.getCause(); + } + + if (rootCause == failure) { + return failure; + } + + String causeMessage = rootCause.getMessage(); + String rootCauseDescription = rootCause.toString(); + String message = failure.getMessage(); + if (message != null) { + if (message.contains(rootCauseDescription)) { + return failure; + } + if (causeMessage != null && !causeMessage.isEmpty() && message.contains(causeMessage)) { + String rootCauseType = rootCause.getClass().getName(); + if (message.contains(rootCauseType)) { + return failure; + } + return new JUnitException(message + ": " + rootCauseType, failure); + } + } + return new JUnitException( + message == null || message.isEmpty() ? rootCauseDescription : message + ": " + rootCauseDescription, + failure); Review Comment: When the root cause message is an empty string, `rootCause.toString()` typically renders a trailing \": \" (e.g., `java.lang.IllegalStateException: `), which produces a slightly malformed user-facing message. Consider formatting the appended root-cause text so that an empty message results in the class name only (e.g., `rootCause.getClass().getName()`), while still using `toString()` when a non-empty message exists. This will make the emitted discovery failure message cleaner and more consistent. ########## surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/LauncherAdapter.java: ########## @@ -57,15 +66,73 @@ void execute(LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... void executeWithoutCancellationToken( LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) { - delegate.execute(discoveryRequest, listeners); + try { + delegate.execute(discoveryRequest, listeners); + } catch (JUnitException e) { + throw withRootCauseMessage(e); + } } private void executeWithCancellationToken( LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) { Method executeMethod = requireNonNull(EXECUTE_METHOD_WITH_LAUNCHER_EXECUTION_REQUEST_PARAMETER); Object executionRequest = createExecutionRequest(discoveryRequest, listeners); - ReflectionUtils.invokeMethodWithArray(delegate, executeMethod, executionRequest); + try { + ReflectionUtils.invokeMethodWithArray(delegate, executeMethod, executionRequest); + } catch (JUnitException e) { + throw withRootCauseMessage(e); + } catch (SurefireReflectionException e) { + JUnitException junitException = findJUnitException(e); + if (junitException != null) { + throw withRootCauseMessage(junitException); + } + throw e; + } + } + + static JUnitException findJUnitException(Throwable failure) { + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + Throwable cause = failure; + while (cause != null && visited.add(cause)) { + if (cause instanceof JUnitException) { + return (JUnitException) cause; + } + cause = cause.getCause(); + } + return null; + } + + private static JUnitException withRootCauseMessage(JUnitException failure) { + Throwable rootCause = failure; + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + visited.add(rootCause); + while (rootCause.getCause() != null && visited.add(rootCause.getCause())) { + rootCause = rootCause.getCause(); + } + + if (rootCause == failure) { + return failure; + } + + String causeMessage = rootCause.getMessage(); + String rootCauseDescription = rootCause.toString(); Review Comment: When the root cause message is an empty string, `rootCause.toString()` typically renders a trailing \": \" (e.g., `java.lang.IllegalStateException: `), which produces a slightly malformed user-facing message. Consider formatting the appended root-cause text so that an empty message results in the class name only (e.g., `rootCause.getClass().getName()`), while still using `toString()` when a non-empty message exists. This will make the emitted discovery failure message cleaner and more consistent. ########## surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/LauncherAdapter.java: ########## @@ -57,15 +66,73 @@ void execute(LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... void executeWithoutCancellationToken( LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) { - delegate.execute(discoveryRequest, listeners); + try { + delegate.execute(discoveryRequest, listeners); + } catch (JUnitException e) { + throw withRootCauseMessage(e); + } } private void executeWithCancellationToken( LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) { Method executeMethod = requireNonNull(EXECUTE_METHOD_WITH_LAUNCHER_EXECUTION_REQUEST_PARAMETER); Object executionRequest = createExecutionRequest(discoveryRequest, listeners); - ReflectionUtils.invokeMethodWithArray(delegate, executeMethod, executionRequest); + try { + ReflectionUtils.invokeMethodWithArray(delegate, executeMethod, executionRequest); + } catch (JUnitException e) { + throw withRootCauseMessage(e); + } catch (SurefireReflectionException e) { + JUnitException junitException = findJUnitException(e); + if (junitException != null) { + throw withRootCauseMessage(junitException); + } + throw e; + } + } + + static JUnitException findJUnitException(Throwable failure) { + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + Throwable cause = failure; + while (cause != null && visited.add(cause)) { + if (cause instanceof JUnitException) { + return (JUnitException) cause; + } + cause = cause.getCause(); + } + return null; + } + + private static JUnitException withRootCauseMessage(JUnitException failure) { + Throwable rootCause = failure; + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + visited.add(rootCause); + while (rootCause.getCause() != null && visited.add(rootCause.getCause())) { + rootCause = rootCause.getCause(); + } Review Comment: The cause-chain traversal logic (including cycle detection) is duplicated in `findJUnitException` and `withRootCauseMessage`, and `withRootCauseMessage` also calls `getCause()` multiple times per loop iteration. Consider extracting a small private helper to iterate a cause chain safely (or to compute `deepestCause(Throwable)`), and reuse it in both methods. This reduces duplication and makes future message/chain handling changes less error-prone. -- 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]
