Check that no verification/statements exist after a ExpectedException is thrown

[cushon]

Original issue created by wesalvaro@google.com on 2013-11-28 at 01:55 AM

---

I recently was changing my tests to JUnit4 and it was suggested to use an @Rule ExpectedException instead of my try/catch(expected).

But how do I verify things after my exception is thrown?

I created a Matcher that I expected as well that would verify the stuff after the exception, but that's clumsy and over-complicated. The fact of the matter is that ExpectedExceptions just don't work well when you need to verify things after the exception is thrown.

I pointed this out to a colleague who was taken aback and immediately searched and found places where ExpectedExceptions were used with verification done after the method threw its exception... Bad news: That verification was never done.

I searched the broader code base and found that he was not alone. It seems that the magic of ExpectedException has caused devs to be lax with how exceptions actually work. Nothing executes after the exception is thrown, of course, but I was able to find several instances of people doing work after the method throws. =(

There can be two varieties of this check:
Error: When there exists statements after the throwing function.
Warning: When there exists statements before the throwing function (i.e. between the throwing function and the call to ExpectedException#expect).

Both are bad with the latter being only bad test style, the former, however, is broken tests.

I'm not super familiar with how error-prone tests are created, but I was able to throw something together with a RegEx, so error-prone seems ideal.

[cushon]

Original comment posted by eaftan@google.com on 2014-04-07 at 10:49 PM

---

(No comment entered for this change.)

---

Labels: -Priority-Medium, Priority-Low

[eaftan]

Copying discussion from an internal bug report:

JUnit's ExpectedException causes execution to silently halt as soon as the exception is thrown. People unaware of this gotcha often put verify(), assert() or other test code after the line that throws the exception and these lines are never run (unless the exception doesn't get thrown, in which case the test fails anyway).

ErrorProne could check that there is no "test" code e.g. verify(), assert() etc. after an ExpectedException#expect() call. This isn't 100% correct, since the problem comes when those lines are after the exception is thrown, not expected. I can't immediately work out the precise pattern to only detect (all) code after the exception, not after the expectation, but maybe somebody smarter than me can spot a trick I've missed.

[kevinb9n]

Ironically, the only flaws that exist with the standard way of writing exception tests that we've been doing for 16 years are ones that error-prone can and does address (e.g. forgetting the fail()). If one is using good static analysis technology then how much value is there in little innovations like ExpectedException anyway?

[eamonnmcmanus]

I think a reasonable approach would be to have a warning (not an error) if there is more than one statement after an ExpectedException#expect() call, or more accurately after a group of ExpectedException#expect*() calls. That's the heuristic I used for an internal scan of Google's code base which found many problems and few false positives. Incidentally the problems were chiefly of one of two varieties: doing verify(), assert(), etc after the method that is supposed to throw the exception; or putting more than one exception-generating statement in the same test method (presumably thinking that ExpectedException somehow intercepts the exception).