Copilot commented on code in PR #653: URL: https://github.com/apache/maven-checkstyle-plugin/pull/653#discussion_r4247879588
########## src/test/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutorTest.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.plugins.checkstyle.exec; + +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.io.Writer; +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests that the files selected for checking honor the standard default excludes (for example files below a + * {@code CVS} or {@code .svn} directory), in addition to any user supplied excludes. + */ +class DefaultCheckstyleExecutorTest { + + @TempDir + File sourceDirectory; + + @Test + void testDefaultExcludesAreApplied() throws Exception { + File included = new File(sourceDirectory, "Included.java"); + writeFile(included, "public class Included {}"); + + File cvs = new File(sourceDirectory, "CVS"); + assertTrue(cvs.mkdirs(), "Unable to create " + cvs); + File excluded = new File(cvs, "Excluded.java"); + writeFile(excluded, "public class Excluded {}"); + + List<String> names = getFilesToProcess(null).stream().map(File::getName).collect(Collectors.toList()); + + assertTrue(names.contains("Included.java"), "The regular source file should be checked: " + names); + assertFalse( + names.contains("Excluded.java"), + "A file below a default-excluded directory (CVS) must not be checked: " + names); + } + + @Test + void testUserExcludesAreCombinedWithDefaultExcludes() throws Exception { + File excludedByUser = new File(sourceDirectory, "ExcludedByUser.java"); + writeFile(excludedByUser, "public class ExcludedByUser {}"); + + File included = new File(sourceDirectory, "Included.java"); + writeFile(included, "public class Included {}"); + + File cvs = new File(sourceDirectory, "CVS"); + assertTrue(cvs.mkdirs(), "Unable to create " + cvs); + File excludedByDefault = new File(cvs, "ExcludedByDefault.java"); + writeFile(excludedByDefault, "public class ExcludedByDefault {}"); + + List<String> names = getFilesToProcess("**/ExcludedByUser.java").stream() + .map(File::getName) + .collect(Collectors.toList()); + + assertTrue(names.contains("Included.java"), "The regular source file should be checked: " + names); + assertFalse(names.contains("ExcludedByUser.java"), "The user excluded file must not be checked: " + names); + assertFalse( + names.contains("ExcludedByDefault.java"), + "A file below a default-excluded directory (CVS) must not be checked: " + names); + } + + private static void writeFile(File file, String content) throws IOException { + try (Writer writer = new FileWriter(file)) { + writer.write(content); + } + } + + /** + * Calls the private {@code getFilesToProcess} method which is where the default excludes are applied. + */ + private List<File> getFilesToProcess(String excludes) throws Exception { + CheckstyleExecutorRequest request = new CheckstyleExecutorRequest(); + request.setIncludes("**/*.java"); + request.setExcludes(excludes); + request.setSourceDirectories(Collections.singletonList(sourceDirectory)); + + DefaultCheckstyleExecutor executor = new DefaultCheckstyleExecutor(null, null); + Method method = + DefaultCheckstyleExecutor.class.getDeclaredMethod("getFilesToProcess", CheckstyleExecutorRequest.class); + method.setAccessible(true); + @SuppressWarnings("unchecked") + List<File> files = (List<File>) method.invoke(executor, request); Review Comment: The test calls a private method via reflection, which is brittle (renames/signature changes won’t be caught at compile time) and can be problematic under stronger Java access controls. Prefer exercising behavior through a public API, or (if acceptable) make the underlying helper package-private so the test can call it directly without reflection. ########## src/main/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutor.java: ########## @@ -501,11 +486,19 @@ private void addFilesToProcess( Collection<File> files, Collection<File> testSourceDirectories) throws IOException { + + String excludes = request.getExcludes(); + String defaultExcludes = String.join(",", FileUtils.getDefaultExcludes()); + if (excludes == null || excludes.isEmpty()) { + excludes = defaultExcludes; + } else { + excludes = excludes + "," + defaultExcludes; Review Comment: The variable name `defaultExcludes` is a `String` (CSV of patterns), but reads like it might be the `String[]` returned by `FileUtils.getDefaultExcludes()`. Renaming to something like `defaultExcludesCsv` / `defaultExcludePatterns` would reduce ambiguity and make the intent clearer. ########## src/test/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutorTest.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.plugins.checkstyle.exec; + +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.io.Writer; +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests that the files selected for checking honor the standard default excludes (for example files below a + * {@code CVS} or {@code .svn} directory), in addition to any user supplied excludes. + */ +class DefaultCheckstyleExecutorTest { + + @TempDir + File sourceDirectory; + + @Test + void testDefaultExcludesAreApplied() throws Exception { + File included = new File(sourceDirectory, "Included.java"); + writeFile(included, "public class Included {}"); + + File cvs = new File(sourceDirectory, "CVS"); + assertTrue(cvs.mkdirs(), "Unable to create " + cvs); + File excluded = new File(cvs, "Excluded.java"); + writeFile(excluded, "public class Excluded {}"); + + List<String> names = getFilesToProcess(null).stream().map(File::getName).collect(Collectors.toList()); + + assertTrue(names.contains("Included.java"), "The regular source file should be checked: " + names); + assertFalse( + names.contains("Excluded.java"), + "A file below a default-excluded directory (CVS) must not be checked: " + names); + } + + @Test + void testUserExcludesAreCombinedWithDefaultExcludes() throws Exception { + File excludedByUser = new File(sourceDirectory, "ExcludedByUser.java"); + writeFile(excludedByUser, "public class ExcludedByUser {}"); + + File included = new File(sourceDirectory, "Included.java"); + writeFile(included, "public class Included {}"); + + File cvs = new File(sourceDirectory, "CVS"); + assertTrue(cvs.mkdirs(), "Unable to create " + cvs); + File excludedByDefault = new File(cvs, "ExcludedByDefault.java"); + writeFile(excludedByDefault, "public class ExcludedByDefault {}"); + + List<String> names = getFilesToProcess("**/ExcludedByUser.java").stream() + .map(File::getName) + .collect(Collectors.toList()); + + assertTrue(names.contains("Included.java"), "The regular source file should be checked: " + names); + assertFalse(names.contains("ExcludedByUser.java"), "The user excluded file must not be checked: " + names); + assertFalse( + names.contains("ExcludedByDefault.java"), + "A file below a default-excluded directory (CVS) must not be checked: " + names); + } + + private static void writeFile(File file, String content) throws IOException { + try (Writer writer = new FileWriter(file)) { + writer.write(content); + } + } Review Comment: Using `FileWriter` relies on the platform default charset, which can make tests less deterministic across environments. Prefer writing with an explicit charset (e.g., via `Files.newBufferedWriter(..., StandardCharsets.UTF_8)` or `Files.writeString(..., StandardCharsets.UTF_8)`). -- 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]
