Using try, catch and finally in test class (dreamhouseapp)

Question

In the test classes of the DreamHouse Sample app all the functions have the following structure:

static testMethod void testSomething() {
    Boolean success = true;
    try {
        ...
    } catch (Exception e) {
        success = false;
    } finally {
        System.assert(success);
    }
}

Source: https://github.com/dreamhouseapp/dreamhouse-sfdx/blob/master/force-app/main/default/classes/PropertyControllerTest.cls

What is the reason for or advantage of this structure?

I am asking, since this app is kind of Salesforce-official (if I got that right) and it says "Get inspired and learn best practices.", so I thought there must be a reason...

Thank you!

Answer 1

finally-assert is not your friend. Do not ever do this. Finally executes even if an exception is thrown, so the test will fail, but for the wrong reason. In any situation where it'd make sense to use finally, you can do it shorter without finally.

In fact, you should only use try-catch if you expect a specific exception:

try {
  doSomething();
  System.assert(false, 'Expected to get an exception');
} catch(SpecificException e) {
  // Good, but maybe System.assert for a specific message, etc
}

If no exception is thrown, you get an assertion failure, if you get the wrong exception, you get a failed test from the thrown exception, otherwise the test passes. finally might look pretty, but it's really just adding more code for no real reason. finally is useful in some situations, but not in unit tests.

Answer 2

The test framework reports exceptions as failures so let it do the work for you. Your question example then simplifies to:

@IsTest
static void something() {
    ...
}

This Simplicity in Software Design: KISS, YAGNI and Occam’s Razor blog post is relevant here and contains this lovely quote:

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

I see that not all the DreamHouse tests use that pattern - e.g. BotTest does not. Perhaps the try/catch/finally code was trying to express the idea that the test was there to check for exceptions. A comment would be a good way to communicate that.

Answer 3

Personally, I find the below structure clearer and it is the best practice we advise where I work.

Our Best Practice

SpecificException unexpectedException;
Test.startTest();
    try
    {
        // logic here
    }
    catch (SpecificException e)
    {
        unexpectedException = e;
    }
Test.stopTest();

system.assertEquals(null, unexpectedException, 'Informative message');

The reason we advise this pattern is that you should never put your test assertions within a conditional block, which executes only some of the time. Always make every assertion you intend. Note that if there is an exception, the assertion will show you a good level of detail.

There are variations of anti-pattern which violate this axiom but one typical example is below.

Anti-Pattern

Test.startTest();
    try
    {
        // logic here
    }
    catch (SpecificException e)
    {
        system.assert(false, 'Informative message');
    }
Test.stopTest();

This anti-pattern violates our axiom that each assertion should be unconditional.

The App

The pattern demonstrated in the app does avoid any violation of the axiom, so the pattern itself would probably pass our code review. However, it would be a marked improvement to cache the specific Exception instance rather than a Boolean flag, and in my opinion it is less clear to nest your blocks in this way.

One final note, we consider it firmly not best practice to catch generic Exception also colloquially called a "pokemon catch"). You should know what type of exception you expect, and handle just that. So while the try/catch/finally pattern would make it through code review (with comment), that aspect of the code would not.

Answer 4

I've looked at this from a slightly different perspective and raised the issue Structure of Apex test cases around Exception handling.

I agree with the other answers that the way the assertions are applied needs to be restructured to make the intent clearer and to highlight what the actual exception is better if the assertion fails.

IMHO there is a larger problem if you look at the actual PropertyController class that is being tested. Lets look at getPropertyList().

@AuraEnabled(cacheable=true)
public static Property__c[] getPropertyList(String searchKey, Decimal minPrice, Decimal maxPrice, Integer numberBedrooms, Integer numberBathrooms, String visualSearchKey) {
    String key = '%' + searchKey + '%';
    String visualKey = '%' + visualSearchKey + '%';
    return [SELECT Id, address__c, city__c, state__c, description__c, price__c, baths__c, beds__c, thumbnail__c, location__latitude__s, location__longitude__s FROM property__c
                  WHERE (title__c LIKE :key OR city__c LIKE :key OR tags__c LIKE :key)
                  AND (title__c LIKE :visualKey OR city__c LIKE :visualKey OR tags__c LIKE :visualKey)
                  AND price__c >= :minPrice
                  AND price__c <= :maxPrice
                  AND beds__c >= :numberBedrooms
                  AND baths__c >= :numberBathrooms
                  ORDER BY price__c LIMIT 100];
}

If you were to test that, what would be your first concern?

1. That it doesn't throw any sort of exception, or
2. That the Property__c records it returns are the ones you were expecting based on the arguments.

As David commented, to me the current test structure reads like a smoke test where the sole purpose is getting the required code coverage. There aren't any meaningful assertions being made about what the method is expected to do. If anything the pattern given looks like a way to always have an assertion present. That's usually encouraged in a good test case, but the second part of that is that it is asserting something useful.

As it is currently, you could replace the body of getPropertyList() with System.debug('The tests, they check nothing'); return null; and it would still pass. Not very useful.

If anything, this reminds me of a recent tweet by Steve Canon:

I bring this up every time someone thinks “full code coverage” means anything at all, but:

double sin(double x) { return x; }

void testSin() { assert( sin(0) == 0 ); }

The tests provided have barely touched all the possible scenarios in the method being tested.