cushon
commented
10 years ago Original comment posted by eaftan@google.com on 2013-08-06 at 12:53 AM
Adam is working on this. When the check is ready, I will review and integrate it.
Status: Started Owner: eaftan@google.com
Original issue created by adamwos@google.com on 2013-07-26 at 08:11 AM
There is a semi-common pattern of testing code that should throw exceptions that goes as follows:
try { somecode.thatShouldthrow.exception(); fail("didn't throw!"); catch (Exception e) { assert(e.getMessage().contains("bad foobar"); // or maybe doSomePostTest.cleanup(); }
However, sometimes people are overeager and catch (Throwable) or catch (Error), which is incorrect. fail(), if reached, throws an AssertionFailedError, which is immediately caught by the following catch because AssertionFailedError is an Error which is a Throwable. This makes the test not work as intended, i.e. instead of failing the test, the Error is caught and handled in the catch clause.
Test cases written with this pattern don't actually test what the author thinks they test. They're basically NOOPs, because the AssertionFailedError from fail() is immediately caught and ignored.
I propose an error prone check that would detect the following pattern
try { // any code here Assert.fail(); // as last statement in try } catch (Throwable whateverName) { // or catch (Error whateverName) // Doesn't matter if the catch block is empty or not. This shouldn't be done. }