Thursday, October 8, 2015

Salesforce code quality test cases for Apex via static code analysis

Lets assume for a moment that your code coverage isn't a perfect 100% and there are some areas of your Apex that aren't exercised as part of the automated deployment test cases. If your suspension of disbelief goes that far, lets also assume that sometimes you add temporary code to aid in debugging those murky places. Something like:

System.assert(false, 'TEMP this assertion was only for debugging and will stop the execution so you can check the debug log etc...');

The general idea is that you can use this assertion to halt execution during manual testing. At which point it would be easy to inspect the debug log state or write out some useful state information in the assertion message. It's certainly not the most advanced debugging technique, but it is effective.

The main problem with the approach is what happens if you forget to take out that line. And then package the code base for deployment. Don't do that. It's not a good look.

Remember, the line in question may or may not be covered by the existing test cases. If it isn't it will sail right into the production managed package and then spring out and surprise a user at the most inopportune moment.

What if you could stop the package from proceeding based on the content of the Apex classes?

We need the poor man's static code analysis tool! And we need it to run as part of the packaging process.

Luckily, we already have Apex test cases that run as part of the packaging process. And we have the ApexClass sObject exposed to Apex which includes the Body of the classes within the current namespace. We just need some apex code that will scan through the Body of the apex classes and assert that they don't contain the problem string. Then add said test class to the package components. Something like:

@isTest
private class SampleCodeQualityTests {
 
 @isTest static void ensureThereAreNoTempAssertions() {
  string searchFor = '\'TEMP';

                // Exclude this class from the check. May also need to exclude based on namespace
  for(ApexClass ac : [Select Id, Name, Body From ApexClass where Name != 'SampleCodeQualityTests']) {
   integer index = ac.Body.indexOf(searchFor);
   if(index == -1) { continue; }

   integer lineNumber = lineNumberForIndex(ac.Body, index);
   string problemLine = extractLineByIndex(ac.body, index);
   System.assertEquals(-1, index, 'Apex class ' + ac.Name + '(' + ac.Id + ') contains the text ' + searchFor + ' at index ' + index + ' lineNumber:' + lineNumber + 
    '\n' + problemLine);
   
   //System.assert(!ac.Body.contains(searchFor), 'Apex class ' + ac.Name + '(' + ac.Id + ') contains the text ' + searchFor);
  }
 }

 private static integer lineNumberForIndex(string body, integer targetIndex) {
  integer lineNumber = body.substring(0, targetIndex).countMatches('\n') + 1;
  return lineNumber;
 }

 private static string extractLineByIndex(string body, integer targetIndex) {
  integer startOfLine = body.lastIndexOf('\n', targetIndex);
  integer endOfLine = body.indexOf('\n', targetIndex);

  string lineOfInterest = body.substring(startOfLine + 1, endOfLine);
  return lineOfInterest;
 }
 
}

Think of this more as a proof of concept. There are some obvious problems with it. For example, it isn't really parsing the Apex body. Just doing a dumb string search for a string that starts with TEMP. No consideration is being made for the context. It might be in comment. There are likely other issues around new line detection as well.

Still, it demonstrates the idea. Using Apex test cases to do static code analysis as part of the packaging and deployment steps.

Now if we could get to the SymbolTable in a testing context (i.e. no callout) there could be all sorts of interesting checks1:

  • Does each test method make at least one assertion?
  • Does each assertion include a message to the user?
  • Excessive lines of code in one Apex Class
  • Using System.debug statements - too many of these can be detrimental to performance.
  • Empty exception handling catch blocks
  • Unused local variables
  • Methods that aren't referenced by other code

Maybe having meaningless whitespace at the end of a code line or using somestring != null && somestring != '' rather than String.isEmpty(somestring) shouldn't be grounds to block packaging. You'll need to decide where the line gets drawn.

The other part of this type of testing is that it could form the basis of an automated org health check. Is the coverage percentage too low on a large class? Test fails. Trigger not bulkified? Test fails. An Apex class with 4000 lines of code that is padding for test coverage? Test fails!

You could easily drop them into an existing org to get a feel for the state of things before embarking on an modifications.

See also:


1. may or may not be actually possible with the SymbolTable alone.