4

While refactoring a test to make it more consistent with the style of our other tests, a colleague came upon something really weird.

The original test created the test account in @TestSetup, like so:

@IsTest
public TestClass2 {
    private static final Boolean doInsert = true;

    @TestSetup
    public static void beforeTest() {
        TestAccountFactory.create(doInsert);
    }

    @IsTest
    public static void testSomething() {
        User testUser = TestUserFactory.create(doInsert);
        Account testAccount = [SELECT Id FROM Account LIMIT 1];
        String expectedStatus = 'Active';

        System.runAs(testUser) {
            Test.startTest();
                SpagettiCode.abandonAllHopeAllYeWhoEnterHere();
                // Passes   
                System.assertEquals(
                         expectedStatus, 
                         [SELECT Status__c FROM Account 
                         WHERE Id = :testAccount.Id].Status__c]
                    ); 
            Test.stopTest();
        }
        // Passes   
        System.assertEquals(
                         expectedStatus, 
                         [SELECT Status__c FROM Account 
                         WHERE Id = :testAccount.Id].Status__c]
                    ); 
    }
}

As noted, the assertions pass.

In the modified form, the test created the Account within the test, like so:

@IsTest
public TestClass1 {
    private static final Boolean doInsert = true;

        @IsTest
        public static void testSomething() {
            User testUser = TestUserFactory.create(doInsert);
            Account testAccount = TestAccountFactory.create(doInsert);
            String expectedStatus = 'Active';

            System.runAs(testUser) {
                Test.startTest();
                    SpagettiCode.abandonAllHopeAllYeWhoEnterHere();
                    // Passes   
                    System.assertEquals(
                         expectedStatus, 
                         [SELECT Status__c FROM Account 
                         WHERE Id = :testAccount.Id].Status__c]
                    ); 
                Test.stopTest();
            }
            // Fails
            System.assertEquals(
                         expectedStatus, 
                         [SELECT Status__c FROM Account 
                         WHERE Id = :testAccount.Id].Status__c]
                    ); 
        }
    }

Now, the second assertion fails even though the only difference is that the Account was not created in @TestSetup.

This is obviously a simplification of the code and our path to discovering this discrepancy was a torturous one, but we are trying to understand how this difference would even be possible and whether this is something that we could have expected or reflects a defect in how SFDC is executing the test?

2
  • This code would not compile. Your first example references doInsert out of scope.
    – Adrian Larson
    Feb 16, 2019 at 17:51
  • Sorry, that's a transcription error... this is a recreation, not the actual code. I'll fix that. Feb 16, 2019 at 17:54

2 Answers 2

6

This is being caused by a "recursion blocker" (as mentioned by David Reed). The fix is to remove or correct the recursion blocker from your trigger logic. The reason why this happened is because static variables are reset between unit tests and test setup methods; as long as the trigger ran during test setup, it would have been reset for the unit tests. When the account is created within the unit test itself, the static variable is not reset, causing the behavior you see here.

To fix the problem, first try commenting out the recursion blocker logic and run some tests to make sure nothing breaks. If you get a "record is already in trigger" or "trigger depth exceeded" error, you'll need the recursion blocker, but you should modify it so it resets itself intelligently, or write "rising edge" trigger logic.


Here's my preferred form of recursion blocking:

static Boolean runOnce = false;
public static void beforeInsert(sObject[] records) {
  if(runOnce) {
    return;
  }
  runOnce = true;
  doBeforeInsertLogicWith(records);
  runOnce = false;
}

The secret sauce is the last line; by resetting the variable when the trigger contexts concludes, you'll fix the unit test problem and also allow the trigger to run correctly with batches of records larger than 200.


For rising edge triggers, you check to make sure the record will meet the condition, instead of checking if it already meets the condition.

Here's an example of this:

public static Boolean checkTransition(sobject oldRecord, sobject newRecord) {
  return oldRecord.get(someField) != newRecord.get(someField);
}
public static void handleRecords(sobject[] oldRecords, sobject[] newRecords) {
  sobject[] updates = new sobject[0];
  for(Integer i = 0, s = newRecords.size(); i < s; i++) {
    if(checkTransition(oldRecords[i], newRecords[i])) {
      updates.add(newRecords[i]);
    }
  }
  if(!updates.isEmpty()) {
    doUpdatesOn(updates);
  }
}

Obviously, these are meant more as pseudo-code, but hopefully you should be able to get to a fix using these ideas. Make sure you have a trigger unit test, and attempt to perform logic on at least 201 records in a single list, just to make sure that it is working appropriately either way.


As a concrete example of a legitimate use of recursion blocking with a flag, we can write code like this:

public class Accounting {
    static Boolean runOnce = false;
    public static void createDoubleBook(Book__c[] books) {
        if(runOnce) {
            return;
        }
        Book__c[] relatedBooks = new Book__c[0];
        for(Book__c book: books) {
            relatedBooks.add(new Book__c(Name=book.RelatedBookName__c));
        }
        runOnce = true;
        insert relatedBooks;
        runOnce = false;
        for(Integer i = 0, s = books.size(); i < s; i++) {
            books[i].RelatedBook__c = relatedBooks[i].Id;
        }
        // Note: incomplete example. We still need an after insert to also link
        // the related books back to the original.
    }
}

Here, we can create pairs of records that will eventually be linked to each other. Without the ability to block recursion, this would simply spiral into a governor limit, but with it, we can safely link records together. This design is also possible with afterInsert triggers and appropriate "rising edge" logic, so this example is for illustrative purposes only.

5
  • Cheers for the feedback. We'll look into this! Feb 17, 2019 at 7:52
  • @sfdcfox Having trouble seeing the usefulness of the first example above – if the beforeInsert method always ends with runOnce = false, then it will always run again, so where is the recursion being blocked? Sure I'm missing something, clarification might be helpful to some though.
    – number41
    May 21 at 0:37
  • @number41 Triggers may be called repeatedly and recursively. Repeated calls are usually legitimate: >200 records in a DML, partial retry because allOrNone=false, multiple DML statements in same transaction, Flow/Workflow-triggered updates. Recursive calls typically look like A->A->A... or A->B->A->B->A... (can involve more than 2 triggers, though). These are undesirable, as they tend to crash. The first design allows repeats but not recursion, so that the legitimate use cases are allowed.
    – sfdcfox
    May 21 at 12:59
  • @number41 w.r.t. beforeInsert, e.g. maybe you have a trigger that recreates "paired" records with lookups that point to each other. Without recursion blocking, you'd end up with an infinite loop, but with recursion blocking, you can create two records that refer to each other even though they were created at the same time. If you'd like, I can write up an example for you if that'd be illuminative.
    – sfdcfox
    May 21 at 13:01
  • @sfdcfox Ah, I should have seen it before - I would have phrased it as "recursion is blocked only in the context of what happens within the "insert relatedBooks" trigger, but the method can still repeat after that trigger completes and subsequent code ensues". Of course, thanks for the additional context.
    – number41
    May 21 at 19:25
6

Side effects in your test data factory, in automation fired on Account insert, or interactions between those entities and your code under test can absolutely cause this kind of behavior change.

@testSetup executes in a mostly-isolated transaction, insulating you from many of the potential side effects and code interactions between your setup process and your actual code under test. Removing that barrier opens a wide range of potential sources of interference, such as static variables that are not reset after test initialization, limits that have already been partially consumed, enqueued asynchronous actions, possible Mixed DML traps, and so on.

I would find this behavior somewhat surprising but not unheard-of while working with legacy or poorly-architected code, as it sounds like you are here.

One specific example where you can get this kind of weirdness is low-quality trigger recursion guards based on static Booleans (people often call them something like runOnce). Insert the Account, the trigger runs once. Do something else with the Account.... the trigger doesn't run again, because you're still in the same transaction and the static didn't get reset.

1
  • Cheers for the feedback. We'll look into this! Feb 17, 2019 at 7:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.