Etydhv

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());

 

 

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.

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

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 often leads people in a bad direction, where they have great code coverage but no assurance that the requirements are being met.