Copilot commented on code in PR #3418:
URL: https://github.com/apache/maven-surefire/pull/3418#discussion_r3762657306
##########
surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java:
##########
@@ -316,12 +317,41 @@ private TestExecutionListener createJUnit4Listeners() {
return null;
}
- private LauncherDiscoveryRequest
buildLauncherDiscoveryRequestForRerunFailures(RunListenerAdapter adapter) {
+ LauncherDiscoveryRequest
buildLauncherDiscoveryRequestForRerunFailures(RunListenerAdapter adapter) {
LauncherDiscoveryRequestBuilder builder = newRequest();
- // Iterate over recorded failures
- for (TestIdentifier identifier :
- new LinkedHashSet<>(adapter.getFailures().keySet())) {
- builder.selectors(selectUniqueId(identifier.getUniqueId()));
+ LinkedHashSet<TestIdentifier> failures =
+ new LinkedHashSet<>(adapter.getFailures().keySet());
+ LinkedHashSet<UniqueId> failureIds = failures.stream()
+ .map(TestIdentifier::getUniqueId)
+ .map(UniqueId::parse)
+ .collect(Collectors.toCollection(LinkedHashSet::new));
+ adapter.setRerunTestIds(failureIds);
+ LinkedHashSet<String> classNames = new LinkedHashSet<>();
+
+ for (TestIdentifier identifier : failures) {
+ // Runner-backed Vintage tests may expose only a ClassSource and
may not
+ // support rediscovery from a leaf UniqueId. Rediscover their
owning class
+ // and retain the failed branch with a post-discovery filter
instead.
+ Optional<ClassSource> classSource =
+
identifier.getSource().filter(ClassSource.class::isInstance).map(ClassSource.class::cast);
+ if (classSource.isPresent()) {
+ classNames.add(classSource.get().getClassName());
+ } else {
+ builder.selectors(selectUniqueId(identifier.getUniqueId()));
+ }
+ }
+
+ if (!classNames.isEmpty()) {
+ classNames.forEach(className ->
builder.selectors(selectClass(className)));
+ builder.filters((PostDiscoveryFilter) testDescriptor -> {
+ UniqueId candidateId = testDescriptor.getUniqueId();
+ boolean isFailureOrRelatedContainer = failureIds.stream()
+ .anyMatch(failureId ->
candidateId.hasPrefix(failureId) || failureId.hasPrefix(candidateId));
Review Comment:
This post-discovery filter scans every failed ID for every discovered
descriptor, so rerunning F failed tests performs O(F²) prefix checks. Large
failing suites can spend substantial time in discovery before executing tests.
Precompute an ancestor/prefix index with constant- or logarithmic-time
membership checks.
##########
surefire-its/src/test/resources/surefire-2274-failsafe-flake-count/pom.xml:
##########
@@ -0,0 +1,66 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
https://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
Review Comment:
This new POM uses four-space nesting, but `.editorconfig:10-13` requires
two-space indentation for XML. Reformat the XML hierarchy to match the
repository convention.
##########
surefire-its/src/test/resources/surefire-2300-before-all-assumption/pom.xml:
##########
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
https://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
Review Comment:
This new POM uses four-space nesting, but `.editorconfig:10-13` requires
two-space indentation for XML. Reformat the XML hierarchy to match the
repository convention.
##########
surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java:
##########
@@ -316,12 +317,41 @@ private TestExecutionListener createJUnit4Listeners() {
return null;
}
- private LauncherDiscoveryRequest
buildLauncherDiscoveryRequestForRerunFailures(RunListenerAdapter adapter) {
+ LauncherDiscoveryRequest
buildLauncherDiscoveryRequestForRerunFailures(RunListenerAdapter adapter) {
LauncherDiscoveryRequestBuilder builder = newRequest();
- // Iterate over recorded failures
- for (TestIdentifier identifier :
- new LinkedHashSet<>(adapter.getFailures().keySet())) {
- builder.selectors(selectUniqueId(identifier.getUniqueId()));
+ LinkedHashSet<TestIdentifier> failures =
+ new LinkedHashSet<>(adapter.getFailures().keySet());
+ LinkedHashSet<UniqueId> failureIds = failures.stream()
+ .map(TestIdentifier::getUniqueId)
+ .map(UniqueId::parse)
+ .collect(Collectors.toCollection(LinkedHashSet::new));
+ adapter.setRerunTestIds(failureIds);
+ LinkedHashSet<String> classNames = new LinkedHashSet<>();
+
+ for (TestIdentifier identifier : failures) {
+ // Runner-backed Vintage tests may expose only a ClassSource and
may not
+ // support rediscovery from a leaf UniqueId. Rediscover their
owning class
+ // and retain the failed branch with a post-discovery filter
instead.
+ Optional<ClassSource> classSource =
+
identifier.getSource().filter(ClassSource.class::isInstance).map(ClassSource.class::cast);
+ if (classSource.isPresent()) {
+ classNames.add(classSource.get().getClassName());
+ } else {
+ builder.selectors(selectUniqueId(identifier.getUniqueId()));
+ }
Review Comment:
`ClassSource` identifies where a test came from; it does not guarantee that
the engine supports `ClassSelector`. Dropping the existing unique-ID selector
can therefore make reruns discover no test for third-party engines that expose
`ClassSource` but only support unique-ID rediscovery. Keep the unique-ID
selector for every failure and add the class selector as the Vintage-runner
fallback.
##########
surefire-its/src/test/resources/surefire-2239-testng-rerun/pom.xml:
##########
@@ -0,0 +1,72 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
https://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
Review Comment:
This new POM uses four-space nesting, but `.editorconfig:10-13` requires
two-space indentation for XML. Reformat the XML hierarchy to match the
repository convention.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/TestMethodKey.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.plugin.surefire.report;
+
+import java.util.Objects;
+
+/**
+ * Identity used to aggregate the executions of one test across reruns.
Providers
+ * without a test run ID retain the legacy class-and-method-name grouping.
+ */
+final class TestMethodKey implements Comparable<TestMethodKey> {
+ private final String testClassMethodName;
+
+ private final Long testRunId;
+
+ TestMethodKey(String testClassMethodName, Long testRunId) {
+ this.testClassMethodName = testClassMethodName;
+ this.testRunId = testRunId;
+ }
+
+ String getTestClassMethodName() {
+ return testClassMethodName;
+ }
+
+ @Override
+ public int compareTo(TestMethodKey other) {
+ int byName = compare(testClassMethodName, other.testClassMethodName);
+ return byName == 0 ? compare(testRunId, other.testRunId) : byName;
+ }
+
+ private static <T extends Comparable<T>> int compare(T left, T right) {
+ if (left == null) {
+ return right == null ? 0 : -1;
+ }
+ return right == null ? 1 : left.compareTo(right);
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (!(other instanceof TestMethodKey)) {
+ return false;
+ }
+ TestMethodKey that = (TestMethodKey) other;
+ return Objects.equals(testClassMethodName, that.testClassMethodName)
+ && Objects.equals(testRunId, that.testRunId);
Review Comment:
This key still requires the human-readable name to match even when a
test-run ID is present. If an engine reports a different display/legacy name
when rediscovering the same unique ID, the failure and successful rerun become
separate tests instead of a flake. Scope the numeric ID with a stable
source/class identity, use that as the key when present, and retain
class-and-method-name grouping only for null IDs.
##########
surefire-its/src/test/resources/surefire-2241-junitparams-rerun/pom.xml:
##########
@@ -0,0 +1,88 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
https://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
Review Comment:
This new POM uses four-space nesting, but `.editorconfig:10-13` requires
two-space indentation for XML. Reformat the XML hierarchy to match the
repository convention.
##########
surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/RunListenerAdapter.java:
##########
@@ -603,6 +693,20 @@ Map<TestIdentifier, TestExecutionResult> getFailures() {
return failures;
}
+ void setRerunTestIds(Set<UniqueId> rerunTestIds) {
+ this.rerunTestIds = unmodifiableSet(new LinkedHashSet<>(rerunTestIds));
+ }
+
+ private boolean isRerunTarget(TestIdentifier testIdentifier) {
+ if (runMode != RERUN_TEST_AFTER_FAILURE || rerunTestIds.isEmpty()) {
+ return true;
+ }
+
+ UniqueId candidateId = UniqueId.parse(testIdentifier.getUniqueId());
+ return rerunTestIds.stream()
+ .anyMatch(failureId -> candidateId.hasPrefix(failureId) ||
failureId.hasPrefix(candidateId));
Review Comment:
Every JUnit execution callback linearly scans all failed IDs. Since reruns
emit O(F) target events when F tests fail, this adds another O(F²) path during
execution. Reuse a precomputed ancestor/prefix index rather than streaming the
full failure set for each callback.
##########
surefire-its/src/test/resources/surefire-2260-non-unique-scenarios/pom.xml:
##########
@@ -0,0 +1,91 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
https://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
Review Comment:
This new POM uses four-space nesting, but `.editorconfig:10-13` requires
two-space indentation for XML. Reformat the XML hierarchy to match the
repository convention.
--
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]
