Using try, catch and finally in test class (dreamhouseapp)
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.