0

I have a very strange issue with some Apex tests for a trigger. The tests each check a different type of field update; when one field is changed, the trigger is supposed to change another field.

On a few of these tests, the first field update kicks off the trigger, and I can see in debug logs that the trigger is firing, updating the second field, successfully updating the record, and exiting.

But, as soon as the trigger is done and context is back in the Apex test, it's as if nothing happened. No trace of any field updates. So of course, the assertions then fail.

The kicker is, this is even after querying for the records again.

The even bigger kicker is this same process works for some of the other tests.

The biggest kicker is one of the tests isn't requiring that I query for the changed records, they're just magically there, and the assertions do pass, so apparently the query afterwards shouldn't even be a required step.

So that I'm not speaking blindly, here is an example.

For anonymity purposes I have replaced custom field names, but their names should be self-explanatory

This looks like a standard method, because it is; we are using a "Trigger Framework" which essentially shotguns out one trigger to methods in external Apex classes. Helps prevent recursion/keep things organized, etc. The trigger behind it is an after trigger only, no before.

private void assignOpportunityOwnership()
{
    if (Trigger.isInsert || Trigger.isUpdate || Trigger.isUndelete)
    {
        Map<Id, Opportunity> oldOpps = new Map<Id, Opportunity>();

        if (Trigger.isUpdate)
        {
            oldOpps = (Map<Id, Opportunity>) Trigger.oldMap;
        }

        List<Opportunity> changed = new List<Opportunity>();

        for (Opportunity opp : (List<Opportunity>) Trigger.new)
        {
            Opportunity oldOpp = oldOpps.get(opp.Id);

            if (!oldOpps.containsKey(opp.Id) || oldOpp.Another_User_Lookup__c != opp.Another_User_Lookup__c)
            {
                Opportunity clone = opp.clone(true, true, true, true);
                clone.OwnerId = opp.Another_User_Lookup__c;
                changed.add(clone);
            }
        }

        Database.update(changed, false);
    }
}

And the Apex test that invokes this trigger:

@IsTest
static void assignOpportunityOwnershipTest()
{
    User user = 
    [
        SELECT Id, Name
        FROM User
        ORDER BY Id ASC
        LIMIT 1
    ];

    List<Opportunity> opps = 
    [
        SELECT Id, OwnerId
        FROM Opportunity
        ORDER BY Id ASC
    ];

    List<Id> originalOwners = new List<Id>();

    for (Opportunity opp : opps)
    {
        originalOwners.add(opp.OwnerId);
        opp.Another_User_Lookup__c = user.Id;
    }

    update opps;

    opps = 
    [
        SELECT Id, OwnerId
        FROM Opportunity
        ORDER BY Id ASC
    ];

    System.assertEquals(originalOwners.size(), opps.size());

    for (Integer i = 0; i < opps.size(); i++)
    {
        System.assertNotEquals(originalOwners.get(i), opps.get(i).OwnerId);
    }
}

I have already spent three days trying to understand what could possibly be causing these changes to not stick. The Database.update() call only ever returns success. I am pretty much out of any practical ideas here.

The only logical explanation I have left, is pointers. However it is a long shot since I have done this in other languages without a hitch. I am recycling a local variable, clone, to add the opportunities to the changed list. Perhaps each call to add() is only storing a pointer, not an actual instance of a record? However to proof this I made a variation of my original code that uses deepClone() with all IDs and data preserved on Trigger.new, instead of recycling a local variable, and modifies the deep clone's fields. DML statements still show as succeeding, but still no change back in the test, even still after querying again.

Some "inb4's" that I see coming and aren't viable:

"why not just make a process builder/flow/workflow rule"

The entire purpose of this project is to minimize the number of those in play. This particular environment has massed so many automations that they are beginning to kick each other off recursively, spinning off into the void and destroying governor limits. Hence why we are deploying a trigger framework to help limit recursion and better control execution order, etc. So no, "just process builder it" isn't a viable solution here.

"well editing the OwnerId is a bad idea isn't it? that's probably your problem"

Tricky part is, it isn't. This is one example, but another failing test simply renames the Opportunity to match a particular format, based on the AccountId field. I have already seen that the DML statements are working, and not throwing an exception because it "doesn't like me editing OwnerId", in both cases.

  • 3
    Using Database.update(changed, false); means that you are allowing for partial, or no success for your DML operation. If you change it to a simple update (update changed) do your tests throw any exception? – George S. Jul 3 at 15:04
  • 1
    "one of the tests isn't requiring that I query for the changed records, they're just magically there, and the assertions do pass" suggests that you are calling a method with an sObject instance and validating changes to that instance, rather than validating DML through a query. I suspect your issues here are heterogenous, and George S. has a good line on the other issue. – David Reed Jul 3 at 15:10
  • @GeorgeS. I have tried, and it neither throws an exception or shows any signs of failure. The records still appear to be changed within the trigger, but still unchanged after the trigger exits back into the test. – A stacked cube Jul 3 at 15:39
  • 2
    @Astackedcube Are you trying to update related records? Can you provide for us a complete, minimal reproduction of either issue? – David Reed Jul 3 at 15:54
  • 2
    From a unit test perspective, as I have mentioned before in other question answers, it would be better to put the code into an Apex class that is called from the trigger and write unit tests for that class rather than the trigger. The benefit of this is that you don't need to actually insert data into the database, which makes the tests more efficient, and more like unit tests, not integration tests. There are other possible benefits too but I won't cover those here. – Phil W Jul 3 at 16:42
0

I seem to have a thing for self-answers.

TL;DR

clone(true, true, true, true) should not be used in situations where multiple changes are happening in different places, as you risk overwriting a previous change. The safest solution is to simply use the new keyword and create a fresh instance, and only copy the ID. Then assign the fields afterwards.

A More Detailed Breakdown

After some senseless head banging and collaboration with those around me, this too is a PEBKAC error. Turns out, the issue lies in my use of clone, and I have simply forgotten some basic rules here.

A coworker asked "Why are you using clone?"

My answer was that you are not allowed to write to the contents of Trigger.new in an after trigger, you must make a new instance, carry over the ID, and update that instead.

The epiphany: If I'm using clone(true, true, true, true), that means yes, I preserve every inch of data in the record. Even fields I didn't necessarily query for.

Problem: If I have multiple functions in the handler, another clone(true, true, true, true) to the same Opportunity ID wipes literally any progress that was made before.

This is where my coworker's solution comes in. When he works with after triggers, he makes his "clone" with the new keyword and sets the ID field in the constructor. That's it. This wasn't even an issue he had known about, just something he has always done. And sure enough, after making that change, every problem test is now passing assertions with flying colors. Of course in hindsight the easier solution is probably to just stick to clone(true) and keep the other preserve flags off, but after talking through it I like his approach better. It is more clear on what is happening at a glance.

Code Samples

for (Opportunity opp : Trigger.new)
{
    // Old, broken for my case. This clone will wipe any
    // changes I may have made in other parts of the trigger.    
    Opportunity clone = opp.clone(true, true, true, true);
    clone.Other_User_Lookup__c = otherUser.Id;
    changed.add(clone);

    // New, fixed code. Simply creates a new Opportunity and
    // only moves the Id. Though this could be accomplished with
    // clone(true), this is probably just safer.
    Opportunity clone = new Opportunity(Id = opp.Id);
    clone.Other_User_Lookup__c = otherUser.Id;
    changed.add(clone);
}

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.