2

I've had a fairly simple run in writing test classes, but am not sure if I'm doing it right or adhering to best practices.

Below you'll find one of my triggers and its corresponding test class (100% coverage). Please let me know if what I'm doing is sensible or what you would suggest otherwise.

Trigger:

trigger PopulateDays on Event__c (after insert, after update) {
    List<Day__c> insertDays = New List<Day__c>();
    
    if (Trigger.IsInsert || Trigger.IsUpdate) {
        for (Event__c ev : Trigger.new) {
            if (ev.Number_of_Days__c != null) {
                for (integer i = 0; i < ev.Number_of_Days__c; i++) {
                    Day__c day = New Day__c();
                    day.Event__c = ev.Id;
                    day.Name = 'Day ' + (i + 1);
                    day.Date__c = ev.Event_Date__c.addDays(i);
                    insertDays.add(day);
                }
            }
            insert insertDays;
        }
    }
}

Test Class:

@isTest
public class TestPopulateDays {
    @isTest static void TestPopulateDaysWithOneEvent() {
        Event__c ev = new Event__c (Name = 'Test Class Event',
                                   Client_Name__c = 'Test Class Client',
                                   Event_Date__c = Date.newInstance(2020, 12, 25),
                                   Number_of_Days__c = 3);
            insert ev;
    }
}

Context: To give you some context, the trigger is between a parent-child relationship ("Event" - "Day"). When entering the Number_of_Days__c into the Event__c object, a series of Day__c records populate corresponding to the number of days entered along with their associated dates.

P.S. If you could provide some questions to ask myself when writing a test class or what method you use, that'd be great. Thank you in advance.

1
  • 1
    Hi, and welcome to SFSE! I'd like to take a moment to say that you've got a really nice first question, and also to invite you to read about How to Ask and take the tour if you haven't already done so. We hope you enjoy your experience here!
    – sfdcfox
    Oct 28, 2020 at 20:03

2 Answers 2

3

You have at least three problems with your test.

First, you're not making any assertions. Second, you didn't test in bulk. Third, you didn't actually test the update logic, even though you have 100% coverage.

How do you know the Day__c records are created?

You haven't tested it to validate it:

System.assertEquals(3, [SELECT COUNT() FROM Day__c]);

How do you know your code will work if someone enters data in bulk?

You need to insert at least two records (preferably more like 200, if possible), because you want to make sure you don't have logic bugs in your code--and you do.

Try this test:

@isTest
public class TestPopulateDays {
    @isTest static void TestPopulateDaysWithOneEvent() {
        Event__c[] events = new Event__c[0];
        for(Integer i = 0; i < 200; i++) {
            events.add(new Event__c (Name = 'Test Class Event',
                                   Client_Name__c = 'Test Class Client',
                                   Event_Date__c = Date.newInstance(2020, 12, 25),
                                   Number_of_Days__c = 3));
        }
        insert events;
    }
}

It will fail because you have your insert operation inside the for loop.

How do you know updates work?

You haven't explicitly tested updates. How do you know it works correctly?

Your test, at minimum, should look like this:

@isTest
public class TestPopulateDays {
    @isTest static void TestPopulateDaysWithOneEvent() {
        Event__c[] events = new Event__c[0];
        for(Integer i = 0; i < 200; i++) {
            events.add(new Event__c (Name = 'Test Class Event',
                                   Client_Name__c = 'Test Class Client',
                                   Event_Date__c = Date.newInstance(2020, 12, 25),
                                   Number_of_Days__c = 3));
        }
        insert events;
        System.assertEquals(3, [SELECT COUNT() FROM Day__c WHERE Event__c = :events[0].Id]);
        update events;
        System.assertEquals(6, [SELECT COUNT() FROM Day__c WHERE Event__c = :events[0].Id]);
    }
}

Note that you're duplicating Day__c records on update. Are you sure you meant to do this? You may need to revise your code to check for existing days already (and update the unit test to match).

Bonus points if you also validate that the dates you expect to be linked to are actually all accounted for. That part looks right, but you'd want to validate as much output as possible to avoid regression bugs.

1

Setting aside the DML inside of a loop (DML and queries should almost never appear in a loop, and certainly not inside of a loop over Trigger.new, Trigger.newList, etc...), the big question you should be asking yourself is this:

What output do I get from running the code I'm trying to test?

"Output" here means things like the return value of calling a method, changes to the public state of a class, insertion/update/deletion of records in the database, and maybe a few other things I'm forgetting.

Whatever output you can see, you should make an assertion (or several assertions) for.

Code coverage is what Salesforce requires (it's realistically the only thing they can require), but assertions are where the true value of unit tests lie. Assertions tell you that your code ran successfully and that you got the expected result. If you don't have assertions, then the only thing you know is that your code didn't crash and burn (i.e. it didn't throw an exception that ended up being uncaught).

The term for a test without assertions is "smoke test". It's not completely useless, but it doesn't prove that there isn't a fire (semantic errors).

Consider this contrived example (pretend it's in a class):

public static Integer add2(Integer input){ return input + 3; }

You can write a simple test

@testMethod static void testAdd2(){ Integer result = MyClass.add2(3); }

You'll have 100% coverage, but 3 + 2 = 5, and this method would return 6. Something is obviously wrong, but you won't know that just by running the test unless you have assertions.

After that, the next question you should be asking yourself when writing tests is

What input am I giving to the code I'm trying to test?

It'd be nice if our code was called with precisely the correct input every time, but reality tends to be less kind.

If your code assumes that a value is provided, that's a chance for an exception to be thrown (nullPointerException, argument cannot be null, no rows to assign to SObject, etc...).

Assertions help show that your code produces the correct output, but it doesn't do much to show that your code is robust (i.e. able to work and produce some output under a variety of circumstances). The opposite of robust code is "fragile" code (code that is very sensitive to the input it's given and/or breaks easily or frequently).

Providing a variety of inputs to the code you're testing helps to show that your code is robust. Fragile code is no fun to work with, and fragile code is harder to use in a larger project.

Applying this general thought to your specific example, what happens if a user deletes the Event_Date__c field from an event?

The answer is that you'd get a null pointer exception (Event_Date__c is null, null has no methods that can be called, so your code effectively becomes null.addDays(i). null.something gives you a null pointer exception).

Now, if Event_Date__c is a required field, then you don't have to worry about this particular scenario. The broader point is that if you make assumptions about the input you get, you open yourself up to various failures.

  • If a method takes an argument, robust code will check to make sure it's not null before it is used later in the method
  • If some code depends on a field on an SObject being populated, robust code will check to make sure it's not null before it gets used
  • If your code takes a string as input, and there's a chance that you need to worry about case sensitivity (Email type fields, or things put into a Set or used as the key of a Map), robust code will normalize the string (use toLowerCase() or toUpperCase())

Also, generally speaking, you'll want to have separate test methods. One for each input variation you're testing. Smaller and more numerous tests tend to be easier to write, and if there are multiple issues, seeing which tests fail can give you a decent idea of what the problem is. You could eventually solve a problem if everything is in a single test method, but it'll typically be a harder and longer process.

Testing different inputs also leads naturally to getting coverage for things like if/else statements (one test provides input to guarantee that you get into the if block, another test provides input to guarantee that you get into the else block).

Finally, by testing a sufficient number of inputs, you will naturally gain high code coverage. 75% is the minimum for Apex classes (triggers can require as low as a single line of coverage), and 100% coverage isn't always possible (or realistic).

You can set a target coverage (say, 85% to give yourself a nice buffer), and then continue to add test methods for different input scenarios until you hit that number.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .