9

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!

  • 3
    I'm curious to see if there's an answer to the contrary, but I don't see the advantage to that structure or think it's best practice. I'd call those smoke tests myself; the assertion proves nothing and actually hides an exception that might have included valuable information. – David Reed Dec 16 '18 at 0:27
  • 1
    If I was going to be pedantic I'd also call out the use of the deprecated testMethod rather than @IsTest. – Daniel Ballinger Dec 16 '18 at 22:30
  • I think Clean Code and Coding Best practices besides "Getting stuff done" is not on the Top List of any dev evangelist at Salesforce. ;-) – Robert Sösemann Jan 8 at 21:51
9

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.

4

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.

  • 1
    +1 This is so true. Why write 7 lines what you can write in 0? – sfdcfox Dec 16 '18 at 17:21
  • 1
    I wonder if they have mandated that all test methods should be making an assertion. These test cases look more like filler for coverage. PropertyController.getPropertyList() returns Property__c[]. IMHO it would be better to make assertions about those records. – Daniel Ballinger Dec 16 '18 at 22:23
1

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.

  • Interesting theory, but why do you advise this over just not using try-catch at all and just let the exception fail the test directly? This gives you an immediate error and stacktrace without the extra assertions, etc. I'd rather focus on writing assertions that prove everything worked successfully rather than checking for specific exceptions that we might get. – sfdcfox Dec 16 '18 at 1:31
  • Also, "This anti-pattern violates our axiom that each assertion should be unconditional." Don't you mean "conditional"? – sfdcfox Dec 16 '18 at 1:34
  • We've been over this already in comments on other threads. We contend that tests should only fail via assertion. I know you disagree. – Adrian Larson Dec 16 '18 at 2:23
  • Sorry, I must have forgot. I've had a lot on my mind recently. – sfdcfox Dec 16 '18 at 2:30
  • 1
    Haha no offense meant. It's just a bit of a standing dispute between us (though I think we agree to disagree). – Adrian Larson Dec 16 '18 at 2:50
1

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.

  • 1
    Sooo many arguments that need a custom object, which can throw a custom exception when the inputs are invalid. The test for that custom input object would include caught exceptions. The test for the SOQL code could then focus on the SOQL results, as you suggest. – snugsfbay Dec 17 '18 at 13:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.