Krdytkyu

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!

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.

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.

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.