CheckReturnValue: support more variants of "fail" in try/execute/fail/catch

[michaelhixson]

Recently I had to add some @SuppressWarnings("CheckReturnValue") to my code because I upgraded from JUnit 4 to 5, and Error Prone no longer recognized the try/execute/fail/catch pattern in my code. I eventually realized this is because CheckReturnValue looks for a specific set of "fail" methods and JUnit 5's is not one of them. See the relevant lines of AbstractReturnValueIgnored.

It took me a long time to figure out what was going wrong and I'd like to save other people from the headache.

Would you be willing to accept a pull request that adds support for each of the following?

• JUnit 5's fail method
• testng's fail method
• throw new AssertionError(...)

In order to add test cases that compile for JUnit 5 and testng, I think I would need to add those libraries as test-scope dependencies of the "core" module, in case that influences the decision.

[cushon]

Thanks for bringing this up. We'll probably want to add support for the new fail methods, but if you're using JUnit 5 I recommend using assertThrows instead of try/execute/fail/catch.

[jbduncan]

I agree with @cushon. assertThrows is the new "try/execute/fail/catch" in JUnit 5, so if migrating to assertThrows is feasible, then that'd be my recommendation.

[michaelhixson]

@cushon So are you saying I should not create the pull request?

I was not aware of the assertThrows method, thanks. Since assertThrows is (apparently) already supported by CheckReturnValue, perhaps it is worth updating the documentation to say so. I could also include that in a PR...

(Not to muddy the waters, but this chain of comments has me wondering whether a new bug pattern would be worthwhile - one that detects try/execute/fail/catch in JUnit 5 and testng code and recommends using assertThrows instead.)

[cushon]

We're happy to take a PR for this. There isn't a great way to add a test since Error Prone uses JUnit 4, but that's fine.

this chain of comments has me wondering whether a new bug pattern would be worthwhile - one that detects try/execute/fail/catch in JUnit 5 and testng code and recommends using assertThrows instead.

We have a refactoring for this but it hasn't been open-sourced yet. I can do that if there'd be interest. We also have checks for ExpectedException and the @Test(expected=...) pattern that can be adapted to refactor all uses of those APIs to assertThrows. We've been migrated those APIs to assertThrows more aggressively because they're easier to mis-use than try/fail/catch.

[jbduncan]

@cushon I'm personally interested in seeing these refactorings/checks open-sourced! But if it would take time away that would otherwise be spent on designing & coding up support for --release and Java 9, I'd personally prefer for those to take priority, if that would be both reasonable and feasible. :)

[michaelhixson]

I'm also interested in those assertThrows checks, for whatever that's worth.

[cushon]

The refactorings I mentioned earlier are now available: 42a9a65.

[cushon]

Also assertThrows was released in JUnit 4.13, so using it doesn't require JUnit 5 (or a snapshot release of JUnit 4) anymore.

I'd still take a PR to add more heuristics to AbstractReturnValueIgnored if anyone is interested in contributing that, but I'm going to close this we're not likely to get to it, since as mentioned above we're mostly using assertThrows.