Better practice to analyse and quantify code coverage with OR conditions

Question

Test Coverage:

I have 100% test coverage for my code base. I believed that, it is sufficient enough to deploy my code in Production and it is trust worthy that there won't be any bugs.

Issue:

But I had a very critical bug reported by my customer, which happened because of the code below:

 Id tmpId;
 if(account.Id == account1.Id || accountMap.get(account.Id) != null) {
    tmpId = accountId;
 }

• I always had the condition account.Id == account1.Id satisfied, so it didn't execute the accountMap.get(account.Id) != null statement anytime
• But still since either one of condition is satisfied the code is assumed to be covered and it gives 100% code coverage

Real Usage:

• When there was a real condition with account.Id != account1.Id the code raised unable to de-reference null exception
• This happened because of the case that accountMap didn't contain the key and Map#get(key) method will raise an exception
• I have replaced the Map#get(key) with Map#containsKey(key) to correct this (which will return boolean instead of raising exception on absence of key)

Question:

• This bug escaped my eyes and even went into production because of the blind faith on Code Coverage
• So my question is how to cover OR conditions effectively so that both LHS and RHS of OR condition will be covered ?
• If we can't completely rely on Code Coverage what is the better practice to cover this kind of bugs in an automated fashion ?

Answer 1

While there are already two fine answers already posted, I want to throw in my two cents. The most important step you can take to make sure this does not happen again is to use effective assertions in your test. Use system.assert methods more liberally to ensure functionality works as expected.

I recommend reading the whole thing, but here are a couple interesting tidbits from How to Write Good Unit Tests (emphasis mine):

The Value of Unit Tests

One of the most valuable benefits of unit tests is that they give you confidence that your code works as you expect it to work. Unit tests give you the confidence to do long-term development because with unit tests in place, you know that your foundation code is dependable.
...

Verify the results are correct

Verifying that your code works as you expect it to work is the most important part of unit testing. It’s also one of the things that Force.com developers commonly neglect. Unit tests that do not verify the results of the code aren’t true unit tests. They are commonly referred to as smoke tests, which aren’t nearly as effective or informative as true unit tests.

A good way to tell if unit tests are properly verifying results is to look for liberal use of the System.assert() methods. If there aren’t any System.assert() method calls, then the tests aren’t verifying results properly. And, no, System.assert(true) doesn’t count.

Properties of Well-Written Unit Tests

Well-written unit tests are thorough, repeatable, and independent. Let’s examine each of these properties in turn.

Thorough

The most important aspect of well-written unit tests is that your unit tests must be thorough. The Force.com platform requires 75% code coverage, but this is a minimum and should not be your end goal. You should only be satisfied with your unit tests when you’re confident that your unit tests thoroughly verify that your code works as we expect it to work. To do this, your tests must use the various System.assert() calls. Unit tests should exercise the expected as well as the unexpected conditions in your code. Special attention should be paid to areas of code that are particularly likely to break.

---

One other change you can make is to test more input scenarios. One more snippet:

Broadly speaking, you should test your custom business logic. How thoroughly you test that business logic will probably vary between situations. On one end of the spectrum, you might choose to implement just a few tests that only cover the code paths that you believe are most likely to contain a bug. On the other end of the spectrum, you might choose to implement a large suite of unit tests that are incredibly thorough and test a wide variety of scenarios. Wherever a given project falls on that spectrum, you should be sure to write unit tests that verify your code behaves as expected in "normal" scenarios as well as in more "unexpected" scenarios, like boundary conditions or error conditions.

Answer 2

Not specific to this scenario, the below applies in general

• Don't write Unit tests with focusing only code coverage as target.
• We should focus on writing tests with both positive and negative scenarios, then ideally your edge cases gets picked up..
• It's definitely a time consuming exercise but it's worthy effort. So don't skip writing additional test methods covering multiple conditions, branching logics.

You can refer the Apex Unit Testing Best Practices for complete guidance on what test classes should go thru..

The testing best practices reference has answer to your question in specific

In the case of conditional logic (including ternary operators), execute each branch of code logic.

Answer 3

First, your Real usage seems wrong to me:

• This happened because of the case that accountMap didn't contain the key and Map#get(key) method will raise an exception
  • False! If a Map does not contain a key, it will simply return null
• I have replaced the Map#get(key) with Map#containsKey(key) to correct this (which will return boolean instead of raising exception on absence of key)
  • False! You will still (most) probably get unable to de-reference null object error

What seems to be the real problem to me(given that it does fail at this line):
Your accountMap object is null, so what actually happens in the RHS of this statement is:

null.get(account.Id) != null

And this leads to the error you are getting. Or it can be that account and/or account1 variables are null which will also raise unable to de-reference null object. To quickly test this I used this code in Execute Anonymus window of Developer Console:

Map<String, String> testMap = new Map<String, String>();
testMap.put('one', 'one');
if (testMap.get(null) != null) {
    System.debug('Not null');
} else {
    System.debug('Null');
}
testMap.put(null, 'null');
if (testMap.get(null) != null) {
    System.debug('Not null');
} else {
    System.debug('Null');
}

The output is very predictable:

14:09:30:006 USER_DEBUG [6]|DEBUG|Null
14:09:30:006 USER_DEBUG [10]|DEBUG|Not null

This proves that calling get() method with null parameter will actually return null and not the exception.

---

Now about test classes.

There is a reason they are called TEST classes and not coverage classes. You are supposed to:

• Test all(or at least most) possible scenarios
• Test both positive AND negative scenarios
• Try to break your code in your test class so that it fails somewhere. And if you succeed to break your code, be aware that this can also happen in production, so go ahead and fix it
• Think as a beginner who has no idea how it works, so test all possible scenarios
• Use System.assert() to check that your test classes actually work. Without them, test classes are (pretty much) useless
• Focus on testing of your code and not focus on getting enough coverage. If you test all possible positive and negative scenarios and you still have uncovered code you should ask yourself: is this code even referenced anywhere?

Answer 4

Part of code coverage is branch coverage. I'm not sure about salesforce (this question hit the 'hot network questions' list), but most code coverage tools I've seen also report branch coverage. If your tool doesn't do that, you may want to manually record which branches are executed and whether all true/false choices are followed.

Typically, your code has relatively few branches compared to lines of code, so it's usually straightforward to get high branch coverage. However, one complicating factor is the complexity of your code, which broadly speaking is asking "How many branching statements does a given part of code have?"

Regardless of whether your conditional statements are &&s or ||s, you need to test every conditional statement for its true and false values.

This blog post discusses two metrics that are related to branches - NPath and Cyclomatic Complexity. The goal of measuring these two is usually to reduce the size of functions and improve testability.

For example, consider having 10 conditional statements. If they're all in one function, then you need to create 2^10 tests to check every permutation of conditions. If you instead break up that big function into 10 smaller functions with 1 condition each, then you need 2 tests per function for a total of 20 tests - far better than 1,024 tests.

I'm not familiar with salesforce or its code coverage tool(s), but wanted to introduce some of the vocabulary about branch coverage. As you've found out the hard way, missing the execution of even a single branch can lead to bugs.