From de8f5b372ae551c279505ea5426f8f421b2b931c Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sat, 23 Feb 2019 21:38:31 -0800 Subject: [PATCH 01/10] apex rules for test method and assert statements --- docs/pages/pmd/rules/apex.md | 4 +- docs/pages/pmd/rules/apex/bestpractices.md | 80 ++++++++++++++++++- .../ApexUnitTestAssertStatementRule.java | 65 +++++++++++++++ ...tMethodShouldHaveIsTestAnnotationRule.java | 78 ++++++++++++++++++ .../resources/category/apex/bestpractices.xml | 50 ++++++++++++ .../main/resources/rulesets/apex/ruleset.xml | 18 +++++ .../ApexUnitTestAssertStatementTest.java | 11 +++ ...tMethodShouldHaveIsTestAnnotationTest.java | 11 +++ .../xml/ApexUnitTestAssertStatement.xml | 38 +++++++++ ...itTestMethodShouldHaveIsTestAnnotation.xml | 25 ++++++ 10 files changed, 378 insertions(+), 2 deletions(-) create mode 100644 pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java create mode 100644 pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java create mode 100644 pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java create mode 100644 pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationTest.java create mode 100644 pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml create mode 100644 pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml diff --git a/docs/pages/pmd/rules/apex.md b/docs/pages/pmd/rules/apex.md index 1f59b133e4..ff62b88ef4 100644 --- a/docs/pages/pmd/rules/apex.md +++ b/docs/pages/pmd/rules/apex.md @@ -11,7 +11,9 @@ folder: pmd/rules {% include callout.html content="Rules which enforce generally accepted best practices." %} +* [ApexUnitTestAssertStatement](pmd_rules_apex_bestpractices.html#apexunittestassertstatement): Apex test assert should have 2 params and assertEquals/assertNotEquals should have 3 params. * [ApexUnitTestClassShouldHaveAsserts](pmd_rules_apex_bestpractices.html#apexunittestclassshouldhaveasserts): Apex unit tests should include at least one assertion. This makes the tests more robust, and usi... +* [ApexUnitTestMethodShouldHaveIsTestAnnotation](pmd_rules_apex_bestpractices.html#apexunittestmethodshouldhaveistestannotation): Apex test methods should have @isTest annotation either in the same line or in previous line. * [ApexUnitTestShouldNotUseSeeAllDataTrue](pmd_rules_apex_bestpractices.html#apexunittestshouldnotuseseealldatatrue): Apex unit tests should not use @isTest(seeAllData=true) because it opens up the existing database... * [AvoidGlobalModifier](pmd_rules_apex_bestpractices.html#avoidglobalmodifier): Global classes should be avoided (especially in managed packages) as they can never be deleted or... * [AvoidLogicInTrigger](pmd_rules_apex_bestpractices.html#avoidlogicintrigger): As triggers do not allow methods like regular classes they are less flexible and suited to apply ... @@ -120,7 +122,7 @@ folder: pmd/rules It contains the following rules: - [ApexBadCrypto](pmd_rules_apex_security.html#apexbadcrypto), [ApexCRUDViolation](pmd_rules_apex_security.html#apexcrudviolation), [ApexCSRF](pmd_rules_apex_security.html#apexcsrf), [ApexDangerousMethods](pmd_rules_apex_security.html#apexdangerousmethods), [ApexDoc](pmd_rules_apex_documentation.html#apexdoc), [ApexInsecureEndpoint](pmd_rules_apex_security.html#apexinsecureendpoint), [ApexOpenRedirect](pmd_rules_apex_security.html#apexopenredirect), [ApexSharingViolations](pmd_rules_apex_security.html#apexsharingviolations), [ApexSOQLInjection](pmd_rules_apex_security.html#apexsoqlinjection), [ApexSuggestUsingNamedCred](pmd_rules_apex_security.html#apexsuggestusingnamedcred), [ApexUnitTestClassShouldHaveAsserts](pmd_rules_apex_bestpractices.html#apexunittestclassshouldhaveasserts), [ApexUnitTestShouldNotUseSeeAllDataTrue](pmd_rules_apex_bestpractices.html#apexunittestshouldnotuseseealldatatrue), [ApexXSSFromEscapeFalse](pmd_rules_apex_security.html#apexxssfromescapefalse), [ApexXSSFromURLParam](pmd_rules_apex_security.html#apexxssfromurlparam), [AvoidDeeplyNestedIfStmts](pmd_rules_apex_design.html#avoiddeeplynestedifstmts), [AvoidDirectAccessTriggerMap](pmd_rules_apex_errorprone.html#avoiddirectaccesstriggermap), [AvoidDmlStatementsInLoops](pmd_rules_apex_performance.html#avoiddmlstatementsinloops), [AvoidGlobalModifier](pmd_rules_apex_bestpractices.html#avoidglobalmodifier), [AvoidHardcodingId](pmd_rules_apex_errorprone.html#avoidhardcodingid), [AvoidLogicInTrigger](pmd_rules_apex_bestpractices.html#avoidlogicintrigger), [AvoidNonExistentAnnotations](pmd_rules_apex_errorprone.html#avoidnonexistentannotations), [AvoidSoqlInLoops](pmd_rules_apex_performance.html#avoidsoqlinloops), [AvoidSoslInLoops](pmd_rules_apex_performance.html#avoidsoslinloops), [ClassNamingConventions](pmd_rules_apex_codestyle.html#classnamingconventions), [CyclomaticComplexity](pmd_rules_apex_design.html#cyclomaticcomplexity), [EmptyCatchBlock](pmd_rules_apex_errorprone.html#emptycatchblock), [EmptyIfStmt](pmd_rules_apex_errorprone.html#emptyifstmt), [EmptyStatementBlock](pmd_rules_apex_errorprone.html#emptystatementblock), [EmptyTryOrFinallyBlock](pmd_rules_apex_errorprone.html#emptytryorfinallyblock), [EmptyWhileStmt](pmd_rules_apex_errorprone.html#emptywhilestmt), [ExcessiveClassLength](pmd_rules_apex_design.html#excessiveclasslength), [ExcessiveParameterList](pmd_rules_apex_design.html#excessiveparameterlist), [ExcessivePublicCount](pmd_rules_apex_design.html#excessivepubliccount), [ForLoopsMustUseBraces](pmd_rules_apex_codestyle.html#forloopsmustusebraces), [IfElseStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifelsestmtsmustusebraces), [IfStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifstmtsmustusebraces), [MethodNamingConventions](pmd_rules_apex_codestyle.html#methodnamingconventions), [MethodWithSameNameAsEnclosingClass](pmd_rules_apex_errorprone.html#methodwithsamenameasenclosingclass), [NcssConstructorCount](pmd_rules_apex_design.html#ncssconstructorcount), [NcssMethodCount](pmd_rules_apex_design.html#ncssmethodcount), [NcssTypeCount](pmd_rules_apex_design.html#ncsstypecount), [OneDeclarationPerLine](pmd_rules_apex_codestyle.html#onedeclarationperline), [StdCyclomaticComplexity](pmd_rules_apex_design.html#stdcyclomaticcomplexity), [TooManyFields](pmd_rules_apex_design.html#toomanyfields), [VariableNamingConventions](pmd_rules_apex_codestyle.html#variablenamingconventions), [WhileLoopsMustUseBraces](pmd_rules_apex_codestyle.html#whileloopsmustusebraces) + [ApexBadCrypto](pmd_rules_apex_security.html#apexbadcrypto), [ApexCRUDViolation](pmd_rules_apex_security.html#apexcrudviolation), [ApexCSRF](pmd_rules_apex_security.html#apexcsrf), [ApexDangerousMethods](pmd_rules_apex_security.html#apexdangerousmethods), [ApexDoc](pmd_rules_apex_documentation.html#apexdoc), [ApexInsecureEndpoint](pmd_rules_apex_security.html#apexinsecureendpoint), [ApexOpenRedirect](pmd_rules_apex_security.html#apexopenredirect), [ApexSharingViolations](pmd_rules_apex_security.html#apexsharingviolations), [ApexSOQLInjection](pmd_rules_apex_security.html#apexsoqlinjection), [ApexSuggestUsingNamedCred](pmd_rules_apex_security.html#apexsuggestusingnamedcred), [ApexUnitTestAssertStatement](pmd_rules_apex_bestpractices.html#apexunittestassertstatement), [ApexUnitTestClassShouldHaveAsserts](pmd_rules_apex_bestpractices.html#apexunittestclassshouldhaveasserts), [ApexUnitTestMethodShouldHaveIsTestAnnotation](pmd_rules_apex_bestpractices.html#apexunittestmethodshouldhaveistestannotation), [ApexUnitTestShouldNotUseSeeAllDataTrue](pmd_rules_apex_bestpractices.html#apexunittestshouldnotuseseealldatatrue), [ApexXSSFromEscapeFalse](pmd_rules_apex_security.html#apexxssfromescapefalse), [ApexXSSFromURLParam](pmd_rules_apex_security.html#apexxssfromurlparam), [AvoidDeeplyNestedIfStmts](pmd_rules_apex_design.html#avoiddeeplynestedifstmts), [AvoidDirectAccessTriggerMap](pmd_rules_apex_errorprone.html#avoiddirectaccesstriggermap), [AvoidDmlStatementsInLoops](pmd_rules_apex_performance.html#avoiddmlstatementsinloops), [AvoidGlobalModifier](pmd_rules_apex_bestpractices.html#avoidglobalmodifier), [AvoidHardcodingId](pmd_rules_apex_errorprone.html#avoidhardcodingid), [AvoidLogicInTrigger](pmd_rules_apex_bestpractices.html#avoidlogicintrigger), [AvoidNonExistentAnnotations](pmd_rules_apex_errorprone.html#avoidnonexistentannotations), [AvoidSoqlInLoops](pmd_rules_apex_performance.html#avoidsoqlinloops), [AvoidSoslInLoops](pmd_rules_apex_performance.html#avoidsoslinloops), [ClassNamingConventions](pmd_rules_apex_codestyle.html#classnamingconventions), [CyclomaticComplexity](pmd_rules_apex_design.html#cyclomaticcomplexity), [EmptyCatchBlock](pmd_rules_apex_errorprone.html#emptycatchblock), [EmptyIfStmt](pmd_rules_apex_errorprone.html#emptyifstmt), [EmptyStatementBlock](pmd_rules_apex_errorprone.html#emptystatementblock), [EmptyTryOrFinallyBlock](pmd_rules_apex_errorprone.html#emptytryorfinallyblock), [EmptyWhileStmt](pmd_rules_apex_errorprone.html#emptywhilestmt), [ExcessiveClassLength](pmd_rules_apex_design.html#excessiveclasslength), [ExcessiveParameterList](pmd_rules_apex_design.html#excessiveparameterlist), [ExcessivePublicCount](pmd_rules_apex_design.html#excessivepubliccount), [ForLoopsMustUseBraces](pmd_rules_apex_codestyle.html#forloopsmustusebraces), [IfElseStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifelsestmtsmustusebraces), [IfStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifstmtsmustusebraces), [MethodNamingConventions](pmd_rules_apex_codestyle.html#methodnamingconventions), [MethodWithSameNameAsEnclosingClass](pmd_rules_apex_errorprone.html#methodwithsamenameasenclosingclass), [NcssConstructorCount](pmd_rules_apex_design.html#ncssconstructorcount), [NcssMethodCount](pmd_rules_apex_design.html#ncssmethodcount), [NcssTypeCount](pmd_rules_apex_design.html#ncsstypecount), [OneDeclarationPerLine](pmd_rules_apex_codestyle.html#onedeclarationperline), [StdCyclomaticComplexity](pmd_rules_apex_design.html#stdcyclomaticcomplexity), [TooManyFields](pmd_rules_apex_design.html#toomanyfields), [VariableNamingConventions](pmd_rules_apex_codestyle.html#variablenamingconventions), [WhileLoopsMustUseBraces](pmd_rules_apex_codestyle.html#whileloopsmustusebraces) * Empty Code (`rulesets/apex/empty.xml`): diff --git a/docs/pages/pmd/rules/apex/bestpractices.md b/docs/pages/pmd/rules/apex/bestpractices.md index 82e42195f4..80fb834f1e 100644 --- a/docs/pages/pmd/rules/apex/bestpractices.md +++ b/docs/pages/pmd/rules/apex/bestpractices.md @@ -5,10 +5,48 @@ permalink: pmd_rules_apex_bestpractices.html folder: pmd/rules/apex sidebaractiveurl: /pmd_rules_apex.html editmepath: ../pmd-apex/src/main/resources/category/apex/bestpractices.xml -keywords: Best Practices, ApexUnitTestClassShouldHaveAsserts, ApexUnitTestShouldNotUseSeeAllDataTrue, AvoidGlobalModifier, AvoidLogicInTrigger +keywords: Best Practices, ApexUnitTestClassShouldHaveAsserts, ApexUnitTestShouldNotUseSeeAllDataTrue, AvoidGlobalModifier, AvoidLogicInTrigger, ApexUnitTestAssertStatement, ApexUnitTestMethodShouldHaveIsTestAnnotation language: Apex --- +## ApexUnitTestAssertStatement + +**Since:** PMD 6.12.0 + +**Priority:** Medium (3) + +Apex test assert should have 2 params and assertEquals/assertNotEquals should have 3 params. + +**This rule is defined by the following Java class:** [net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestAssertStatementRule](https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java) + +**Example(s):** + +``` java +@isTest +public class Foo { + @isTest + static void methodATest() { + System.assertNotEquals('123', o.StageName); // not good + System.assertEquals('123', o.StageName, o); // good + System.assert(1==1); // not good + System.assert(1==1, 2); // good + } +} +``` + +**This rule has the following properties:** + +|Name|Default Value|Description|Multivalued| +|----|-------------|-----------|-----------| +|cc\_categories|Style|Code Climate Categories|yes. Delimiter is '\|'.| +|cc\_remediation\_points\_multiplier|1|Code Climate Remediation Points multiplier|no| +|cc\_block\_highlighting|false|Code Climate Block Highlighting|no| + +**Use this rule by referencing it:** +``` xml + +``` + ## ApexUnitTestClassShouldHaveAsserts **Since:** PMD 5.5.1 @@ -47,6 +85,46 @@ public class Foo { ``` +## ApexUnitTestMethodShouldHaveIsTestAnnotation + +**Since:** PMD 6.12.0 + +**Priority:** Medium (3) + +Apex test methods should have @isTest annotation either in the same line or in previous line. + +**This rule is defined by the following Java class:** [net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestMethodShouldHaveIsTestAnnotationRule](https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java) + +**Example(s):** + +``` java +@isTest +private class ATest { + @isTest + static void methodATest() { + } + static void methodBTest() { + } + @isTest static void methodCTest() { + } + private void fetchData() { + } +} +``` + +**This rule has the following properties:** + +|Name|Default Value|Description|Multivalued| +|----|-------------|-----------|-----------| +|cc\_categories|Style|Code Climate Categories|yes. Delimiter is '\|'.| +|cc\_remediation\_points\_multiplier|1|Code Climate Remediation Points multiplier|no| +|cc\_block\_highlighting|false|Code Climate Block Highlighting|no| + +**Use this rule by referencing it:** +``` xml + +``` + ## ApexUnitTestShouldNotUseSeeAllDataTrue **Since:** PMD 5.5.1 diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java new file mode 100644 index 0000000000..2b690748a8 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java @@ -0,0 +1,65 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.bestpractices; + +import java.util.HashSet; +import java.util.Locale; +import java.util.Set; + +import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; +import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; + +/** + * Apex unit tests should have System.assert methods in them + * + * @author sudhansu + */ +public class ApexUnitTestAssertStatementRule extends AbstractApexUnitTestRule { + + private static final Set ASSERT_METHODS = new HashSet<>(); + private static final String ASSERT = "system.assert"; + private static final String ASSERT_EQUALS = "system.assertequals"; + private static final String ASSERT_NOT_EQUALS = "system.assertnotequals"; + + static { + ASSERT_METHODS.add(ASSERT); + ASSERT_METHODS.add(ASSERT_EQUALS); + ASSERT_METHODS.add(ASSERT_NOT_EQUALS); + } + + @Override + public Object visit(ASTMethodCallExpression node, Object data) { + if (!ASSERT_METHODS.contains(node.getFullMethodName().toLowerCase(Locale.ROOT))) { + return data; + } + + return checkForAssertStatement(node, data); + } + + protected Object checkForAssertStatement(ApexNode node, Object data) { + // if System.assert has 1 argument, throw error + // if System.assertEquals/System.assertNotEquals has only 2 arguments, throw error + ASTMethodCallExpression assertMethodCall = (ASTMethodCallExpression) node; + if (assertMethodCall == null) { + return data; + } + if (ASSERT.equalsIgnoreCase(assertMethodCall.getFullMethodName()) + && assertMethodCall.jjtGetNumChildren() == 2) { + addViolationWithMessage(data, node, + "''{0}'' should have 2 parameters.", + new Object[] { ASSERT }); + } else if ((ASSERT_EQUALS.equalsIgnoreCase(assertMethodCall.getFullMethodName()) + || ASSERT_NOT_EQUALS.equalsIgnoreCase(assertMethodCall.getFullMethodName())) + && assertMethodCall.jjtGetNumChildren() == 3) { + Object obj = ASSERT_EQUALS.equalsIgnoreCase(assertMethodCall.getFullMethodName()) + ? ASSERT_EQUALS : ASSERT_NOT_EQUALS; + addViolationWithMessage(data, node, + "''{0}'' should have 3 parameters.", + new Object[] { obj }); + } + return data; + } +} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java new file mode 100644 index 0000000000..f5965a60e3 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java @@ -0,0 +1,78 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.bestpractices; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTModifierNode; +import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; +import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; + +import apex.jorje.semantic.ast.modifier.Annotation; +import apex.jorje.semantic.ast.modifier.ModifierOrAnnotation; +import apex.jorje.semantic.symbol.type.AnnotationTypeInfos; +import apex.jorje.semantic.symbol.type.ModifierOrAnnotationTypeInfo; +import apex.jorje.semantic.symbol.type.TypeInfoEquivalence; +import apex.jorje.services.Version; + +public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractApexUnitTestRule { + private static final String TEST = "test"; + + @Override + public Object visit(final ASTUserClass node, final Object data) { + // test methods should have @isTest annotation. + final Version classApiVersion = node.getNode().getDefiningType().getCodeUnitDetails().getVersion(); + + if (!isTestMethodOrClass(node) && classApiVersion.isGreaterThan(Version.V174)) { + return data; + } + + checkForIsTestAnnotation(node, data); + return super.visit(node, data); + } + + private Object checkForIsTestAnnotation(final ApexNode node, final Object data) { + final List methods = node.findDescendantsOfType(ASTMethod.class); + final List testMethods = new ArrayList<>(); + for (ASTMethod method : methods) { + final Version classApiVersion = method.getNode().getDefiningType().getCodeUnitDetails().getVersion(); + if (!isTestMethodOrClass(method) && classApiVersion.isGreaterThan(Version.V174) + && !method.getImage().toLowerCase(Locale.ROOT).contains(TEST)) { + continue; + } + testMethods.add(method); + } + Map methodLocMap = new HashMap<>(); + for (ASTMethod testMethod : testMethods) { + methodLocMap.put(testMethod.getNode().getLoc().getLine(), testMethod); + } + List modifierList = node.findDescendantsOfType(ASTModifierNode.class); + final Map modifierLocMap = new HashMap<>(); + for (ASTModifierNode modifier : modifierList) { + for (final ModifierOrAnnotationTypeInfo modifierOrAnnotationTypeInfo : modifier.getNode().getModifiers().all()) { + ModifierOrAnnotation modifierOrAnnotation = modifier.getNode().getModifiers().get(modifierOrAnnotationTypeInfo); + if (modifierOrAnnotation instanceof Annotation && TypeInfoEquivalence + .isEquivalent(modifierOrAnnotationTypeInfo, AnnotationTypeInfos.IS_TEST)) { + modifierLocMap.put(modifierOrAnnotation.getLoc().getLine(), modifierOrAnnotation); + } + } + } + for (Map.Entry entry : methodLocMap.entrySet()) { + if (entry != null && modifierLocMap.get(entry.getKey()) == null + && modifierLocMap.get(entry.getKey() - 1) == null) { + addViolationWithMessage(data, node, + "''{0}'' method should have @isTest annotation.", + new Object[] { entry.getValue().getImage() }); + } + } + return data; + } +} diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index 1ac782d8b1..bdea213275 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -106,6 +106,56 @@ trigger Accounts on Account (before insert, before update, before delete, after } } } +]]> + + + + + Apex test assert should have 2 params and assertEquals/assertNotEquals should have 3 params. + + 3 + + + + + + + Apex test methods should have @isTest annotation either in the same line or in previous line. + + 3 + + diff --git a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml index 993f33a772..c49d32854a 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml +++ b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml @@ -233,6 +233,24 @@ + + 3 + + + + + + + + + 3 + + + + + + + diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java new file mode 100644 index 0000000000..1ceb9c97df --- /dev/null +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java @@ -0,0 +1,11 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.bestpractices; + +import net.sourceforge.pmd.testframework.PmdRuleTst; + +public class ApexUnitTestAssertStatementTest extends PmdRuleTst { + // no additional unit tests +} diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationTest.java new file mode 100644 index 0000000000..b753e8fb8a --- /dev/null +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationTest.java @@ -0,0 +1,11 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.bestpractices; + +import net.sourceforge.pmd.testframework.PmdRuleTst; + +public class ApexUnitTestMethodShouldHaveIsTestAnnotationTest extends PmdRuleTst { + // no additional unit tests +} diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml new file mode 100644 index 0000000000..3caa373a0b --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml @@ -0,0 +1,38 @@ + + + + + Problematic apex unit test assert statements - assert should have 2/3 params + 2 + + + + + [apex] assert should have 2 param and assertEquals should have 3 param. + 0 + + + diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml new file mode 100644 index 0000000000..f32c75ef5b --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml @@ -0,0 +1,25 @@ + + + + + Problematic apex unit test methods - test methods should have @isTest annotation. + 1 + + + From 833ab3f221a0ffe0f5d27a7e509fd1c92c78e3cf Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sat, 2 Mar 2019 20:31:41 -0800 Subject: [PATCH 02/10] review feedback implemented on new apex rules --- docs/pages/pmd/rules/apex.md | 5 ++--- docs/pages/pmd/rules/apex/bestpractices.md | 16 ++++++++------ .../ApexUnitTestAssertStatementRule.java | 4 ++++ ...tMethodShouldHaveIsTestAnnotationRule.java | 6 ++++- .../resources/category/apex/bestpractices.xml | 18 ++++++++------- .../xml/ApexUnitTestAssertStatement.xml | 11 +++++----- ...itTestMethodShouldHaveIsTestAnnotation.xml | 22 +++++++++++++++++++ 7 files changed, 58 insertions(+), 24 deletions(-) diff --git a/docs/pages/pmd/rules/apex.md b/docs/pages/pmd/rules/apex.md index ff62b88ef4..bf1a646680 100644 --- a/docs/pages/pmd/rules/apex.md +++ b/docs/pages/pmd/rules/apex.md @@ -11,9 +11,8 @@ folder: pmd/rules {% include callout.html content="Rules which enforce generally accepted best practices." %} -* [ApexUnitTestAssertStatement](pmd_rules_apex_bestpractices.html#apexunittestassertstatement): Apex test assert should have 2 params and assertEquals/assertNotEquals should have 3 params. * [ApexUnitTestClassShouldHaveAsserts](pmd_rules_apex_bestpractices.html#apexunittestclassshouldhaveasserts): Apex unit tests should include at least one assertion. This makes the tests more robust, and usi... -* [ApexUnitTestMethodShouldHaveIsTestAnnotation](pmd_rules_apex_bestpractices.html#apexunittestmethodshouldhaveistestannotation): Apex test methods should have @isTest annotation either in the same line or in previous line. +* [ApexUnitTestMethodShouldHaveIsTestAnnotation](pmd_rules_apex_bestpractices.html#apexunittestmethodshouldhaveistestannotation): Apex test methods should have @isTest annotation. As testMethod keyword is deprecated,... * [ApexUnitTestShouldNotUseSeeAllDataTrue](pmd_rules_apex_bestpractices.html#apexunittestshouldnotuseseealldatatrue): Apex unit tests should not use @isTest(seeAllData=true) because it opens up the existing database... * [AvoidGlobalModifier](pmd_rules_apex_bestpractices.html#avoidglobalmodifier): Global classes should be avoided (especially in managed packages) as they can never be deleted or... * [AvoidLogicInTrigger](pmd_rules_apex_bestpractices.html#avoidlogicintrigger): As triggers do not allow methods like regular classes they are less flexible and suited to apply ... @@ -122,7 +121,7 @@ folder: pmd/rules It contains the following rules: - [ApexBadCrypto](pmd_rules_apex_security.html#apexbadcrypto), [ApexCRUDViolation](pmd_rules_apex_security.html#apexcrudviolation), [ApexCSRF](pmd_rules_apex_security.html#apexcsrf), [ApexDangerousMethods](pmd_rules_apex_security.html#apexdangerousmethods), [ApexDoc](pmd_rules_apex_documentation.html#apexdoc), [ApexInsecureEndpoint](pmd_rules_apex_security.html#apexinsecureendpoint), [ApexOpenRedirect](pmd_rules_apex_security.html#apexopenredirect), [ApexSharingViolations](pmd_rules_apex_security.html#apexsharingviolations), [ApexSOQLInjection](pmd_rules_apex_security.html#apexsoqlinjection), [ApexSuggestUsingNamedCred](pmd_rules_apex_security.html#apexsuggestusingnamedcred), [ApexUnitTestAssertStatement](pmd_rules_apex_bestpractices.html#apexunittestassertstatement), [ApexUnitTestClassShouldHaveAsserts](pmd_rules_apex_bestpractices.html#apexunittestclassshouldhaveasserts), [ApexUnitTestMethodShouldHaveIsTestAnnotation](pmd_rules_apex_bestpractices.html#apexunittestmethodshouldhaveistestannotation), [ApexUnitTestShouldNotUseSeeAllDataTrue](pmd_rules_apex_bestpractices.html#apexunittestshouldnotuseseealldatatrue), [ApexXSSFromEscapeFalse](pmd_rules_apex_security.html#apexxssfromescapefalse), [ApexXSSFromURLParam](pmd_rules_apex_security.html#apexxssfromurlparam), [AvoidDeeplyNestedIfStmts](pmd_rules_apex_design.html#avoiddeeplynestedifstmts), [AvoidDirectAccessTriggerMap](pmd_rules_apex_errorprone.html#avoiddirectaccesstriggermap), [AvoidDmlStatementsInLoops](pmd_rules_apex_performance.html#avoiddmlstatementsinloops), [AvoidGlobalModifier](pmd_rules_apex_bestpractices.html#avoidglobalmodifier), [AvoidHardcodingId](pmd_rules_apex_errorprone.html#avoidhardcodingid), [AvoidLogicInTrigger](pmd_rules_apex_bestpractices.html#avoidlogicintrigger), [AvoidNonExistentAnnotations](pmd_rules_apex_errorprone.html#avoidnonexistentannotations), [AvoidSoqlInLoops](pmd_rules_apex_performance.html#avoidsoqlinloops), [AvoidSoslInLoops](pmd_rules_apex_performance.html#avoidsoslinloops), [ClassNamingConventions](pmd_rules_apex_codestyle.html#classnamingconventions), [CyclomaticComplexity](pmd_rules_apex_design.html#cyclomaticcomplexity), [EmptyCatchBlock](pmd_rules_apex_errorprone.html#emptycatchblock), [EmptyIfStmt](pmd_rules_apex_errorprone.html#emptyifstmt), [EmptyStatementBlock](pmd_rules_apex_errorprone.html#emptystatementblock), [EmptyTryOrFinallyBlock](pmd_rules_apex_errorprone.html#emptytryorfinallyblock), [EmptyWhileStmt](pmd_rules_apex_errorprone.html#emptywhilestmt), [ExcessiveClassLength](pmd_rules_apex_design.html#excessiveclasslength), [ExcessiveParameterList](pmd_rules_apex_design.html#excessiveparameterlist), [ExcessivePublicCount](pmd_rules_apex_design.html#excessivepubliccount), [ForLoopsMustUseBraces](pmd_rules_apex_codestyle.html#forloopsmustusebraces), [IfElseStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifelsestmtsmustusebraces), [IfStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifstmtsmustusebraces), [MethodNamingConventions](pmd_rules_apex_codestyle.html#methodnamingconventions), [MethodWithSameNameAsEnclosingClass](pmd_rules_apex_errorprone.html#methodwithsamenameasenclosingclass), [NcssConstructorCount](pmd_rules_apex_design.html#ncssconstructorcount), [NcssMethodCount](pmd_rules_apex_design.html#ncssmethodcount), [NcssTypeCount](pmd_rules_apex_design.html#ncsstypecount), [OneDeclarationPerLine](pmd_rules_apex_codestyle.html#onedeclarationperline), [StdCyclomaticComplexity](pmd_rules_apex_design.html#stdcyclomaticcomplexity), [TooManyFields](pmd_rules_apex_design.html#toomanyfields), [VariableNamingConventions](pmd_rules_apex_codestyle.html#variablenamingconventions), [WhileLoopsMustUseBraces](pmd_rules_apex_codestyle.html#whileloopsmustusebraces) + [ApexBadCrypto](pmd_rules_apex_security.html#apexbadcrypto), [ApexCRUDViolation](pmd_rules_apex_security.html#apexcrudviolation), [ApexCSRF](pmd_rules_apex_security.html#apexcsrf), [ApexDangerousMethods](pmd_rules_apex_security.html#apexdangerousmethods), [ApexDoc](pmd_rules_apex_documentation.html#apexdoc), [ApexInsecureEndpoint](pmd_rules_apex_security.html#apexinsecureendpoint), [ApexOpenRedirect](pmd_rules_apex_security.html#apexopenredirect), [ApexSharingViolations](pmd_rules_apex_security.html#apexsharingviolations), [ApexSOQLInjection](pmd_rules_apex_security.html#apexsoqlinjection), [ApexSuggestUsingNamedCred](pmd_rules_apex_security.html#apexsuggestusingnamedcred), [ApexUnitTestClassShouldHaveAsserts](pmd_rules_apex_bestpractices.html#apexunittestclassshouldhaveasserts), [ApexUnitTestShouldNotUseSeeAllDataTrue](pmd_rules_apex_bestpractices.html#apexunittestshouldnotuseseealldatatrue), [ApexXSSFromEscapeFalse](pmd_rules_apex_security.html#apexxssfromescapefalse), [ApexXSSFromURLParam](pmd_rules_apex_security.html#apexxssfromurlparam), [AvoidDeeplyNestedIfStmts](pmd_rules_apex_design.html#avoiddeeplynestedifstmts), [AvoidDirectAccessTriggerMap](pmd_rules_apex_errorprone.html#avoiddirectaccesstriggermap), [AvoidDmlStatementsInLoops](pmd_rules_apex_performance.html#avoiddmlstatementsinloops), [AvoidGlobalModifier](pmd_rules_apex_bestpractices.html#avoidglobalmodifier), [AvoidHardcodingId](pmd_rules_apex_errorprone.html#avoidhardcodingid), [AvoidLogicInTrigger](pmd_rules_apex_bestpractices.html#avoidlogicintrigger), [AvoidNonExistentAnnotations](pmd_rules_apex_errorprone.html#avoidnonexistentannotations), [AvoidSoqlInLoops](pmd_rules_apex_performance.html#avoidsoqlinloops), [AvoidSoslInLoops](pmd_rules_apex_performance.html#avoidsoslinloops), [ClassNamingConventions](pmd_rules_apex_codestyle.html#classnamingconventions), [CyclomaticComplexity](pmd_rules_apex_design.html#cyclomaticcomplexity), [EmptyCatchBlock](pmd_rules_apex_errorprone.html#emptycatchblock), [EmptyIfStmt](pmd_rules_apex_errorprone.html#emptyifstmt), [EmptyStatementBlock](pmd_rules_apex_errorprone.html#emptystatementblock), [EmptyTryOrFinallyBlock](pmd_rules_apex_errorprone.html#emptytryorfinallyblock), [EmptyWhileStmt](pmd_rules_apex_errorprone.html#emptywhilestmt), [ExcessiveClassLength](pmd_rules_apex_design.html#excessiveclasslength), [ExcessiveParameterList](pmd_rules_apex_design.html#excessiveparameterlist), [ExcessivePublicCount](pmd_rules_apex_design.html#excessivepubliccount), [ForLoopsMustUseBraces](pmd_rules_apex_codestyle.html#forloopsmustusebraces), [IfElseStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifelsestmtsmustusebraces), [IfStmtsMustUseBraces](pmd_rules_apex_codestyle.html#ifstmtsmustusebraces), [MethodNamingConventions](pmd_rules_apex_codestyle.html#methodnamingconventions), [MethodWithSameNameAsEnclosingClass](pmd_rules_apex_errorprone.html#methodwithsamenameasenclosingclass), [NcssConstructorCount](pmd_rules_apex_design.html#ncssconstructorcount), [NcssMethodCount](pmd_rules_apex_design.html#ncssmethodcount), [NcssTypeCount](pmd_rules_apex_design.html#ncsstypecount), [OneDeclarationPerLine](pmd_rules_apex_codestyle.html#onedeclarationperline), [StdCyclomaticComplexity](pmd_rules_apex_design.html#stdcyclomaticcomplexity), [TooManyFields](pmd_rules_apex_design.html#toomanyfields), [VariableNamingConventions](pmd_rules_apex_codestyle.html#variablenamingconventions), [WhileLoopsMustUseBraces](pmd_rules_apex_codestyle.html#whileloopsmustusebraces) * Empty Code (`rulesets/apex/empty.xml`): diff --git a/docs/pages/pmd/rules/apex/bestpractices.md b/docs/pages/pmd/rules/apex/bestpractices.md index 80fb834f1e..d3d684aed8 100644 --- a/docs/pages/pmd/rules/apex/bestpractices.md +++ b/docs/pages/pmd/rules/apex/bestpractices.md @@ -11,11 +11,12 @@ language: Apex ## ApexUnitTestAssertStatement -**Since:** PMD 6.12.0 +**Since:** PMD 6.13.0 **Priority:** Medium (3) -Apex test assert should have 2 params and assertEquals/assertNotEquals should have 3 params. +The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. +Having a second/third parameter provides more information and makes it easier to debug the test failure and improves the readability of test output. **This rule is defined by the following Java class:** [net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestAssertStatementRule](https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java) @@ -27,9 +28,9 @@ public class Foo { @isTest static void methodATest() { System.assertNotEquals('123', o.StageName); // not good - System.assertEquals('123', o.StageName, o); // good - System.assert(1==1); // not good - System.assert(1==1, 2); // good + System.assertEquals('123', o.StageName, 'Opportunity stageName is wrong.'); // good + System.assert(o.isClosed); // not good + System.assert(o.isClosed, 'Opportunity is not closed.'); // good } } ``` @@ -87,11 +88,12 @@ public class Foo { ## ApexUnitTestMethodShouldHaveIsTestAnnotation -**Since:** PMD 6.12.0 +**Since:** PMD 6.13.0 **Priority:** Medium (3) -Apex test methods should have @isTest annotation either in the same line or in previous line. +Apex test methods should have @isTest annotation. +As testMethod keyword is deprecated, Salesforce advices to use @isTest annotation for test class/methods. **This rule is defined by the following Java class:** [net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestMethodShouldHaveIsTestAnnotationRule](https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java index 2b690748a8..35ff462122 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java @@ -30,6 +30,10 @@ public class ApexUnitTestAssertStatementRule extends AbstractApexUnitTestRule { ASSERT_METHODS.add(ASSERT_NOT_EQUALS); } + public ApexUnitTestAssertStatementRule() { + addRuleChainVisit(ASTMethodCallExpression.class); + } + @Override public Object visit(ASTMethodCallExpression node, Object data) { if (!ASSERT_METHODS.contains(node.getFullMethodName().toLowerCase(Locale.ROOT))) { diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java index f5965a60e3..6d8ca1f082 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java @@ -26,6 +26,10 @@ import apex.jorje.services.Version; public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractApexUnitTestRule { private static final String TEST = "test"; + public ApexUnitTestMethodShouldHaveIsTestAnnotationRule() { + addRuleChainVisit(ASTUserClass.class); + } + @Override public Object visit(final ASTUserClass node, final Object data) { // test methods should have @isTest annotation. @@ -36,7 +40,7 @@ public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractAp } checkForIsTestAnnotation(node, data); - return super.visit(node, data); + return data; } private Object checkForIsTestAnnotation(final ApexNode node, final Object data) { diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index bdea213275..55a77e5189 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -110,12 +110,13 @@ trigger Accounts on Account (before insert, before update, before delete, after - Apex test assert should have 2 params and assertEquals/assertNotEquals should have 3 params. + The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. + Having a second/third parameter provides more information and makes it easier to debug the test failure and improves the readability of test output. 3 @@ -125,21 +126,22 @@ public class Foo { @isTest static void methodATest() { System.assertNotEquals('123', o.StageName); // not good - System.assertEquals('123', o.StageName, o); // good - System.assert(1==1); // not good - System.assert(1==1, 2); // good + System.assertEquals('123', o.StageName, 'Opportunity stageName is wrong.'); // good + System.assert(o.isClosed); // not good + System.assert(o.isClosed, 'Opportunity is not closed.'); // good } } ]]> - Apex test methods should have @isTest annotation either in the same line or in previous line. + Apex test methods should have @isTest annotation. + As testMethod keyword is deprecated, Salesforce advices to use @isTest annotation for test class/methods. 3 diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml index 3caa373a0b..dba55d1863 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml @@ -7,15 +7,16 @@ Problematic apex unit test assert statements - assert should have 2/3 params 2 + 5,7 @@ -29,8 +30,8 @@ public class Foo { public class Foo { @isTest static void methodATest() { - System.assertEquals('123', o.StageName, o); // good - System.assert(1==1, 2); // good + System.assertEquals('Test opportunity', o.Name, 'Opportunity Name' + o.Name + 'is wrong.'); // good + System.assert(o.isClosed, 'Opportunity is not closed.'); // good } } ]]> diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml index f32c75ef5b..a0ef640db7 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml @@ -19,6 +19,28 @@ private class ATest { } private void fetchData() { } +} + ]]> + + + Test methods should have @isTest annotation. + 0 + From 99f9cc60545825680f51cc218be88f3dd4206715 Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sat, 23 Mar 2019 14:49:50 -0700 Subject: [PATCH 03/10] Modified is test annotation rule and renamed ApexAssertionsShouldIncludeMessage rule --- docs/pages/pmd/rules/apex/bestpractices.md | 2 +- ...exAssertionsShouldIncludeMessageRule.java} | 16 ++-- ...tMethodShouldHaveIsTestAnnotationRule.java | 76 +++++++------------ .../resources/category/apex/bestpractices.xml | 10 ++- .../main/resources/rulesets/apex/ruleset.xml | 9 --- ...exAssertionsShouldIncludeMessageTest.java} | 2 +- ...=> ApexAssertionsShouldIncludeMessage.xml} | 0 ...itTestMethodShouldHaveIsTestAnnotation.xml | 21 ++++- 8 files changed, 61 insertions(+), 75 deletions(-) rename pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/{ApexUnitTestAssertStatementRule.java => ApexAssertionsShouldIncludeMessageRule.java} (80%) rename pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/{ApexUnitTestAssertStatementTest.java => ApexAssertionsShouldIncludeMessageTest.java} (76%) rename pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/{ApexUnitTestAssertStatement.xml => ApexAssertionsShouldIncludeMessage.xml} (100%) diff --git a/docs/pages/pmd/rules/apex/bestpractices.md b/docs/pages/pmd/rules/apex/bestpractices.md index d3d684aed8..6c3e96b13f 100644 --- a/docs/pages/pmd/rules/apex/bestpractices.md +++ b/docs/pages/pmd/rules/apex/bestpractices.md @@ -18,7 +18,7 @@ language: Apex The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. Having a second/third parameter provides more information and makes it easier to debug the test failure and improves the readability of test output. -**This rule is defined by the following Java class:** [net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestAssertStatementRule](https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java) +**This rule is defined by the following Java class:** [net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexAssertionsShouldIncludeMessageRule](https://github.com/pmd/pmd/blob/master/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java) **Example(s):** diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java similarity index 80% rename from pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java rename to pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java index 35ff462122..8aced97aca 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java @@ -17,20 +17,20 @@ import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; * * @author sudhansu */ -public class ApexUnitTestAssertStatementRule extends AbstractApexUnitTestRule { +public class ApexAssertionsShouldIncludeMessageRule extends AbstractApexUnitTestRule { private static final Set ASSERT_METHODS = new HashSet<>(); - private static final String ASSERT = "system.assert"; - private static final String ASSERT_EQUALS = "system.assertequals"; - private static final String ASSERT_NOT_EQUALS = "system.assertnotequals"; + private static final String ASSERT = "System.assert"; + private static final String ASSERT_EQUALS = "System.assertEquals"; + private static final String ASSERT_NOT_EQUALS = "System.assertNotEquals"; static { - ASSERT_METHODS.add(ASSERT); - ASSERT_METHODS.add(ASSERT_EQUALS); - ASSERT_METHODS.add(ASSERT_NOT_EQUALS); + ASSERT_METHODS.add(ASSERT.toLowerCase(Locale.ROOT)); + ASSERT_METHODS.add(ASSERT_EQUALS.toLowerCase(Locale.ROOT)); + ASSERT_METHODS.add(ASSERT_NOT_EQUALS.toLowerCase(Locale.ROOT) ); } - public ApexUnitTestAssertStatementRule() { + public ApexAssertionsShouldIncludeMessageRule() { addRuleChainVisit(ASTMethodCallExpression.class); } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java index 6d8ca1f082..973ded14d5 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java @@ -4,77 +4,53 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; -import java.util.ArrayList; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Locale; -import java.util.Map; +import java.util.Set; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; -import net.sourceforge.pmd.lang.apex.ast.ASTModifierNode; -import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; +import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; -import apex.jorje.semantic.ast.modifier.Annotation; -import apex.jorje.semantic.ast.modifier.ModifierOrAnnotation; -import apex.jorje.semantic.symbol.type.AnnotationTypeInfos; -import apex.jorje.semantic.symbol.type.ModifierOrAnnotationTypeInfo; -import apex.jorje.semantic.symbol.type.TypeInfoEquivalence; -import apex.jorje.services.Version; - public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractApexUnitTestRule { - private static final String TEST = "test"; + private static final Set ASSERT_METHODS = new HashSet<>(); + private static final String ASSERT = "system.assert"; + private static final String ASSERT_EQUALS = "system.assertequals"; + private static final String ASSERT_NOT_EQUALS = "system.assertnotequals"; + + static { + ASSERT_METHODS.add(ASSERT); + ASSERT_METHODS.add(ASSERT_EQUALS); + ASSERT_METHODS.add(ASSERT_NOT_EQUALS); + } public ApexUnitTestMethodShouldHaveIsTestAnnotationRule() { - addRuleChainVisit(ASTUserClass.class); + addRuleChainVisit(ASTMethod.class); } @Override - public Object visit(final ASTUserClass node, final Object data) { + public Object visit(final ASTMethod node, final Object data) { // test methods should have @isTest annotation. - final Version classApiVersion = node.getNode().getDefiningType().getCodeUnitDetails().getVersion(); - - if (!isTestMethodOrClass(node) && classApiVersion.isGreaterThan(Version.V174)) { + if (isTestMethodOrClass(node)) { return data; } - - checkForIsTestAnnotation(node, data); - return data; + return checkForIsTestAnnotation(node, data); } private Object checkForIsTestAnnotation(final ApexNode node, final Object data) { - final List methods = node.findDescendantsOfType(ASTMethod.class); - final List testMethods = new ArrayList<>(); - for (ASTMethod method : methods) { - final Version classApiVersion = method.getNode().getDefiningType().getCodeUnitDetails().getVersion(); - if (!isTestMethodOrClass(method) && classApiVersion.isGreaterThan(Version.V174) - && !method.getImage().toLowerCase(Locale.ROOT).contains(TEST)) { - continue; - } - testMethods.add(method); + ASTMethod testMethod = (ASTMethod) node; + if (testMethod == null) { + return data; } - Map methodLocMap = new HashMap<>(); - for (ASTMethod testMethod : testMethods) { - methodLocMap.put(testMethod.getNode().getLoc().getLine(), testMethod); - } - List modifierList = node.findDescendantsOfType(ASTModifierNode.class); - final Map modifierLocMap = new HashMap<>(); - for (ASTModifierNode modifier : modifierList) { - for (final ModifierOrAnnotationTypeInfo modifierOrAnnotationTypeInfo : modifier.getNode().getModifiers().all()) { - ModifierOrAnnotation modifierOrAnnotation = modifier.getNode().getModifiers().get(modifierOrAnnotationTypeInfo); - if (modifierOrAnnotation instanceof Annotation && TypeInfoEquivalence - .isEquivalent(modifierOrAnnotationTypeInfo, AnnotationTypeInfos.IS_TEST)) { - modifierLocMap.put(modifierOrAnnotation.getLoc().getLine(), modifierOrAnnotation); - } - } - } - for (Map.Entry entry : methodLocMap.entrySet()) { - if (entry != null && modifierLocMap.get(entry.getKey()) == null - && modifierLocMap.get(entry.getKey() - 1) == null) { + List methodCallList = testMethod.findDescendantsOfType(ASTMethodCallExpression.class); + for (ASTMethodCallExpression assertMethodCall : methodCallList) { + if (ASSERT_METHODS.contains(assertMethodCall.getFullMethodName().toLowerCase(Locale.ROOT))) { addViolationWithMessage(data, node, - "''{0}'' method should have @isTest annotation.", - new Object[] { entry.getValue().getImage() }); + "''{0}'' method should have @IsTest annotation.", + new Object[] { testMethod.getImage() }); + return data; } } return data; diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index 55a77e5189..d5b6606913 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -109,11 +109,11 @@ trigger Accounts on Account (before insert, before update, before delete, after ]]> - + class="net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexAssertionsShouldIncludeMessageRule" + externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_bestpractices.html#apexassertionsshouldincludemessage"> The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. Having a second/third parameter provides more information and makes it easier to debug the test failure and improves the readability of test output. @@ -154,6 +154,10 @@ private class ATest { static void methodBTest() { } @isTest static void methodCTest() { + System.assert(1==2); + } + @isTest static void methodCTest() { + System.debug('I am a debug statement'); } private void fetchData() { } diff --git a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml index c49d32854a..96230cbc5c 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml +++ b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml @@ -233,15 +233,6 @@ - - 3 - - - - - - - 3 diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageTest.java similarity index 76% rename from pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java rename to pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageTest.java index 1ceb9c97df..8b9464e9e9 100644 --- a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestAssertStatementTest.java +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageTest.java @@ -6,6 +6,6 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; import net.sourceforge.pmd.testframework.PmdRuleTst; -public class ApexUnitTestAssertStatementTest extends PmdRuleTst { +public class ApexAssertionsShouldIncludeMessageTest extends PmdRuleTst { // no additional unit tests } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml similarity index 100% rename from pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestAssertStatement.xml rename to pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml index a0ef640db7..4a1861280f 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml @@ -6,7 +6,8 @@ Problematic apex unit test methods - test methods should have @isTest annotation. - 1 + 2 + 6,18 - + \ No newline at end of file From 7e44f025e4ad98c81c37e21f2462eca0b6eab327 Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sat, 23 Mar 2019 15:23:31 -0700 Subject: [PATCH 04/10] removed spaces --- .../bestpractices/ApexAssertionsShouldIncludeMessageRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java index 8aced97aca..3f0031dd35 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java @@ -27,7 +27,7 @@ public class ApexAssertionsShouldIncludeMessageRule extends AbstractApexUnitTest static { ASSERT_METHODS.add(ASSERT.toLowerCase(Locale.ROOT)); ASSERT_METHODS.add(ASSERT_EQUALS.toLowerCase(Locale.ROOT)); - ASSERT_METHODS.add(ASSERT_NOT_EQUALS.toLowerCase(Locale.ROOT) ); + ASSERT_METHODS.add(ASSERT_NOT_EQUALS.toLowerCase(Locale.ROOT)); } public ApexAssertionsShouldIncludeMessageRule() { From a0eabd34b0ce4a86a6b2f30a197d9695a06c1f59 Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sat, 23 Mar 2019 16:16:32 -0700 Subject: [PATCH 05/10] merge conflicts --- .../main/resources/rulesets/apex/ruleset.xml | 496 ++---------------- 1 file changed, 47 insertions(+), 449 deletions(-) diff --git a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml index 96230cbc5c..821fb07339 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml +++ b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml @@ -2,452 +2,50 @@ Default ruleset used by the Code Climate Engine for Salesforce.com Apex - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - - - - 3 - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file From 72cbd2b63bdd7df56f9d70b622128869c5e5e376 Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sat, 23 Mar 2019 16:25:40 -0700 Subject: [PATCH 06/10] merge conflicts --- pmd-apex/src/main/resources/rulesets/apex/ruleset.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml index 821fb07339..00b1054ad8 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml +++ b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml @@ -48,4 +48,4 @@ - \ No newline at end of file + From d1a9630ee20fd2d89e551f4972decc4ef6489ef8 Mon Sep 17 00:00:00 2001 From: sudhansu Date: Sun, 24 Mar 2019 21:01:24 -0700 Subject: [PATCH 07/10] review feedback --- ...pexAssertionsShouldIncludeMessageRule.java | 49 ++++--------------- ...tMethodShouldHaveIsTestAnnotationRule.java | 30 +++--------- .../ApexAssertionsShouldIncludeMessage.xml | 15 +++++- ...itTestMethodShouldHaveIsTestAnnotation.xml | 11 +++++ 4 files changed, 42 insertions(+), 63 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java index 3f0031dd35..03be1465a8 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java @@ -4,65 +4,36 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; -import java.util.HashSet; import java.util.Locale; -import java.util.Set; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; -import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; -/** - * Apex unit tests should have System.assert methods in them - * - * @author sudhansu - */ public class ApexAssertionsShouldIncludeMessageRule extends AbstractApexUnitTestRule { - private static final Set ASSERT_METHODS = new HashSet<>(); private static final String ASSERT = "System.assert"; private static final String ASSERT_EQUALS = "System.assertEquals"; private static final String ASSERT_NOT_EQUALS = "System.assertNotEquals"; - static { - ASSERT_METHODS.add(ASSERT.toLowerCase(Locale.ROOT)); - ASSERT_METHODS.add(ASSERT_EQUALS.toLowerCase(Locale.ROOT)); - ASSERT_METHODS.add(ASSERT_NOT_EQUALS.toLowerCase(Locale.ROOT)); - } - - public ApexAssertionsShouldIncludeMessageRule() { - addRuleChainVisit(ASTMethodCallExpression.class); - } - @Override public Object visit(ASTMethodCallExpression node, Object data) { - if (!ASSERT_METHODS.contains(node.getFullMethodName().toLowerCase(Locale.ROOT))) { + String methodName = node.getFullMethodName().toLowerCase(Locale.ROOT); + if (!ASSERT.equalsIgnoreCase(methodName) + && !ASSERT_EQUALS.equalsIgnoreCase(methodName) + && !ASSERT_NOT_EQUALS.equalsIgnoreCase(methodName)) { return data; } - - return checkForAssertStatement(node, data); - } - - protected Object checkForAssertStatement(ApexNode node, Object data) { - // if System.assert has 1 argument, throw error - // if System.assertEquals/System.assertNotEquals has only 2 arguments, throw error - ASTMethodCallExpression assertMethodCall = (ASTMethodCallExpression) node; - if (assertMethodCall == null) { - return data; - } - if (ASSERT.equalsIgnoreCase(assertMethodCall.getFullMethodName()) - && assertMethodCall.jjtGetNumChildren() == 2) { + if (ASSERT.equalsIgnoreCase(methodName) + && node.jjtGetNumChildren() == 2) { addViolationWithMessage(data, node, "''{0}'' should have 2 parameters.", new Object[] { ASSERT }); - } else if ((ASSERT_EQUALS.equalsIgnoreCase(assertMethodCall.getFullMethodName()) - || ASSERT_NOT_EQUALS.equalsIgnoreCase(assertMethodCall.getFullMethodName())) - && assertMethodCall.jjtGetNumChildren() == 3) { - Object obj = ASSERT_EQUALS.equalsIgnoreCase(assertMethodCall.getFullMethodName()) - ? ASSERT_EQUALS : ASSERT_NOT_EQUALS; + } else if ((ASSERT_EQUALS.equalsIgnoreCase(methodName) + || ASSERT_NOT_EQUALS.equalsIgnoreCase(methodName)) + && node.jjtGetNumChildren() == 3) { addViolationWithMessage(data, node, "''{0}'' should have 3 parameters.", - new Object[] { obj }); + new Object[] { methodName }); } return data; } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java index 973ded14d5..843d001696 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java @@ -4,50 +4,36 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; -import java.util.HashSet; import java.util.List; import java.util.Locale; -import java.util.Set; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; -import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractApexUnitTestRule { - private static final Set ASSERT_METHODS = new HashSet<>(); private static final String ASSERT = "system.assert"; private static final String ASSERT_EQUALS = "system.assertequals"; private static final String ASSERT_NOT_EQUALS = "system.assertnotequals"; - static { - ASSERT_METHODS.add(ASSERT); - ASSERT_METHODS.add(ASSERT_EQUALS); - ASSERT_METHODS.add(ASSERT_NOT_EQUALS); - } - - public ApexUnitTestMethodShouldHaveIsTestAnnotationRule() { - addRuleChainVisit(ASTMethod.class); - } - @Override public Object visit(final ASTMethod node, final Object data) { // test methods should have @isTest annotation. if (isTestMethodOrClass(node)) { return data; } - return checkForIsTestAnnotation(node, data); + return checkForAssertStatements(node, data); } - private Object checkForIsTestAnnotation(final ApexNode node, final Object data) { - ASTMethod testMethod = (ASTMethod) node; - if (testMethod == null) { - return data; - } + private Object checkForAssertStatements(final ASTMethod testMethod, final Object data) { List methodCallList = testMethod.findDescendantsOfType(ASTMethodCallExpression.class); + String assertMethodName; for (ASTMethodCallExpression assertMethodCall : methodCallList) { - if (ASSERT_METHODS.contains(assertMethodCall.getFullMethodName().toLowerCase(Locale.ROOT))) { - addViolationWithMessage(data, node, + assertMethodName = assertMethodCall.getFullMethodName().toLowerCase(Locale.ROOT); + if (ASSERT.equalsIgnoreCase(assertMethodName) + || ASSERT_EQUALS.equalsIgnoreCase(assertMethodName) + || ASSERT_NOT_EQUALS.equalsIgnoreCase(assertMethodName)) { + addViolationWithMessage(data, testMethod, "''{0}'' method should have @IsTest annotation.", new Object[] { testMethod.getImage() }); return data; diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml index dba55d1863..9c1d0ec390 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml @@ -21,18 +21,29 @@ public class Foo { } ]]> - [apex] assert should have 2 param and assertEquals should have 3 param. 0 + + + [apex] Avoid false positive in non test classes. + 0 + diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml index 4a1861280f..4c804775d9 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestMethodShouldHaveIsTestAnnotation.xml @@ -56,6 +56,17 @@ private class ATest { } private void fetchData() { } +} + ]]> + + + Avoid false positive in non test class methods. + 0 + From db3b211d8faa2cca2aee8c4007685d42bb544b05 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 25 Mar 2019 19:50:32 +0100 Subject: [PATCH 08/10] Undo changes to rulesets/apex/ruleset.xml --- .../main/resources/rulesets/apex/ruleset.xml | 487 ++++++++++++++++-- 1 file changed, 440 insertions(+), 47 deletions(-) diff --git a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml index 00b1054ad8..25a94a61ca 100644 --- a/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml +++ b/pmd-apex/src/main/resources/rulesets/apex/ruleset.xml @@ -2,50 +2,443 @@ Default ruleset used by the Code Climate Engine for Salesforce.com Apex - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + + + + 3 + + + + + + + + \ No newline at end of file From 2958f83517865f2dff2c16808c87d240c3f2acc3 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 25 Mar 2019 19:56:47 +0100 Subject: [PATCH 09/10] Simplify the rules even more --- .../ApexAssertionsShouldIncludeMessageRule.java | 13 +++---------- ...TestMethodShouldHaveIsTestAnnotationRule.java | 16 ++++++++++------ 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java index 03be1465a8..10c8424826 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexAssertionsShouldIncludeMessageRule.java @@ -4,8 +4,6 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; -import java.util.Locale; - import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; @@ -17,14 +15,9 @@ public class ApexAssertionsShouldIncludeMessageRule extends AbstractApexUnitTest @Override public Object visit(ASTMethodCallExpression node, Object data) { - String methodName = node.getFullMethodName().toLowerCase(Locale.ROOT); - if (!ASSERT.equalsIgnoreCase(methodName) - && !ASSERT_EQUALS.equalsIgnoreCase(methodName) - && !ASSERT_NOT_EQUALS.equalsIgnoreCase(methodName)) { - return data; - } - if (ASSERT.equalsIgnoreCase(methodName) - && node.jjtGetNumChildren() == 2) { + String methodName = node.getFullMethodName(); + + if (ASSERT.equalsIgnoreCase(methodName) && node.jjtGetNumChildren() == 2) { addViolationWithMessage(data, node, "''{0}'' should have 2 parameters.", new Object[] { ASSERT }); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java index 843d001696..66185f9a3d 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotationRule.java @@ -4,17 +4,23 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; +import java.util.HashSet; import java.util.List; import java.util.Locale; +import java.util.Set; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.rule.AbstractApexUnitTestRule; public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractApexUnitTestRule { - private static final String ASSERT = "system.assert"; - private static final String ASSERT_EQUALS = "system.assertequals"; - private static final String ASSERT_NOT_EQUALS = "system.assertnotequals"; + private static final Set ASSERT_METHODS = new HashSet<>(); + + static { + ASSERT_METHODS.add("system.assert"); + ASSERT_METHODS.add("system.assertequals"); + ASSERT_METHODS.add("system.assertnotequals"); + } @Override public Object visit(final ASTMethod node, final Object data) { @@ -30,9 +36,7 @@ public class ApexUnitTestMethodShouldHaveIsTestAnnotationRule extends AbstractAp String assertMethodName; for (ASTMethodCallExpression assertMethodCall : methodCallList) { assertMethodName = assertMethodCall.getFullMethodName().toLowerCase(Locale.ROOT); - if (ASSERT.equalsIgnoreCase(assertMethodName) - || ASSERT_EQUALS.equalsIgnoreCase(assertMethodName) - || ASSERT_NOT_EQUALS.equalsIgnoreCase(assertMethodName)) { + if (ASSERT_METHODS.contains(assertMethodName)) { addViolationWithMessage(data, testMethod, "''{0}'' method should have @IsTest annotation.", new Object[] { testMethod.getImage() }); From 53d123703b0757602f7f7a16e160e2eaa30cb2ad Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 25 Mar 2019 20:04:19 +0100 Subject: [PATCH 10/10] Update release notes, refs #1694 --- docs/pages/release_notes.md | 7 ++ .../resources/category/apex/bestpractices.xml | 115 +++++++++--------- .../main/resources/rulesets/releases/6130.xml | 15 +++ 3 files changed, 81 insertions(+), 56 deletions(-) create mode 100644 pmd-core/src/main/resources/rulesets/releases/6130.xml diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 2a49668a30..eb6602df54 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -52,6 +52,12 @@ More information is available in [the user documentation](pmd_userdocs_cpd.html# for try-blocks, that could be changed to a try-with-resources statement. This statement ensures that each resource is closed at the end of the statement and is available since Java 7. +* The new Apex rule {% rule "apex/bestpractices/ApexAssertionsShouldIncludeMessage" %} (`apex-bestpractices`) + searches for assertions in unit tests and checks, whether they use a message argument. + +* The new Apex rule {% rule "apex/bestpractices/ApexUnitTestMethodShouldHaveIsTestAnnotation" %} (`apex-bestpractices`) + searches for methods in test classes, which are missing the `@IsTest` annotation. + #### Modified Rules * The Apex rule {% rule "apex/codestyle/MethodNamingConventions" %} (apex-codestyle) has a new @@ -106,6 +112,7 @@ More information is available in [the user documentation](pmd_userdocs_cpd.html# * [#1654](https://github.com/pmd/pmd/pull/1654): \[core] Antlr token filter - [Tomi De Lucca](https://github.com/tomidelucca) * [#1655](https://github.com/pmd/pmd/pull/1655): \[kotlin] Kotlin tokenizer refactor - [Lucas Soncini](https://github.com/lsoncini) * [#1686](https://github.com/pmd/pmd/pull/1686): \[doc] Replaced wrong escaping with ">" - [Himanshu Pandey](https://github.com/hpandeycodeit) +* [#1694](https://github.com/pmd/pmd/pull/1694): \[apex] New rules for test method and assert statements - [triandicAnt](https://github.com/triandicAnt) {% endtocmaker %} diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index d5b6606913..404502d662 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -9,6 +9,33 @@ Rules which enforce generally accepted best practices. + + +The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. +Having a second/third parameter provides more information and makes it easier to debug the test failure and +improves the readability of test output. + + 3 + + + + + + + +Apex test methods should have @isTest annotation. +As testMethod keyword is deprecated, Salesforce advices to use @isTest annotation for test class/methods. + + 3 + + + + + - - - - - The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. - Having a second/third parameter provides more information and makes it easier to debug the test failure and improves the readability of test output. - - 3 - - - - - - - Apex test methods should have @isTest annotation. - As testMethod keyword is deprecated, Salesforce advices to use @isTest annotation for test class/methods. - - 3 - - diff --git a/pmd-core/src/main/resources/rulesets/releases/6130.xml b/pmd-core/src/main/resources/rulesets/releases/6130.xml new file mode 100644 index 0000000000..0dbbfb1f5e --- /dev/null +++ b/pmd-core/src/main/resources/rulesets/releases/6130.xml @@ -0,0 +1,15 @@ + + + + +This ruleset contains links to rules that are new in PMD v6.13.0 + + + + + + +