Part of the issue is in your @testSetup method.
You need to perform a dml insert on the padreConHijos case (at the very least) before trying to set the ParentId on hijo1 and hijo2. Records don't have an Id before they're inserted. There are a few situations where you can manually set an Id on a test record, but this is not one of them (because you're using a query in your trigger).
@testSetup
static void dataCreation() {
Case padreConHijos = new Case(Status = 'New', Origin = 'Phone');
Case padreSinHijos = new Case(Status = 'New', Origin = 'Phone');
// You only need to insert padreConHijos here, but might as well insert
// padreSinHijos too.
// It's good practice to group things into fewer DML statements where possible
insert new List<Case>{padreConHijos, padreSinHijos};
// After the insert has finished, the inserted records automatically have their
// Id field populated.
// This is the only field that is automatically populated.
// If you have a trigger/flow/process builder that causes other fields to be populated,
// you need to execute a query to retrieve those fields.
Case hijo1 = new Case(Status = 'New', Origin = 'Phone', ParentId = padreConHijos.Id, Description = 'hijo1');
Case hijo2 = new Case(Status = 'New', Origin = 'Phone', ParentId = padreConHijos.Id, Description = 'hijo2');
insert new List<Case>{hijo1, hijo2};
}
You only get coverage for code executed as part of a unit test
So if you expect a certain piece of code to be covered, and it isn't, then you know something is wrong.
In testCaseTrigger(), the query in for(Case c : [SELECT Id,CaseNumber FROM Case WHERE ParentId = null]) was returning 4 cases (both parents, and both children) instead of 2 cases (both parents) because your test setup ended up populating ParentId on the child cases before padreConHijos had an Id.
When you got to the first query in your trigger, in for(Case c :[SELECT Id, ParentId, CaseNumber FROM Case WHERE ParentId IN : Trigger.newMap.keySet()]){, it returned no results, so the body of the for loop was not executed.
Fixing the test setup should fix that issue, and get you coverage for the body of that for loop. It'll also ensure that you have a case where numeroDeCasosHijos in your second loop is not null (thus gaining coverage for that as well).
Code coverage should not be the focus of unit tests
Code coverage is the metric that Salesforce uses, and we as programmers need to care about it to some degree, but coverage isn't what makes a test useful to us.
Instead, you want to focus on whether or not the your code behaves as intended. That's what assertions help us do. A method that returns the result of 2 + 2 could have 100% coverage, but if it returns 5 as a result then the method is doing something wrong.
As for what you should be writing assertions for, you should look at the results, the output of running your code. Things like
- The return value from a method
- SObject records that were created (or deleted)
- Fields on SObject record(s) that were updated
- Whether or not a particular method in a helper class was called
- Changes to the "public state" (i.e. public class variables) in a class that you're testing
The main thing you're concerned about in this case is if the case Description is updated or not.
Try to write more, small tests instead of fewer, large tests
Smaller tests are generally easier to write, and having multiple tests makes it easier to see where issues are.
If you have a single, monolithic test method and it fails partway through, you'll only get a single error message. If your code has more than one issue, you'll need to fix the first error before you can see the second error.
With multiple tests, you have a chance to see multiple errors at once. You'll still probably end up fixing them one at a time, but having a better idea of the overall state of your code can help track down issues and make better decisions.
One of the other important reasons to write many, smaller tests is that if you test a wide range of scenarios, your code coverage will naturally be high.
What tests would I write for this?
- One test where you update a parent Case with children (assert that the description changed)
- One test where you update a parent Case with no children (assert that the description did not change)
- Perhaps one test where you update a child Case (assert that neither the child nor parent's description changed)
For that first test, I'd write it along these lines
@isTest
static void caseTrigger_UpdateParentWithChildren_UpdatesParentDescription(){
// Gather data from before running the trigger (or method) that you want to test
// so you have something to compare against
// Self-lookups (like ParentId on Case, which points to another Case)
// tend to need some additional processing because we can't perform a
// semi-join on the same object as the base query
Map<Id, Case> caseMapBefore = new Map<Id, Case>([SELECT Id, Description, ParentId FROM Case]);
Set<Id> parentIds = new Set<Id>();
for(Case c :caseMapBefore.values()){
parentIds.add(c.ParentId);
}
// My personal preference is to execute one remove() outside of a loop instead
// of putting something like "if(String.isNotBlank(c.ParentId))" inside a loop
parentIds.remove(null);
// Removing cases that have no children (because cases with children is
// what we're interested in for this test)
caseMapBefore.keySet().retainAll(parentIds);
// Step 2 of unit testing: execute the target code
Test.startTest();
update caseMapBefore.values();
Test.stopTest();
// Step 3 of unit testing: gather results and make assertions
Map<Id, Case> caseMapAfter = new Map<Id, Case>([SELECT Id, Description, ParentId FROM Case WHERE Id IN :caseMapBefore.keySet()]);
// Another important part of testing is to notice that you're asserting against things
// that happened as a result of running your code.
// If you were to set the case Description directly in this test method (and not call
// your trigger), an assertion that checks the description wouldn't tell you anything
// about how the code you're trying to test behaves.
for(Case caseAfter :caseMapAfter.values()){
Case caseBefore = caseMapBefore.get(caseAfter.Id);
// The third argument is optional, but you should always set it.
// It's the message that you'll be given if the assertion fails.
// Having a helpful message really helps to understand why and where a test failed
System.assertNotEquals(caseBefore.Description, caseAfter.Description, 'Updating a parent case with children should have caused the Description to be updated');
}
}
Parting advice
Once you're comfortable with writing triggers like this (and writing tests), then the next step you should take is to use a trigger framework.
Best practice here is to have only one trigger per object (you can have multiple triggers, but they are not guaranteed to be run in any particular order), and to keep the trigger logic-free.
Triggers are harder to test than Apex classes. Triggers can only be executed by performing DML. As you continue to customize your org, the number of requirements that you have to satisfy to create your test data will increase.
Ideally, your trigger would look something like this
trigger case on Case (before insert, after insert, before update, after update, before delete, after delete, after undelete){
MyFramework.run(trigger.old, trigger.new, trigger.operationType);
}
with MyFramework.run() using the trigger operation type to decide which handler method to call, and the handler method dictating the code to be run (or classes to be executed).
Calling a method in an Apex class is more flexible. If you have the handler methods take lists and/or maps instead of directly using trigger context variables, then your code is independent of the trigger (which can really help if you need to set up a situation for a test that would otherwise be difficult or impossible to do).
