For being new to Apex and unit testing, I'd say you did pretty well. There are, of course, issues, but we'll work those out.
The first big thing that I'm seeing with your trigger is that you're querying inside of a loop. Queries inside of loops are bad, and will cause problems (eventually) because you can only use 100 queries (under normal circumstances) per transaction.
The general pattern to use instead is something like...
// Declare a collection (usually a list or a set) to hold Ids that we want to use
// in the query.
List<Id> myRecIds = new List<Id>();
// Iterate over Trigger.new to gather the Ids
for(MyObj__c myRec :Trigger.new){
myRecIds.add(myRec.someLookup__c);
}
// Perform your query, and then loop over the results.
for(OtherObj__c otherRec :[SELECT Id, someField__c FROM OtherObj__c WHERE Id IN :myRecIds]){
// Do interesting things in here
}
That said, we can do something a little different here. Since Contact has a lookup to Account, we say that Account is the "parent" object and Contact is the "child" object. We can perform a parent-child subquery (aka a left outer join). This provides a few benefits, such as having each related Contact automatically correlated to its parent Account record and being able to determine how many Contacts an Account has without an additional query.
That query looks like this:
// The open/close parenthesis enclose the subquery.
// The subquery is placed in the SELECT clause, just like it were a field
// The FROM clause of the subquery uses the child relationship name, which is
// 'Contacts' in this case
List<Account> acctsWithContactsList = [SELECT Id, Total__c, (SELECT Id FROM Contacts) FROM Account WHERE Id IN :Trigger.new];
We can use that to simplify your trigger.
trigger claimTest on Account (after update) {
// DML inside of a loop is also a bad idea, declare a collection to hold the
// Contact records we want to update.
List<Contact> contactsToUpdate = new List<Contact>();
// Using a query to feed a loop like this is called a "soql for loop"
for(Account a : [SELECT Id, Total__c, (SELECT Id FROM Contacts) FROM Account WHERE Id IN :Trigger.new]){
if(a.Total__c != Trigger.oldMap.get(a.id).Total__c){
// The subquery gives us a list of the child records.
// We can access this list using the child relationship name (Contacts).
// Since it is a list, we can use the size() method to get the number
// of related contacts.
// Directly using subquery results like this only works (reliably) if
// you have less than about 200 child records per parent record.
// If you have more than that, you'll need to use a nested loop.
Integer numContacts = a.Contacts.size();
// Now that we know the number of contacts for this account, we can go about
// updating Contacts.
for(Contact cont :a.Contacts){
// Normally, a check to make sure we won't divide by 0 is a good idea.
// However, in this case, we won't enter this loop if there are 0
// contacts, so it's safe to ignore the check in this case.
cont.Share__c = a.Total/numContacts;
// Add the contact to our list of things to update
contactsToUpdate.add(cont);
}
}
}
// Outside of all loops, it's safe to do DML
update contactsToUpdate;
}
As far as your unit test goes, the big thing you were missing was already covered by Himanshu. To assert whether or not your Contacts were updated with the correct amount in Share__c, you need to re-query the Contacts. Other than that, your test has all the important things:
- Creating data inside of the test itself
- Test is not overbroad, focusing on limited input to a small unit of code
- Assertion to verify the result
About the only other thing I can say about your test class is that I'd like to see more test methods in it. Right now, you're testing the 'happy path' where your data is well-formed and won't cause any problems. The real world (once you get actual users involved) is more harsh. Writing tests to cover less-than-ideal situations will give you confidence that your code is not only correct, but that it can also stand up to abuse (i.e. your code is 'robust').
Some additional tests that would be good to write:
- What happens when
Account.Total__c becomes 0 or negative?
- What happens when
Account.Total__c becomes null?
- What happens if your
Account has no Contacts?
- What happens if your
Account has only one Contact?
- What happens if your
Account has a couple hundred Contacts?
- What happens when
Account.Total__c can't be evenly divided (e.g. total = 100, 3 Contacts)
Each of those would be a different test method.
