diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/junit/JUnitTestsShouldIncludeAssertRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/junit/JUnitTestsShouldIncludeAssertRule.java index c3ce9b7280..9056cc65ea 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/junit/JUnitTestsShouldIncludeAssertRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/junit/JUnitTestsShouldIncludeAssertRule.java @@ -1,98 +1,163 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ -package net.sourceforge.pmd.lang.java.rule.junit; - -import java.util.List; - -import net.sourceforge.pmd.lang.ast.Node; -import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTMemberValuePair; -import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTName; -import net.sourceforge.pmd.lang.java.ast.ASTNormalAnnotation; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; -import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; - -public class JUnitTestsShouldIncludeAssertRule extends AbstractJUnitRule { - - @Override - public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { - if (node.isInterface()) { - return data; - } - return super.visit(node, data); - } - - @Override - public Object visit(ASTMethodDeclaration method, Object data) { - if (isJUnitMethod(method, data)) { - if (!containsAssert(method.getBlock(), false) && !containsExpect(method.jjtGetParent())) { - addViolation(data, method); - } - } - return data; - } - - private boolean containsAssert(Node n, boolean assertFound) { - if (!assertFound) { - if (n instanceof ASTStatementExpression) { - if (isAssertOrFailStatement((ASTStatementExpression) n)) { - return true; - } - } - if (!assertFound) { - for (int i = 0; i < n.jjtGetNumChildren() && !assertFound; i++) { - Node c = n.jjtGetChild(i); - if (containsAssert(c, assertFound)) { - return true; - } - } - } - } - return false; - } - - /** - * Tells if the node contains a Test annotation with an expected exception. - */ - private boolean containsExpect(Node methodParent) { - List annotations = methodParent.findDescendantsOfType(ASTNormalAnnotation.class); - for (ASTNormalAnnotation annotation : annotations) { - ASTName name = annotation.getFirstChildOfType(ASTName.class); - if (name != null - && ("Test".equals(name.getImage()) || name.getType() != null && name.getType().equals(JUNIT4_CLASS))) { - List memberValues = annotation.findDescendantsOfType(ASTMemberValuePair.class); - for (ASTMemberValuePair pair : memberValues) { - if ("expected".equals(pair.getImage())) { - return true; - } - } - } - } - return false; - } - - /** - * Tells if the expression is an assert statement or not. - */ - private boolean isAssertOrFailStatement(ASTStatementExpression expression) { - if (expression != null && expression.jjtGetNumChildren() > 0 - && expression.jjtGetChild(0) instanceof ASTPrimaryExpression) { - ASTPrimaryExpression pe = (ASTPrimaryExpression) expression.jjtGetChild(0); - if (pe.jjtGetNumChildren() > 0 && pe.jjtGetChild(0) instanceof ASTPrimaryPrefix) { - ASTPrimaryPrefix pp = (ASTPrimaryPrefix) pe.jjtGetChild(0); - if (pp.jjtGetNumChildren() > 0 && pp.jjtGetChild(0) instanceof ASTName) { - String img = ((ASTName) pp.jjtGetChild(0)).getImage(); - if (img != null - && (img.startsWith("assert") || img.startsWith("fail") || img.startsWith("Assert.assert") || img - .startsWith("Assert.fail"))) { - return true; - } - } - } - } - return false; - } -} +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.junit; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTMarkerAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTMemberValuePair; +import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTNormalAnnotation; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; +import net.sourceforge.pmd.lang.java.ast.ASTReferenceType; +import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; +import net.sourceforge.pmd.lang.symboltable.NameDeclaration; +import net.sourceforge.pmd.lang.symboltable.NameOccurrence; +import net.sourceforge.pmd.lang.symboltable.Scope; + +public class JUnitTestsShouldIncludeAssertRule extends AbstractJUnitRule { + + @Override + public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { + if (node.isInterface()) { + return data; + } + return super.visit(node, data); + } + + @Override + public Object visit(ASTMethodDeclaration method, Object data) { + if (isJUnitMethod(method, data)) { + if (!isExpectAnnotated(method.jjtGetParent())) { + Scope classScope = method.getScope().getParent(); + Map> expectables = getRuleAnnotatedExpectedExceptions(classScope); + + if (!containsExpectOrAssert(method.getBlock(), expectables)) { + addViolation(data, method); + } + } + } + return data; + } + + private boolean containsExpectOrAssert(Node n, Map> expectables) { + if (n instanceof ASTStatementExpression) { + if (isExpectStatement((ASTStatementExpression) n, expectables) + || isAssertOrFailStatement((ASTStatementExpression) n)) { + return true; + } + } else { + for (int i = 0; i < n.jjtGetNumChildren(); i++) { + Node c = n.jjtGetChild(i); + if (containsExpectOrAssert(c, expectables)) { + return true; + } + } + } + return false; + } + + /** + * Gets a list of NameDeclarations for all the fields that have type + * ExpectedException and have a Rule annotation. + * + * @param classScope + * The class scope to search for + * @return See description + */ + private Map> getRuleAnnotatedExpectedExceptions(Scope classScope) { + Map> result = new HashMap<>(); + Map> decls = classScope.getDeclarations(); + + for (NameDeclaration decl : decls.keySet()) { + Node parent = decl.getNode().jjtGetParent().jjtGetParent().jjtGetParent(); + if (parent.hasDescendantOfType(ASTAnnotation.class) + && parent.getFirstChildOfType(ASTFieldDeclaration.class) != null) { + String annot = parent.getFirstDescendantOfType(ASTMarkerAnnotation.class).jjtGetChild(0).getImage(); + if (!"Rule".equals(annot) && !"org.junit.Rule".equals(annot)) { + continue; + } + + Node type = parent.getFirstDescendantOfType(ASTReferenceType.class); + if (!"ExpectedException".equals(type.jjtGetChild(0).getImage())) { + continue; + } + result.put(decl.getName(), decls.get(decl)); + } + } + return result; + } + + /** + * Tells if the node contains a Test annotation with an expected exception. + */ + private boolean isExpectAnnotated(Node methodParent) { + List annotations = methodParent.findDescendantsOfType(ASTNormalAnnotation.class); + for (ASTNormalAnnotation annotation : annotations) { + ASTName name = annotation.getFirstChildOfType(ASTName.class); + if (name != null && ("Test".equals(name.getImage()) + || name.getType() != null && name.getType().equals(JUNIT4_CLASS))) { + List memberValues = annotation.findDescendantsOfType(ASTMemberValuePair.class); + for (ASTMemberValuePair pair : memberValues) { + if ("expected".equals(pair.getImage())) { + return true; + } + } + } + } + return false; + } + + /** + * Tells if the expression is an assert statement or not. + */ + private boolean isAssertOrFailStatement(ASTStatementExpression expression) { + if (expression != null) { + ASTPrimaryExpression pe = expression.getFirstChildOfType(ASTPrimaryExpression.class); + if (pe != null) { + String img = pe.jjtGetChild(0).jjtGetChild(0).getImage(); + if (img != null && (img.startsWith("assert") || img.startsWith("fail") + || img.startsWith("Assert.assert") || img.startsWith("Assert.fail"))) { + return true; + } + } + } + return false; + } + + private boolean isExpectStatement(ASTStatementExpression expression, + Map> expectables) { + + if (expression != null) { + ASTPrimaryExpression pe = expression.getFirstChildOfType(ASTPrimaryExpression.class); + if (pe != null) { + String img = pe.jjtGetChild(0).jjtGetChild(0).getImage(); + if (img.indexOf(".") == -1) { + return false; + } + String varname = img.split("\\.")[0]; + + if (!expectables.containsKey(varname)) { + return false; + } + + for (NameOccurrence occ : expectables.get(varname)) { + if (occ.getLocation() == pe.jjtGetChild(0).jjtGetChild(0) && img.startsWith(varname + ".expect")) { + return true; + } + } + } + } + return false; + } +} + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/junit/xml/JUnitTestsShouldIncludeAssert.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/junit/xml/JUnitTestsShouldIncludeAssert.xml index bec384fb77..4ef280593f 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/junit/xml/JUnitTestsShouldIncludeAssert.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/junit/xml/JUnitTestsShouldIncludeAssert.xml @@ -271,4 +271,45 @@ public class Foo_Test } ]]> + + #285 Issues with @Rule annotation and ExpectedException + 1 + 7 + + + + #285 Follow-up for @org.junit.Rule + 0 + + diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 559ddfbe5e..6338418204 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -23,6 +23,8 @@ The PMD team is pleased to announce PMD 5.5.5. * [#275](https://github.com/pmd/pmd/issues/275): \[java] FinalFieldCouldBeStatic: Constant in @interface incorrectly reported as "could be made static" * [#282](https://github.com/pmd/pmd/issues/282): \[java] UnnecessaryLocalBeforeReturn false positive when cloning Maps * [#291](https://github.com/pmd/pmd/issues/291): \[java] Improve quality of AccessorClassGeneration +* java-junit + * [#285](https://github.com/pmd/pmd/issues/285): \[java] JUnitTestsShouldIncludeAssertRule should support @Rule as well as @Test(expected = ...) * java-optimizations: * [#222](https://github.com/pmd/pmd/issues/222): \[java] UseStringBufferForStringAppends: False Positive with ternary operator * java-strings: @@ -37,4 +39,5 @@ The PMD team is pleased to announce PMD 5.5.5. * [#296](https://github.com/pmd/pmd/pull/296): \[apex] Adding String.IsNotBlank to the whitelist to prevent False positives * [#303](https://github.com/pmd/pmd/pull/303): \[java] InefficientEmptyStringCheckRule now reports String.trim().isEmpty() * [#307](https://github.com/pmd/pmd/pull/307): \[java] Fix false positive with UseStringBufferForStringAppendsRule +* [#308](https://github.com/pmd/pmd/pull/308): \[java] JUnitTestsShouldIncludeAssertRule supports @Rule annotated ExpectedExceptions