Merge pull request #4162 from adangel:improve-TestClassWithoutTestCases

[java] Improve TestClassWithoutTestCases #4162

docs/pages/release_notes.md

@@ -25,6 +25,13 @@ This is a {{ site.pmd.release_type }} release.
 
 The rule is part of the quickstart.xml ruleset.
 
+#### Modified Rules
+
+* The Java rule {% rule java/errorprone/TestClassWithoutTestCases %} has a new property `testClassPattern`. This is
+  used to detect empty test classes by name. Previously this rule could only detect empty JUnit3 test cases
+  properly. To switch back to the old behavior, this property can be set to an empty value which disables the
+  test class detection by pattern.
+
 ### Fixed Issues
 * apex
     * [#4149](https://github.com/pmd/pmd/issues/4149): \[apex] New rule: ApexUnitTestClassShouldHaveRunAs
@@ -45,6 +52,9 @@ The rule is part of the quickstart.xml ruleset.
     * [#3977](https://github.com/pmd/pmd/issues/3977): \[java] StringToString false-positive with local method name confusion
     * [#4091](https://github.com/pmd/pmd/issues/4091): \[java] AvoidArrayLoops false negative with do-while loops
     * [#4148](https://github.com/pmd/pmd/issues/4148): \[java] UseArrayListInsteadOfVector ignores Vector when other classes are imported
+* java-errorprone
+    * [#929](https://github.com/pmd/pmd/issues/929): \[java] Inconsistent results with TestClassWithoutTestCases
+    * [#2636](https://github.com/pmd/pmd/issues/2636): \[java] TestClassWithoutTestCases false positive with JUnit5 ParameterizedTest
 
 ### API Changes
 

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/AbstractJUnitRule.java

@@ -4,7 +4,11 @@
 
 package net.sourceforge.pmd.lang.java.rule;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.annotation.InternalApi;
@@ -30,6 +34,15 @@ public abstract class AbstractJUnitRule extends AbstractJavaRule {
     protected static final String JUNIT3_CLASS_NAME = "junit.framework.TestCase";
     protected static final String JUNIT4_CLASS_NAME = "org.junit.Test";
     protected static final String JUNIT5_CLASS_NAME = "org.junit.jupiter.api.Test";
+    private static final String JUNIT5_NESTED = "org.junit.jupiter.api.Nested";
+    private static final Set<String> JUNIT5_TEST_ANNOTATIONS = new HashSet<>(Arrays.asList(
+            JUNIT5_CLASS_NAME,
+            "org.junit.jupiter.api.RepeatedTest",
+            "org.junit.jupiter.api.TestFactory",
+            "org.junit.jupiter.api.TestTemplate",
+            "org.junit.jupiter.params.ParameterizedTest"));
+
+    private static final String TESTNG_ANNOTATION = "org.testng.annotations.Test";
 
     protected boolean isJUnit3Class;
     protected boolean isJUnit4Class;
@@ -92,12 +105,12 @@ protected void analyzeJUnitClass(ASTClassOrInterfaceDeclaration node) {
 
         if (isJUnit4Class && isJUnit5Class) {
             isJUnit4Class &= hasImports(node, JUNIT4_CLASS_NAME);
-            isJUnit5Class &= hasImports(node, JUNIT5_CLASS_NAME);
+            isJUnit5Class &= hasImports(node, JUNIT5_TEST_ANNOTATIONS);
         }
     }
 
     public static boolean isTestClass(ASTClassOrInterfaceBody node) {
-        return !isAbstractClass(node)
+        return !isAbstractClass(node) && node.getParent() instanceof ASTClassOrInterfaceDeclaration
                 && (isTestClassJUnit3(node) || isTestClassJUnit4(node) || isTestClassJUnit5(node));
     }
 
@@ -131,7 +144,7 @@ public static boolean isTestClassJUnit5(ASTClassOrInterfaceBody node) {
         Node parent = node.getParent();
         if (parent instanceof TypeNode) {
             TypeNode type = (TypeNode) parent;
-            return isJUnit5Class(type) && hasImports(type, JUNIT5_CLASS_NAME);
+            return isJUnit5Class(type) && hasImports(type, JUNIT5_TEST_ANNOTATIONS);
         }
         return false;
     }
@@ -145,7 +158,12 @@ public static boolean isTestMethod(ASTMethodDeclaration method) {
         ASTClassOrInterfaceBody type = method.getFirstParentOfType(ASTClassOrInterfaceBody.class);
         return isTestClassJUnit3(type) && method.isPublic() && method.isVoid() && method.getName().startsWith("test")
                 || isTestClassJUnit4(type) && method.isPublic() && doesNodeContainJUnitAnnotation(method.getParent(), JUNIT4_CLASS_NAME)
-                || isTestClassJUnit5(type) && doesNodeContainJUnitAnnotation(method.getParent(), JUNIT5_CLASS_NAME);
+                || isTestClassJUnit5(type) && doesNodeContainJUnitAnnotation(method.getParent(), JUNIT5_TEST_ANNOTATIONS)
+                || doesNodeContainJUnitAnnotation(method.getParent(), TESTNG_ANNOTATION);
+    }
+
+    public static boolean isJUnit5NestedClass(ASTClassOrInterfaceBody innerClassDecl) {
+        return doesNodeContainJUnitAnnotation((JavaNode) innerClassDecl.getNthParent(2), JUNIT5_NESTED);
     }
 
     public boolean isJUnitMethod(ASTMethodDeclaration method, Object data) {
@@ -170,7 +188,7 @@ private boolean isJUnit4Method(ASTMethodDeclaration method) {
     }
 
     private boolean isJUnit5Method(ASTMethodDeclaration method) {
-        return isJUnit5Class && doesNodeContainJUnitAnnotation(method.getParent(), JUNIT5_CLASS_NAME);
+        return isJUnit5Class && doesNodeContainJUnitAnnotation(method.getParent(), JUNIT5_TEST_ANNOTATIONS);
     }
 
     private boolean isJUnit3Method(ASTMethodDeclaration method) {
@@ -186,34 +204,72 @@ private static boolean isJUnit4Class(JavaNode node) {
     }
 
     private static boolean isJUnit5Class(JavaNode node) {
-        return doesNodeContainJUnitAnnotation(node, JUNIT5_CLASS_NAME);
+        return doesNodeContainJUnitAnnotation(node, JUNIT5_TEST_ANNOTATIONS);
     }
 
     private static boolean doesNodeContainJUnitAnnotation(JavaNode node, String annotationTypeClassName) {
+        return doesNodeContainJUnitAnnotation(node, Collections.singleton(annotationTypeClassName));
+    }
+
+    private static boolean doesNodeContainJUnitAnnotation(JavaNode node, Set<String> annotationTypeClassNames) {
         List<ASTAnnotation> annotations = node.findDescendantsOfType(ASTAnnotation.class);
         for (ASTAnnotation annotation : annotations) {
             Node annotationTypeNode = annotation.getChild(0);
             TypeNode annotationType = (TypeNode) annotationTypeNode;
             if (annotationType.getType() == null) {
                 ASTName name = annotationTypeNode.getFirstChildOfType(ASTName.class);
-                if (name != null && (name.hasImageEqualTo("Test") || name.hasImageEqualTo(annotationTypeClassName))) {
-                    return true;
+                if (name != null) {
+                    for (String annotationTypeName : annotationTypeClassNames) {
+                        if (areNamesEqual(name, annotationTypeName)) {
+                            return true;
+                        }
+                    }
+                }
+            } else {
+                for (String annotationTypeClassName : annotationTypeClassNames) {
+                    if (TypeTestUtil.isA(annotationTypeClassName, annotationType)) {
+                        return true;
+                    }
                 }
-            } else if (TypeTestUtil.isA(annotationTypeClassName, annotationType)) {
-                return true;
             }
         }
         return false;
     }
 
     private static boolean hasImports(JavaNode node, String className) {
+        return hasImports(node, Collections.singleton(className));
+    }
+
+    private static boolean hasImports(JavaNode node, Set<String> classNames) {
         List<ASTImportDeclaration> imports = node.getRoot().findDescendantsOfType(ASTImportDeclaration.class);
         for (ASTImportDeclaration importDeclaration : imports) {
             ASTName name = importDeclaration.getFirstChildOfType(ASTName.class);
-            if (name != null && name.hasImageEqualTo(className)) {
+            // imports are always fully qualified
+            if (name != null && classNames.contains(name.getImage())) {
                 return true;
             }
         }
         return false;
     }
+
+    /**
+     * Compares the name against the given fully qualified name. If a direct comparison doesn't
+     * work, try to compare as a simple name.
+     *
+     * <p>Note: This method is only used, if the auxclasspath is incomplete.</p>
+     */
+    private static boolean areNamesEqual(ASTName node, String fullyQualifiedAnnotationName) {
+        String simpleName = node.getImage();
+        if (simpleName.equals(fullyQualifiedAnnotationName)) {
+            return true;
+        }
+
+        String simpleAnnotationName = fullyQualifiedAnnotationName;
+        int lastDot = fullyQualifiedAnnotationName.lastIndexOf('.');
+        if (lastDot != -1) {
+            simpleAnnotationName = fullyQualifiedAnnotationName.substring(lastDot + 1);
+        }
+        // when comparing by simple name, double check whether we have a import to avoid false positives
+        return simpleName.equals(simpleAnnotationName) && hasImports(node, fullyQualifiedAnnotationName);
+    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/TestClassWithoutTestCasesRule.java

@@ -5,42 +5,103 @@
 package net.sourceforge.pmd.lang.java.rule.errorprone;
 
 import java.util.List;
+import java.util.regex.Pattern;
 
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBody;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTPackageDeclaration;
 import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.rule.AbstractJUnitRule;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+import net.sourceforge.pmd.properties.PropertyDescriptor;
+import net.sourceforge.pmd.properties.PropertyFactory;
 
 public class TestClassWithoutTestCasesRule extends AbstractJavaRule {
 
+    private static final PropertyDescriptor<Pattern> TEST_CLASS_PATTERN = PropertyFactory.regexProperty("testClassPattern")
+            .defaultValue("^(?:.*\\.)?Test[^\\.]*$|^(?:.*\\.)?.*Tests?$|^(?:.*\\.)?.*TestCase$")
+            .desc("Test class name pattern to identify test classes by their fully qualified name. "
+                    + "A empty pattern disables test class detection by name. Since PMD 6.51.0.")
+            .build();
+
     public TestClassWithoutTestCasesRule() {
         addRuleChainVisit(ASTClassOrInterfaceBody.class);
+        definePropertyDescriptor(TEST_CLASS_PATTERN);
     }
 
     @Override
     public Object visit(ASTClassOrInterfaceBody node, Object data) {
-        if (AbstractJUnitRule.isTestClass(node)) {
+        if (AbstractJUnitRule.isTestClass(node) || AbstractJUnitRule.isJUnit5NestedClass(node) || isTestClassByPattern(node)) {
             List<ASTClassOrInterfaceBodyDeclaration> declarations = node.findChildrenOfType(ASTClassOrInterfaceBodyDeclaration.class);
             int testMethods = 0;
+            int nestedTestClasses = 0;
             for (ASTClassOrInterfaceBodyDeclaration decl : declarations) {
                 if (isTestMethod(decl)) {
                     testMethods++;
+                } else if (isNestedClass(decl)) {
+                    nestedTestClasses++;
                 }
             }
-            if (testMethods == 0) {
-                addViolation(data, node);
+            if (testMethods == 0 && nestedTestClasses == 0) {
+                addViolation(data, node, getSimpleClassName(node));
             }
         }
         return data;
     }
 
+    private String getSimpleClassName(ASTClassOrInterfaceBody node) {
+        JavaNode parent = node.getParent();
+        if (parent instanceof ASTClassOrInterfaceDeclaration) {
+            return ((ASTClassOrInterfaceDeclaration) parent).getSimpleName();
+        }
+        return "<anon>";
+    }
+
+    private boolean isTestClassByPattern(ASTClassOrInterfaceBody node) {
+        Pattern testClassPattern = getProperty(TEST_CLASS_PATTERN);
+        if (testClassPattern.pattern().isEmpty()) {
+            // detection by pattern is disabled
+            return false;
+        }
+
+        ASTClassOrInterfaceDeclaration classDecl = null;
+        if (node.getParent() instanceof ASTClassOrInterfaceDeclaration) {
+            classDecl = (ASTClassOrInterfaceDeclaration) node.getParent();
+        }
+
+        // classDecl can be null in case of anonymous classes or enum constants
+        if (classDecl == null || classDecl.isAbstract() || classDecl.isInterface()) {
+            return false;
+        }
+
+        StringBuilder fullyQualifiedName = new StringBuilder();
+        ASTPackageDeclaration packageDeclaration = classDecl.getRoot().getPackageDeclaration();
+        if (packageDeclaration != null) {
+            fullyQualifiedName.append(packageDeclaration.getName()).append('.');
+        }
+        List<ASTClassOrInterfaceDeclaration> parentClasses = classDecl.getParentsOfType(ASTClassOrInterfaceDeclaration.class);
+        for (int i = parentClasses.size() - 1; i >= 0; i--) {
+            fullyQualifiedName.append(parentClasses.get(i).getSimpleName()).append('.');
+        }
+        fullyQualifiedName.append(classDecl.getSimpleName());
+        return testClassPattern.matcher(fullyQualifiedName).find();
+    }
+
     private boolean isTestMethod(ASTClassOrInterfaceBodyDeclaration decl) {
         JavaNode node = decl.getDeclarationNode();
         if (node instanceof ASTMethodDeclaration) {
             return AbstractJUnitRule.isTestMethod((ASTMethodDeclaration) node);
         }
         return false;
     }
+
+    private boolean isNestedClass(ASTClassOrInterfaceBodyDeclaration decl) {
+        JavaNode node = decl.getParent();
+        if (node instanceof ASTClassOrInterfaceBody) {
+            return AbstractJUnitRule.isJUnit5NestedClass((ASTClassOrInterfaceBody) node);
+        }
+        return false;
+    }
 }

pmd-java/src/main/resources/category/java/errorprone.xml

@@ -3332,12 +3332,16 @@ public void foo() {
     <rule name="TestClassWithoutTestCases"
           language="java"
           since="3.0"
-          message="This class name ends with 'Test' but contains no test cases"
+          message="The class ''{0}'' might be a test class, but it contains no test cases."
           class="net.sourceforge.pmd.lang.java.rule.errorprone.TestClassWithoutTestCasesRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#testclasswithouttestcases">
         <description>
-Test classes end with the suffix Test. Having a non-test class with that name is not a good practice,
-since most people will assume it is a test case. Test classes have test methods named testXXX.
+Test classes typically end with the suffix "Test", "Tests" or "TestCase". Having a non-test class with that name
+is not a good practice, since most people will assume it is a test case. Test classes have test methods
+named "testXXX" (JUnit3) or use annotations (e.g. `@Test`).
+
+The suffix can be configured using the property `testClassPattern`. To disable the detection of possible test classes
+by name, set this property to an empty string.
         </description>
         <priority>3</priority>
         <example>