Potential inspection profile duplication issue and PMD false positives

[John De Santiago]

Seeing a couple of inspection issues with PMD. Not sure if they are PMD-centric or IC but reporting just in case, as I do see that the inspection profile got duplicated (see screenshots). I tried to delete one of the entries both are removed. After deleting and recreating the entry and re-opening the window, the profile entry is duplicated.

I do have the sfdx-scanner plugin as well and when I run that against the same test class I don’t get the runAs warning. So not sure if it’s my setup or a bug. The PMD version I am using is pmd-bin-6.51.0.

False Positives:

Empty Statement Block - flagged on an empty constructor. It should be valid to have an empty constructor when using

ApexUnitTestClassShouldHaveRunAs - My interpretation is that at least one method should have a runAs call, but it flags all test methods that do not utilize runAs. I attempted to look at the PMD source, and based on my understanding iterates and only looks for at least one runAs call to exist in the class, but I couldn’t determine if the nodes it was iterating were methods or lines. Using @SuppressWarnings suppresses the flag at both the class and method as expected.

‌

[Scott Wells]

John, for the most part IC2 simply integrates PMD as-is, so if there are specifics to how certain PMD rules behave, you’d likely need to log bugs against that project. For example, the EmptyStatementBlock rule does allow configuration of whether empty private no-arg constructors should or should not be flagged via the reportEmptyPrivateNoArgConstructor property:

https://pmd.sourceforge.io/pmd-6.45.0/pmd_rules_apex_errorprone.html#emptystatementblock

Not sure about ApexUnitTestClassShouldHaveRunAs as I haven’t looked at the internals, but I would think that it would apply to each test method because the idea is that, if you agree with that rule conceptually, you should be running all of your tests as specific, well-established personae. I think that this:

Apex unit tests should include at least one runAs method.

is perhaps more clearly worded as:

Each Apex unit test method should include at least one runAs method.

vs. how I think you’re interpreting it which would be:

Each Apex unit test class should include at least one test method that uses runAs.

That seems more like a documentation ambiguity than a bug.

[Scott Wells]

Resolving here as I don't think there's anything IC2-specific here, but if I've missed/misunderstood something, feel free to reopen and let me know what it is.

[John De Santiago]

Your feedback makes sense and I will do more research on my end and log anything appropriate to the PMD team.

There is one item that is IC-centric that I wanted to point out, and it's the duplicate profile entry for inspections in the first screenshot. It doesn’t affect the functionality of the inspections, and based on the edit and delete behavior I observe, it looks like the same profile is just listed twice in the UI.

Concerning the runAs rule, philosophically, enforcing all test methods to use runAs makes sense if all tests invoke a CRUD operation but there are plenty of use-cases where you want to perform unit testing vs. integration testing on methods (e.g. a string manipulation method or similar utility methods). The overhead of using runAs for any and all test scenarios just doesn’t seem efficient, given the number of bugs around permissions and record locking that I observe anecdotally. Anyway, I will look into this more and discuss it with PMD now that I know that IC has a passive role in the rule evaluation.

‌

[Scott Wells]

Regarding the multiple inspection profiles, IC2 includes a bundled inspection profile called “Illuminated Cloud” now. If there was already one with that name, the IDE will try to avoid the collision, so one might actually be renamed “Illuminated_Cloud” when you actually access it. If those aren’t customized, you can safely delete both and restart the IDE and the bundled one will be restored under that name. I would then recommend copying it into the project before customizing it for things like PMD so that there’s no future conflict with the IDE-level bundled one.

[John De Santiago]

Makes sense thanks. I will do that.