forked from phoedos/pmd
Merge branch 'apex-commits'
This commit is contained in:
@ -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();
|
||||
}
|
||||
}
|
@ -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<ASTBlockStatement> blockStatements = node.findDescendantsOfType(ASTBlockStatement.class);
|
||||
final List<ASTStatement> statements = Iterables.getOnlyElement(blockStatements).findDescendantsOfType(ASTStatement.class);
|
||||
boolean isAssertFound = false;
|
||||
|
||||
for (final ASTStatement statement : statements) {
|
||||
final List<ASTMethodCallExpression> 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;
|
||||
}
|
||||
}
|
@ -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;
|
||||
|
||||
/**
|
||||
* <p>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.</p>
|
||||
*
|
||||
* @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;
|
||||
}
|
||||
}
|
58
pmd-apex/src/main/resources/rulesets/apex/apexunit.xml
Normal file
58
pmd-apex/src/main/resources/rulesets/apex/apexunit.xml
Normal file
@ -0,0 +1,58 @@
|
||||
<?xml version="1.0"?>
|
||||
|
||||
<ruleset name="ApexUnit"
|
||||
xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
|
||||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
|
||||
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
|
||||
<description>
|
||||
These rules deal with different problems that can occur with Apex unit tests.
|
||||
</description>
|
||||
|
||||
<rule name="ApexUnitTestClassShouldHaveAsserts"
|
||||
since="2.0"
|
||||
message="Apex unit tests should System.assert() or assertEquals() or assertNotEquals()"
|
||||
class="net.sourceforge.pmd.lang.apex.rule.apexunit.ApexUnitTestClassShouldHaveAsserts"
|
||||
externalInfoUrl="${pmd.website.baseurl}/rules/apex/apexunit.html#ApexUnitTestClassShouldHaveAsserts">
|
||||
<description>
|
||||
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.
|
||||
</description>
|
||||
<priority>3</priority>
|
||||
<example>
|
||||
<![CDATA[
|
||||
@isTest
|
||||
public class Foo {
|
||||
public static testMethod void testSomething() {
|
||||
Account a = null;
|
||||
// This is better than having a NullPointerException
|
||||
// System.assertNotEquals(a, null, 'account not found');
|
||||
a.toString();
|
||||
}
|
||||
}
|
||||
]]>
|
||||
</example>
|
||||
</rule>
|
||||
<rule name="ApexUnitTestShouldNotUseSeeAllDataTrue"
|
||||
since="2.0"
|
||||
message="Apex unit tests should not use @isTest(seeAllData = true)"
|
||||
class="net.sourceforge.pmd.lang.apex.rule.apexunit.ApexUnitTestShouldNotUseSeeAllDataTrue"
|
||||
externalInfoUrl="${pmd.website.baseurl}/rules/apex/apexunit.html#ApexUnitTestShouldNotUseSeeAllDataTrue">
|
||||
<description>
|
||||
Apex unit tests should not use @isTest(seeAllData=true) because it opens up the existing database data for unexpected modification by tests.
|
||||
</description>
|
||||
<priority>3</priority>
|
||||
<example>
|
||||
<![CDATA[
|
||||
@isTest(seeAllData = true)
|
||||
public class Foo {
|
||||
public static testMethod void testSomething() {
|
||||
Account a = null;
|
||||
// This is better than having a NullPointerException
|
||||
// System.assertNotEquals(a, null, 'account not found');
|
||||
a.toString();
|
||||
}
|
||||
}
|
||||
]]>
|
||||
</example>
|
||||
</rule>
|
||||
</ruleset>
|
@ -146,7 +146,29 @@
|
||||
<property name="cc_block_highlighting" value="false"/>
|
||||
</properties>
|
||||
</rule>
|
||||
|
||||
|
||||
<rule ref="rulesets/apex/apexunit.xml/ApexUnitTestClassShouldHaveAsserts" message="Apex unit test classes should have at least one System.assert() or assertEquals() or AssertNotEquals() call">
|
||||
<priority>3</priority>
|
||||
|
||||
<properties>
|
||||
<!-- relevant for Code Climate output only -->
|
||||
<property name="cc_categories" value="Bug Risk"/>
|
||||
<property name="cc_remediation_points_multiplier" value="100"/>
|
||||
<property name="cc_block_highlighting" value="false"/>
|
||||
</properties>
|
||||
</rule>
|
||||
|
||||
<rule ref="rulesets/apex/apexunit.xml/ApexUnitTestShouldNotUseSeeAllDataTrue" message="@isTest(seeAllData=true) should not be used in Apex unit tests because it opens up the existing database data for unexpected modification by tests">
|
||||
<priority>3</priority>
|
||||
|
||||
<properties>
|
||||
<!-- relevant for Code Climate output only -->
|
||||
<property name="cc_categories" value="Bug Risk"/>
|
||||
<property name="cc_remediation_points_multiplier" value="100"/>
|
||||
<property name="cc_block_highlighting" value="false"/>
|
||||
</properties>
|
||||
</rule>
|
||||
|
||||
<rule ref="rulesets/apex/style.xml/AvoidLogicInTrigger" message="Avoid logic in triggers">
|
||||
<priority>3</priority>
|
||||
|
||||
|
@ -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
|
||||
|
@ -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");
|
||||
}
|
||||
}
|
@ -0,0 +1,19 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<test-data>
|
||||
|
||||
<test-code>
|
||||
<description>Problematic apex unit test - no assert calls</description>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code><![CDATA[
|
||||
@isTest
|
||||
public class Foo {
|
||||
public static testMethod void testSomething() {
|
||||
Account a = null;
|
||||
// This is better than having a NullPointerException
|
||||
// System.assertNotEquals(a, null, 'account not found');
|
||||
a.toString();
|
||||
}
|
||||
}
|
||||
]]></code>
|
||||
</test-code>
|
||||
</test-data>
|
@ -0,0 +1,32 @@
|
||||
<test-data>
|
||||
|
||||
<test-code>
|
||||
<description>Problematic apex unit test class - uses SeeAllData = true</description>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code><![CDATA[
|
||||
@isTest (seeAllData = true)
|
||||
public class Foo {
|
||||
public static testMethod void testSomething() {
|
||||
Account a = null;
|
||||
// This is better than having a NullPointerException
|
||||
System.assertNotEquals(a, null, 'account not found');
|
||||
}
|
||||
}
|
||||
]]></code>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>Problematic apex unit test method - uses SeeAllData = true</description>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code><![CDATA[
|
||||
@isTest
|
||||
public class Foo {
|
||||
@isTest (seeAllData = true)
|
||||
public static void testSomething() {
|
||||
Account a = null;
|
||||
// This is better than having a NullPointerException
|
||||
System.assertNotEquals(a, null, 'account not found');
|
||||
}
|
||||
}
|
||||
]]></code>
|
||||
</test-code>
|
||||
</test-data>
|
@ -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:**
|
||||
|
||||
|
Reference in New Issue
Block a user