2

I am trying to write a test class to test my handler class that has 3 methods. One handles creation of new records. One handles if the record was edited, and one deletes related records. I'm trying to test that when the record is updated the handler's actions actually work.

I can see that if I use a real record it works as expected. However, in code it does not. There are no error messages just my assert does not equal what I expect.

System.AssertException: Assertion Failed: Expected: 50, Actual: 100.00

Example Works, record is created and generates schedule as expected:

@isTest static void testCreateSchedule() {

    // Create Revenue Pipeline record
    Revenue_Pipeline__c revPipe = new Revenue_Pipeline__c();
        revPipe.Weighted_Product_Price__c = 1200;
        revPipe.Product_Start_Date__c = date.newInstance(2016, 08, 1);
        revPipe.Product_End_Date__c = date.newInstance(2017, 07, 1);
        revPipe.Name = 'Test Pipeline';
        revPipe.CurrencyIsoCode = 'USD';
    insert revPipe; 

    // Get the Revenue Pipeline record with the schedule
    Revenue_Pipeline_Schedule__c revPipeS = [SELECT Id, Amount__c
                                             FROM Revenue_Pipeline_Schedule__c
                                             WHERE Date__c = :date.newInstance(2016, 10, 1)
                                             LIMIT 1];  

    // Schedule record should be 1200/12 or 100
    System.assertEquals(100, revPipeS.Amount__c);
}

Example does not work, record is created but does not update the test record:

@isTest static void testCheckRecord() {

    // Create Revenue Pipeline record
    Revenue_Pipeline__c revPipe = new Revenue_Pipeline__c();
        revPipe.Weighted_Product_Price__c = 1200;
        revPipe.Product_Start_Date__c = date.newInstance(2016, 08, 1);
        revPipe.Product_End_Date__c = date.newInstance(2017, 07, 1);
        revPipe.Name = 'Test Pipeline';
        revPipe.CurrencyIsoCode = 'USD';
    insert revPipe;

    // Edit Revenue Pipeline record to change the Price
    Test.startTest();
    revPipe.Weighted_Product_Price__c = 600;
    update revPipe;
    Test.stopTest();

    // Get the Revenue Pipeline record with the schedule
    Revenue_Pipeline_Schedule__c revPipeS = [SELECT Id, Amount__c
                                             FROM Revenue_Pipeline_Schedule__c
                                             WHERE Date__c = :date.newInstance(2016, 10, 1) AND
                                             Revenue_Pipeline__c = :revPipe.Id
                                             LIMIT 1];      
    // Schedule record should be 600/12 or 50
    System.debug('Amount is:: ' + revPipeS.Amount__c);
    //System.assertEquals(50, revPipeS.Amount__c);
}

On After Insert this creates Schedule records.

public static void createRevSchedule(Map<Id, Revenue_Pipeline__c> newMap)
{
    List<Revenue_Pipeline_Schedule__c> newSchedule = new List<Revenue_Pipeline_Schedule__c>();
    for(Revenue_Pipeline__c revPipe : newMap.values()) {
        Integer numOfMonths = revPipe.Product_Start_Date__c.monthsBetween(revPipe.Product_End_Date__c) + 1;
        Integer n = revPipe.Subscription_Type__c == 'Subscription' ? 1 : 0;
        for(Integer i=0; i < numOfMonths; i++) {
                Revenue_Pipeline_Schedule__c revSchedule = new Revenue_Pipeline_Schedule__c();
                revSchedule.Amount__c = revPipe.Weighted_Product_Price__c/numOfMonths;
                revSchedule.Date__c = revPipe.Product_Start_Date__c.addMonths(i + n);
                revSchedule.Name = revPipe.Name + ' - ' + revSchedule.Date__c.year() + '-' + revSchedule.Date__c.month();
                revSchedule.Revenue_Pipeline__c = revPipe.Id;
                revSchedule.CurrencyIsoCode = revPipe.CurrencyIsoCode;
                newSchedule.add(revSchedule);           
        }
    }
    if(newSchedule.size() > 0){
        try{
            insert newSchedule;             
        } catch (DmlException ex) {
            ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.INFO, ex.getMessage()));
        }
    }
}

On After Update this checks if one of the fields changed and sends to delete method.

// Checks if the fields were changed in the newMap compared to OldMap
public void checkUpdate(Map<Id, Revenue_Pipeline__c> newMap, Map<Id, Revenue_Pipeline__c> oldMap)
{
    Map<Id, Revenue_Pipeline__c> revPipeline = new Map<Id, Revenue_Pipeline__c>();

    // Check if the fields changed
    for(Revenue_Pipeline__c revPipe : newMap.values())
    {
        if(
            revPipe.Product_Start_Date__c       != oldMap.get(revPipe.Id).Product_Start_Date__c ||
            revPipe.Product_End_Date__c         != oldMap.get(revPipe.Id).Product_End_Date__c ||
            revPipe.Product_Price__c            != oldMap.get(revPipe.Id).Product_Price__c ||
            revPipe.Weighted_Product_Price__c   != oldMap.get(revPipe.Id).Weighted_Product_Price__c)
            {
                revPipeline.put(revPipe.Id, revPipe);
            }
    }
    if(revPipeline.size() > 0)
    {
        deleteSchedule(revPipeline);
    }
}

If there is a change to the the fields this deletes the schedules and sends back to the create method to create new schedule records.

// Delete Revenue_Pipeline_Schedule records 
public void deleteSchedule(Map<Id, Revenue_Pipeline__c> revPipeline)
{
    List<Revenue_Pipeline_Schedule__c> scheduleToDelete = [SELECT Id,
                                                                 Revenue_Pipeline__r.Id
                                                         FROM Revenue_Pipeline_Schedule__c
                                                         WHERE Revenue_Pipeline__r.Id In :revPipeline.keyset()];
    try{
        delete scheduleToDelete;        
    }   
    catch (DmlException ex){
        ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.INFO, ex.getMessage()));
    }
    createRevSchedule(revPipeline);
}
  • 2
    I think we need to see the code that creates your Revenue_Pipeline_Schedule__c records... Also imo magic numbers should be avoided. Much better to use a descriptively named constant than some magic number (e.g. Integer expectedAmount = weightedPrice/12). Developers may delete your comments. Also make sure you include assertion messages! – Adrian Larson Jan 18 '17 at 15:30
  • I updated, So if you do your example then you can assert on the expectedAmount instead of the number 50.. good thought. – Dan Wooding Jan 18 '17 at 15:41
  • 1
    What does deletion have to do with any of this? You still haven't shared any code that would change the schedule amount when you update the parent pipeline. – Adrian Larson Jan 18 '17 at 15:43
  • Instead of trying to figure out what schedule records would need to be updated if there is a change in the 'checkUpdate' I delete the schedule record and create a new one based on the updated information on the parent. So I would never update the schedule record it's either created or deleted. – Dan Wooding Jan 18 '17 at 15:47
  • 1
    Also, it might not be completely appropriate to test this (as it feels more like an implementation detail than a system requirement that needs to be verified), but it may also be worth your time in testCheckRecord to query for the Revenue_Pipeline_Schedule__c records that exist before you update your revPipe, and make an assertion that the old revPipeSchedule records have indeed been deleted (which can be done in a number of ways). – Derek F Jan 18 '17 at 16:48
2

Big Picture

When you are implementing complex functionality, Separation Of Concerns and a proper suite of Unit Tests can really help you pinpoint critical aspects of your functionality that can be difficult to catch when you're only looking at the big picture.

Individual Concerns

Right now, you simply test that the trigger does what you expect. But really, there are a bunch of small steps, not just one big one.

  1. Fire your after update trigger
  2. Filter parent records that should be processed
  3. Query child records
  4. Delete child records
  5. Handle delete exceptions
  6. Create new child records
  7. Handle create exceptions?

You have actually co-mingled 2-5, (and chained 5 to 6) which doesn't help you write simple, effective tests. For all we know, the error is really in step 3. But you never directly test that concern!


Filter

I recommend the first step for you is to separate out the filter and test it rigorously:

public Map<Id, Revenue_Pipeline__c> needsNewSchedules(/*parameters*/) { /* filter logic */ }

Now you can test your filter mechanism very rigorously:

static testMethod void testNeedsNewSchedule_WeightedPriceChanged() { /* test logic */ }
static testMethod void testNeedsNewSchedule_StartDateChanged() { /* test logic */ }
static testMethod void testNeedsNewSchedule_EndDateChanged() { /* test logic */ }
static testMethod void testNeedsNewSchedule_PriceChanged() { /* test logic */ }

Delete

I also recommend you decouple the delete and create methods, and call them separately from your trigger/handler.

public void afterInsert()
{
    List<Revenue_Pipeline__c> filtered = RevenuePipelineService.needsNewSchedules(
        newMap, oldMap
    );
    RevenuePipelineService.deleteSchedule(filtered);
    RevenuePipelineService.createSchedule(filtered);
}

Now testing the delete mechanism is much simpler.

static testMethod void testDeleteSchedule()
{
    // setup data
    Test.startTest();
        RevenuePipelineService.deleteSchedule(pipelineRecords);
    Test.stopTest();
    system.assertEquals(0, [SELECT count() FROM ChildObject__c], '<assertion_message>');
}

This pattern will make it much easier for you to test different execution paths for each method.


Handle Exceptions

If you move out your error handling, testing becomes much easier, because instead of passing queried records, you can pass some you know will fail to delete (using dummy ids).

static testMethod void testSafeDelete()
{
    List<MyObject__c> records = new List<MyObject___c>
    {
        new MyObject__c(Id=SObjectType.MyObject__c.getKeyPrefix().rightPad(15, '0')
    };

    DmlException expectedException;
    Test.startTest();
        try { MyObjectService.safeDelete(records); }
        catch (DmlException d) { expectedException = d; }
    Test.stopTest();

    system.assertNotEquals(null, expectedException, '<assertion_message>');
}

You can then simplify the body of your deleteSchedule method to:

safeDelete[/*child query*/]);

Also note that ApexPages.addMessages is not an appropriate error handling mechanism from within a trigger. You should map errors back to the parent.

try
{
    delete children;
}
catch (DmlException d)
{
    for (Integer i = 0; i < d.getNumDml(); i++)
    {
        Id parentId = children[i].Parent__c;
        parentId.addError(d);
    }
}

Closing Thoughts

There's more yet to explain, but this post is already practically a book, and I think by now you get the idea. Separate your functionality into smaller chunks, test each rigorously, and you will find the breakdown. One last note, though, is that if you really want to stick with a delete-then-create strategy, you should strongly consider Transaction Control

  • This is a lot to take in which is good. When you say parameters here: public Map<Id, Revenue_Pipeline__c> needsNewSchedules(/*parameters*/) { /* filter logic */ } that is the map that is coming from the after insert? – Dan Wooding Jan 18 '17 at 17:06
  • @Dan you'll need trigger.oldMap as well as trigger.newMap (or really just trigger.new). – Adrian Larson Jan 18 '17 at 18:21
  • 1
    I'm going to ask a new question. I'm getting compile errors that's not directly tied to the original question. – Dan Wooding Jan 18 '17 at 18:27

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.