2

I've got a theoretical, rather than a practical, question today. I just wrote the trigger and test class below. I'd love to get feedback on such questions as: 1. Do I test sufficient cases? (It gets 100% coverage, but that's not the same thing.) 2. Any comments on my style or method?

Trigger:

trigger OpportunityBeforeDelete on Opportunity (before delete) {

    String profileName = [SELECT Name FROM Profile WHERE Id = :UserInfo.getProfileId()].Name;

    for(Opportunity opp: trigger.old) {
        if (opp.Receivable__c == true && profileName != 'System Administrator'){
            opp.adderror('Receivable opportunities should not be deleted. See wiki: https://sites.google.com/a/sparkprogram.org/spark-interconnected-data-systems-wiki/finance/receivable-opportunities Receivable opportunities can only be deleted by a system administrator.');
        } 
    }
}

And this test class:

@isTest
public class testOpportunityBeforeDelete {
    //test class for trigger OpportunityBeforeDelete  

    @isTest
    public static void myTestMethod(){

        //insert a User that is not sysadmin
        Profile p = [SELECT Id FROM Profile WHERE Name='Spark Exec'];

        User standardu = new User(Alias = 'standt', Email='standarduser@sparkprogram.org',
                                  EmailEncodingKey='UTF-8', LastName='Testing', LanguageLocaleKey='en_US',
                                  LocaleSidKey='en_US', ProfileId = p.Id,
                                  TimeZoneSidKey='America/Los_Angeles', UserName='standarduser@sparkprogram.org');

        insert standardu;

        system.runAs(standardu){
            //Insert an account
            Account newaccount = new Account (
                Name = 'Account');
            insert newaccount;
            //Insert an opportunity
            Opportunity newopp = new Opportunity (
                Name = 'Test to be Receivable',
                AccountId = newaccount.Id,
                Amount = 10000,
                StageName = 'Receivable',
                Receivable__c = true,
                CloseDate = system.today()
            );
            insert newopp;

            // Perform test
            Test.startTest();
            try {
                delete newopp;
            }
            catch (DMLException e)
            {
                // expected - could assert the message here

            }



            Database.DeleteResult result = Database.delete(newopp, false);
            //Test.stopTest();
            System.assert(!result.isSuccess());
            System.assert(result.getErrors().size() > 0);
            System.assertEquals('Receivable opportunities should not be deleted. See wiki: https://sites.google.com/a/sparkprogram.org/spark-interconnected-data-systems-wiki/finance/receivable-opportunities Receivable opportunities can only be deleted by a system administrator.',
                                result.getErrors()[0].getMessage());

        }

    }
}
  • 2
    Per this meta post, we would advise you to ask your question on CRSE for a more in-depth code review. Do, however, feel free to post in our Salesforce Meta so that we can be made aware of it. Thanks! – sfdcfox May 14 '19 at 18:56
  • 2
    you should also do bulk testing (delete 2+ Opportunities). Note also that unless you have VR enforcing Oppos must have accounts, you can actually create oppos w/o accounts and thus make the test faster – cropredy May 14 '19 at 22:53
4

No, your coverage is not adequate, even if you hit every line. You should look up and read about Cyclomatic Complexity, and aim to cover every major branch. Specifically, you do not have a test method which verifies a System Administrator is exempted from the rule. Similarly, you should verify that all users are still able to delete records where Receivable__c == false.

In every unit test, you should have unconditional assertions which verify the behavior you expect. Typically to verify that a validation is thrown, mine looks like:

DmlException expectedException;
Test.startTest();
    try
    {
        // perform DML which should fail
    }
    catch (DmlException dmx)
    {
        expectedException = dmx;
    }
Test.stopTest();

system.assertNotEquals(null, expectedException, 'The operation should fail');
// optionally run a query and assert on those results as well

The converse:

DmlException unexpectedException;
Test.startTest();
    try
    {
        // perform DML which should fail
    }
    catch (DmlException dmx)
    {
        unexpectedException = dmx;
    }
Test.stopTest();

system.assertEquals(null, unexpectedException, 'The operation should succeed');
// optionally run a query and assert on those results as well
| improve this answer | |
  • 1
    Point taken that I need to test for a sysadmin being exempted. But it's not possible to prevent a delete with a validation rule, that's why I have to write a trigger. – Michael Kolodner May 14 '19 at 19:16
  • Ah good point, I did not look at the trigger event. – Adrian Larson May 14 '19 at 19:18
  • I see no point in actually checking the exception itself. The structure I use is basically try { /* call function(s) under test /; System.assert(false); } catch (...) { / expected exception was caught */ } when an exception is expected and just call the function(s) under test without try/catch if no exception is expected since that will fail the test anyway if an exception is thrown. This works well when doing unit testing where you do NOT string a lot of asserts one after another. – Phil W May 14 '19 at 22:02
  • You have a different philosophy, and that's fine, but mine (as stated in the answer) is that your tests should contain unconditional assertions. Your success case does not assert anything. Another aspect of my philosophy, though not explicitly stated, is that unit tests should only fail as a result of assertions. Uncaught exception failures are an anit-pattern in my book. – Adrian Larson May 14 '19 at 22:22
2

I wanted to point out a major hole in your logic, you probably will want to correct the pattern you used above.

Here's your original code with a note

try {
    delete newopp;
    //NOTE: WHAT HAPPENS HERE IF THE EXCEPTION IS NOT THROWN?
} catch (DMLException e){
    // expected - could assert the message here
}

If the error is thrown, your test has to fail... but you're not making it fail. So, with a small change, your test will correctly fail if your exception is not thrown

try {
    delete newopp;

    System.assert(false);
} catch (DMLException e){
    System.assert(true); //Not Needed, I just like doing it anyway
    // Assert everything else you need here
}

With that small change, you'll be validating correctly that an exception must be thrown.

| improve this answer | |
  • 1
    This can't be a validation rule because you can't prevent deletion with a VR, only an edit. (Unless you know something about VRs that I don't know.) – Michael Kolodner May 14 '19 at 19:18
  • 1
    I'll be damned, I missed that this was a deletion. Anyway, my tip is still valid – Sebastian Kessel May 14 '19 at 19:18
  • Updated my answer based on your feedback. – Sebastian Kessel May 14 '19 at 19:19
2

Another perspective on testing.

IMHO tests should confirm the planned behavior of the software: i.e. confirming customer visible/required behavior is priority #1. That is what your customer is paying for and wants to work. Start from those requirements when you write your tests, not from the code you have written to implement those requirements.

Sometimes you might write some tests that cover the building blocks that help you deliver that required behavior but that is essentially arbitrary implementation detail so is much less important.

Focussing on code coverage leads people in a bad direction, where they have great code coverage but no assurance that the requirements are being met.

If the tests are hard to write, then often the problem is with the code being tested and that should be refactored/improved. It's worth learning a bit about the core ideas of TDD (Test Driven Development) before writing lots of tests. See e.g. Martin Fowler's TestDrivenDevelopment post and the links there.

| improve this answer | |

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