Copilot commented on code in PR #3528:
URL: https://github.com/apache/maven-surefire/pull/3528#discussion_r4346495889
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -3015,6 +3019,29 @@ private void warnIfDefunctGroupsCombinations() throws
MojoFailureException, Mojo
}
}
+ private boolean containsTestClasses(File directory) {
+ if (directory == null) {
+ return false;
+ }
+
+ File[] files = directory.listFiles();
Review Comment:
This scan allocates a `File[]` for every directory visited and may traverse
large trees under `target/test-classes`, which can add noticeable overhead on
big projects. A more efficient approach is to use NIO streaming traversal (so
you can short-circuit without materializing entire directory listings) or at
least filter `listFiles` to only directories and `*.class` to reduce
allocations.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -3015,6 +3019,29 @@ private void warnIfDefunctGroupsCombinations() throws
MojoFailureException, Mojo
}
}
+ private boolean containsTestClasses(File directory) {
+ if (directory == null) {
+ return false;
+ }
+
+ File[] files = directory.listFiles();
+ if (files == null) {
+ return false;
+ }
+
+ for (File file : files) {
+ if (file.isDirectory()) {
+ if (containsTestClasses(file)) {
+ return true;
+ }
+ } else if (file.getName().endsWith(".class")) {
+ return true;
+ }
+ }
+
+ return false;
Review Comment:
This recursive traversal can follow directory symlinks (depending on
platform/filesystem), which can lead to cycles (infinite recursion) or very
deep recursion (stack overflow) in unusual but real-world layouts. Consider
switching to `java.nio.file.Files.walk(...)`/`walkFileTree(...)` without
following links and using an early-terminating `anyMatch`/visitor to keep the
scan bounded and robust.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -3015,6 +3019,29 @@ private void warnIfDefunctGroupsCombinations() throws
MojoFailureException, Mojo
}
}
+ private boolean containsTestClasses(File directory) {
+ if (directory == null) {
+ return false;
+ }
+
+ File[] files = directory.listFiles();
+ if (files == null) {
+ return false;
+ }
+
+ for (File file : files) {
Review Comment:
This scan allocates a `File[]` for every directory visited and may traverse
large trees under `target/test-classes`, which can add noticeable overhead on
big projects. A more efficient approach is to use NIO streaming traversal (so
you can short-circuit without materializing entire directory listings) or at
least filter `listFiles` to only directories and `*.class` to reduce
allocations.
##########
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:
##########
@@ -1949,6 +1949,14 @@ protected File[] getSuiteXmlFiles() {
mojo.ensureNoSuiteXmlFiles();
}
+ @Test
Review Comment:
The PR description appears to still contain the default template placeholder
text and the Apache License/ICLA checkbox section is left unchecked. Please
update the PR description to explain the change and select the appropriate
licensing checkbox so the submission metadata is complete.
##########
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:
##########
@@ -1949,6 +1949,14 @@ protected File[] getSuiteXmlFiles() {
mojo.ensureNoSuiteXmlFiles();
}
+ @Test
+ public void shouldNotValidateGroupsWhenNoTestClassesExist() throws
Exception {
+ mojo.setTestClassesDirectory(tempFolder.toFile());
+ setInternalState(mojo, "excludedGroups", "SlowTest");
+
+ invokeMethod(mojo, "warnIfDefunctGroupsCombinations");
+ }
Review Comment:
This test currently has no explicit assertion and relies on the absence of
an exception. Making the intent explicit (e.g., using an
`assertDoesNotThrow`-style assertion around `invokeMethod(...)`) will improve
readability and ensure the test continues to clearly document the expected
behavior.
--
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]
