diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/AbstractApexUnitTestRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/AbstractApexUnitTestRule.java new file mode 100644 index 0000000000..aaf58a9f7b --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/AbstractApexUnitTestRule.java @@ -0,0 +1,38 @@ +package net.sourceforge.pmd.lang.apex.rule.apexunit; + +import net.sourceforge.pmd.lang.apex.ast.*; +import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; + +import apex.jorje.services.Version; + +/** + * Do special checks for apex unit test classes and methods + * @author a.subramanian + */ +public abstract class AbstractApexUnitTestRule extends AbstractApexRule{ + + public AbstractApexUnitTestRule() { + setProperty(CODECLIMATE_CATEGORIES, new String[]{ "Bug Risk" }); + setProperty(CODECLIMATE_REMEDIATION_MULTIPLIER, 100); + setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false); + } + + /** + * Don't bother visiting this class if it's not a class with @isTest and newer than API v24 + */ + @Override + public Object visit(final ASTUserClass node,final Object data) { + final Version classApiVersion = node.getNode().getDefiningType().getCodeUnitDetails().getVersion(); + if (!isTestMethodOrClass(node) + && classApiVersion.isGreaterThan(Version.V174)) { + return data; + } + return super.visit(node, data); + } + + boolean isTestMethodOrClass(final ApexNode node) { + final ASTModifierNode modifierNode = node.getFirstChildOfType(ASTModifierNode.class); + return modifierNode != null + && modifierNode.getNode().getModifiers().isTest(); + } +} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitTestClassShouldHaveAsserts.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitTestClassShouldHaveAsserts.java new file mode 100644 index 0000000000..927f5624b8 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitTestClassShouldHaveAsserts.java @@ -0,0 +1,56 @@ +package net.sourceforge.pmd.lang.apex.rule.apexunit; + +import java.util.List; + +import com.google.common.collect.Iterables; + +import net.sourceforge.pmd.lang.apex.ast.*; + +/** + * Apex unit tests should have System.assert methods in them + * + * @author a.subramanian + */ +public class ApexUnitTestClassShouldHaveAsserts extends AbstractApexUnitTestRule { + + private static final String SYSTEM = "System"; + private static final String ASSERT = "assert"; + private static final String ASSERT_EQUALS = "assertEquals"; + private static final String ASSERT_NOT_EQUALS = "assertNotEquals"; + + @Override + public Object visit(ASTMethod node, Object data) { + if (!isTestMethodOrClass(node)) { + return data; + } + + return checkForAssertStatements(node, data); + } + + private Object checkForAssertStatements(ApexNode node, Object data) { + final List blockStatements = node.findDescendantsOfType(ASTBlockStatement.class); + final List statements = Iterables.getOnlyElement(blockStatements).findDescendantsOfType(ASTStatement.class); + boolean isAssertFound = false; + + for (final ASTStatement statement : statements) { + final List methodCalls = statement.findDescendantsOfType(ASTMethodCallExpression.class); + + for (final ASTMethodCallExpression methodCallExpression : methodCalls) { + final String methodName = methodCallExpression.getNode().getMethod().getName(); + + if (methodCallExpression.getNode().getDefiningType().getApexName().equalsIgnoreCase(SYSTEM) + && (methodName.equalsIgnoreCase(ASSERT) + || methodName.equalsIgnoreCase(ASSERT_EQUALS) + || methodName.equalsIgnoreCase(ASSERT_NOT_EQUALS))) { + isAssertFound = true; + } + } + } + + if (!isAssertFound) { + addViolation(data, node); + } + + return data; + } +} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitTestShouldNotUseSeeAllDataTrue.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitTestShouldNotUseSeeAllDataTrue.java new file mode 100644 index 0000000000..c8c8dbdd2b --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitTestShouldNotUseSeeAllDataTrue.java @@ -0,0 +1,60 @@ +package net.sourceforge.pmd.lang.apex.rule.apexunit; + +import net.sourceforge.pmd.lang.apex.ast.*; + +import apex.jorje.semantic.ast.modifier.*; +import apex.jorje.semantic.symbol.type.AnnotationTypeInfos; +import apex.jorje.semantic.symbol.type.TypeInfoEquivalence; +import apex.jorje.services.Version; + +/** + *

It's a very bad practice to use @isTest(seeAllData=true) in Apex unit tests, + * because it opens up the existing database data for unexpected modification by tests.

+ * + * @author a.subramanian + */ +public class ApexUnitTestShouldNotUseSeeAllDataTrue extends AbstractApexUnitTestRule { + + @Override + public Object visit(final ASTUserClass node, final Object data) { + // @isTest(seeAllData) was introduced in v24, and was set to false by default + final Version classApiVersion = node.getNode().getDefiningType().getCodeUnitDetails().getVersion(); + + if (!isTestMethodOrClass(node) + && classApiVersion.isGreaterThan(Version.V174)) { + return data; + } + + checkForSeeAllData(node, data); + return super.visit(node, data); + } + + @Override + public Object visit(ASTMethod node, Object data) { + if (!isTestMethodOrClass(node)) { + return data; + } + + return checkForSeeAllData(node, data); + } + + private Object checkForSeeAllData(final ApexNode node, final Object data) { + final ASTModifierNode modifierNode = node.getFirstChildOfType(ASTModifierNode.class); + + if (modifierNode != null) { + for(final ModifierOrAnnotation modifierOrAnnotation : modifierNode.getNode().getModifiers().allNodes()) { + if (modifierOrAnnotation instanceof Annotation && TypeInfoEquivalence.isEquivalent(modifierOrAnnotation.getType(), AnnotationTypeInfos.IS_TEST)) { + final Annotation annotation = (Annotation) modifierOrAnnotation; + final AnnotationParameter parameter = annotation.getParameter("seeAllData"); + + if (parameter != null && parameter.getBooleanValue() == true) { + addViolation(data, node); + return data; + } + } + } + } + + return data; + } +} diff --git a/pmd-apex/src/main/resources/rulesets/apex/apexunit.xml b/pmd-apex/src/main/resources/rulesets/apex/apexunit.xml new file mode 100644 index 0000000000..ddaa26d05c --- /dev/null +++ b/pmd-apex/src/main/resources/rulesets/apex/apexunit.xml @@ -0,0 +1,58 @@ + + + + + These rules deal with different problems that can occur with Apex unit tests. + + + + + Apex unit tests should include at least one assertion. This makes the tests more robust, and using assert + with messages provide the developer a clearer idea of what the test does. + + 3 + + + + + + + Apex unit tests should not use @isTest(seeAllData=true) because it opens up the existing database data for unexpected modification by tests. + + 3 + + + + + diff --git a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml index 6f817c148a..236c07fb54 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml +++ b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml @@ -146,7 +146,29 @@ - + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 diff --git a/pmd-apex/src/main/resources/rulesets/apex/rulesets.properties b/pmd-apex/src/main/resources/rulesets/apex/rulesets.properties index 50a8d972ff..5c5a2fdb8a 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/rulesets.properties +++ b/pmd-apex/src/main/resources/rulesets/apex/rulesets.properties @@ -2,4 +2,4 @@ # BSD-style license; for more info see http://pmd.sourceforge.net/license.html # -rulesets.filenames=rulesets/apex/complexity.xml,rulesets/apex/performance.xml,rulesets/apex/style.xml +rulesets.filenames=rulesets/apex/complexity.xml,rulesets/apex/performance.xml,rulesets/apex/style.xml,rulesets/apex/apexunit.xml diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitRulesTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitRulesTest.java new file mode 100644 index 0000000000..66ff070ebd --- /dev/null +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/apexunit/ApexUnitRulesTest.java @@ -0,0 +1,17 @@ +package net.sourceforge.pmd.lang.apex.rule.apexunit; + +import net.sourceforge.pmd.testframework.SimpleAggregatorTst; + +/** + * @author a.subramanian + */ +public class ApexUnitRulesTest extends SimpleAggregatorTst { + + private static final String RULESET = "apex-apexunit"; + + @Override + public void setUp() { + addRule(RULESET, "ApexUnitTestClassShouldHaveAsserts"); + addRule(RULESET, "ApexUnitTestShouldNotUseSeeAllDataTrue"); + } +} diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/apexunit/xml/ApexUnitTestClassShouldHaveAsserts.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/apexunit/xml/ApexUnitTestClassShouldHaveAsserts.xml new file mode 100644 index 0000000000..9b87884fdb --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/apexunit/xml/ApexUnitTestClassShouldHaveAsserts.xml @@ -0,0 +1,19 @@ + + + + + Problematic apex unit test - no assert calls + 1 + + + \ No newline at end of file diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/apexunit/xml/ApexUnitTestShouldNotUseSeeAllDataTrue.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/apexunit/xml/ApexUnitTestShouldNotUseSeeAllDataTrue.xml new file mode 100644 index 0000000000..81ef9c478e --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/apexunit/xml/ApexUnitTestShouldNotUseSeeAllDataTrue.xml @@ -0,0 +1,32 @@ + + + + Problematic apex unit test class - uses SeeAllData = true + 1 + + + + Problematic apex unit test method - uses SeeAllData = true + 1 + + + \ No newline at end of file diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index e2272a1a23..dd1f667fe8 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -14,6 +14,7 @@ * [#102](https://github.com/pmd/pmd/pull/102): \[apex] Restrict AvoidLogicInTrigger rule to max. 1 violation per file * [#103](https://github.com/pmd/pmd/pull/103): \[java] \[apex] Fix for 1501: CyclomaticComplexity rule causes OOM when class reporting is disabled * [#104](https://github.com/pmd/pmd/pull/104): \[core] \[java] Close opened file handles +* [apex #43](https://github.com/Up2Go/pmd/pull/43): \[apex] Basic apex unit test rules **Bugfixes:**