[apex] ApexUnitTestMethodShouldHaveIsTestAnnotation false positive with helper method

[YodaDaCoda]

Affects PMD Version:
6.32.0

Rule:
ApexUnitTestMethodShouldHaveIsTestAnnotation

Please provide the rule name and a link to the rule documentation:
https://pmd.github.io/latest/pmd_rules_apex_bestpractices.html#apexunittestmethodshouldhaveistestannotation

Description:

Code Sample demonstrating the issue:

@isTest 
public class TestFactory {
    @isTest 
    static void test1() {
        // do things
        checkWork(data);
    }
    @isTest 
    static void test2() {
        // do things
        checkWork(data);
    }
    static void checkWork(SObject data) {
        System.assertEquals(null, data.Id, 'this should be null');
    }
}

Expected outcome:

In the example given, PMD flags the checkWork function as it has a System.assert call within it. Yet actual test methods take no parameters. Flagging this function is unexpected as it is not a test method, and cannot be a test method as it accepts parameters. IMO the rule should be improved to exclude methods with parameters as these cannot be actual test methods.

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]
CLI and/or VSCode - same result

[rsoesemann]

This PMD rule is not smart enough to understand that the assert is moved into another method. Someone could improve it but then the next person would move the assert to another class and then PMD as a single-file checker would not be able to catch it.

So @adangel , @oowekyala I vote for closing this.

[oowekyala]

Doesn't #2667 resolve a call graph? If so it's not that unrealistic

[YodaDaCoda]

I'm not saying that PMD should detect the assert in the called function (as is the case in #1089) but that the assert in the called function should not make PMD detect that function as a test function, because a function that accepts parameters cannot be a test function.

The rule seems to search each function to see if it has an assert call, and if it finds one but that function does not have the isTest annotation, flags it as a problem. IMO an early-exit if the function accepts parameters should be sufficient?

[adangel]

I'd say to keep this issue open. It's a small improvement, but nevertheless a improvement for the rule. Feel free to contribute.

And right, this is about Test Methods should have IsTest annotation and not Test should have asserts.

[jonathanwiesel]

Taking into account that the intention behind the rule is to prevent the usage of the testMethod keyword in favor of the @isTest annotation because of the testMethod deprecation, probably will suffice to change the rule to check for the presence of deprecated keyword instead of checking for the absence of the new annotation

[YodaDaCoda]

@jonathanwiesel You're right, that would be a better solution. I've made a PR to that effect - #3272. Unfortunately I'm not sure how to find and fix the tests that are now failing.

I'm also unsure that the implementation is a good one - in order to get it to work, I had to use a deprecated/internalonly getNode() method.