Add assertThrows overloads with ThrowingSupplier

[marcphilipp]

This PR aligns Assert with Jupiter's Assertions class by adding overloaded variants of assertThrows that take a ThrowingSupplier instead of a ThrowingRunnable which was originally proposed and implemented by @cushon in junit-team/junit5#1401.

If the given ThrowingSupplier fails to throw the expected exception and instead returns a value, the string representation of the value is included in the failure message to aide debugging.

[kcooney]

I am concerned that Java will not be able to tell which version to call, and produce an error. Which versions of the compiler has this been tested on?

[panchenko]

Java 8 is able to distinguish such things, based on having a return value or not.

[cushon]

I think this has the same issue that was discussed in junit-team/junit5#1414 about making some call sites using method references ambiguous:

// error: both method <T#1>assertThrows(Class<? extends T#1>,Runnable) and
// method <T#2>assertThrows(Class<? extends T#2>,Callable<?>) in Z match
assertThrows(ExecutionException.class, future::get);

The fix for the other bug used default methods. Is that an option for JUnit 4, or does it need to be source-compatible with Java < 8?

[kcooney]

@cushon yes that's the issue I was worried about. I saw something similar with a different API in my code base.

Believe it or not, JUnit 4 compiles with JDK 5, so we cannot add default methods to JUnit 4 classes or interfaces.

Also, I know of at least one large code base that patched assertThrows() into their copy of JUnit 4.12 (can't blame them; JUnit 4.13 has been in the pipeline a long time). So I would consider this a breaking change as-is.

[cushon]

@kcooney if we're talking about the same codebase, I shared some research in the other bug that showed the issue affected about 1% of files. I can migrate the affected assertions from future::get to () -> future.get() to work around this change.

Personally I think the benefit of the improved failed messages outweighs the cost of occasionally not being able to use method references.

And since this didn't make it into a non-snapshot release it doesn't really seem like a regression. If someone cherry-picked assertThrows into a fork of JUnit they could also choose not to cherry-pick this improvement into their fork.

[kcooney]

@cushon we are almost certainly talking about the same code base 😊

There could be other code bases with the same problem. Some users think we have abandoned JUnit 4.13, and using assertThrows() is promoted on StackOverflow.

That being said, if you use a prerelease version of JUnit or patch changes, you should expect that your next upgrade might be a pain.

I am not sure if the added benefit is worth the cost of not allowing method references. The compiler error doesn't make it obvious that you can resolve the problem by replacing the method reference with a lambda. Also, when I see a failure in assertThrows() I think "why didn't these inputs trigger the exception?" not "I wonder what did get returned?"

I could be convinced the benefits outweigh the costs. Perhaps good documentation can resolve all my concerns.

(For JUnit 5, adding the default method sounds reasonable, as long as we document why we did it)

[cushon]

My motivation for suggesting the original change was that I've seen some recommendations to use other expected exception testing idioms (e.g. try/fail/catch) in situations where it's helpful to include the result that was returning in the failure message. I think assertThrows is much easier to use correctly (I've seen a lot of bugs where people got try/fail/catch wrong in subtle ways), so I wanted to eliminate barriers to using assertThrows.

I agree that the fix for the ambiguity with method references might be unobvious. Having good documentation as you suggest would probably help.

I'd be interested in knowing how often this comes up in practice for other projects using JUnit. The ambiguity doesn't affect all method references, just references to some generic methods. Nearly every instance of the problem I saw was specifically using future::get, and I wouldn't be surprised assertThrows(..., future::get) was unusually common in the codebase I looked at.

[kcooney]

What if the object returned by the lambda throws an exception when you call toString() on it?

[marcphilipp]

We don't handle that case anywhere, do we? But we should probably call formatClassAndValue(). I can make this change but I think we should agree whether to proceed with this PR first.

@kcooney WDYT?

[sbrannen]

Note that toString() for arrays ends up being ugly, in case the return value is an array.

I addressed that in JUnit Jupiter like this: junit-team/junit5@c5c4741

[sbrannen]

We don't handle that case anywhere, do we?

No, I don't think there is any exception handling for invocations of toString() on objects supplied to assertion methods in either JUnit 4 or JUnit Jupiter.

[marcphilipp]

@junit-team/junit-committers Any additional thoughts on this one?

[marcphilipp]

I'm closing the PR as it's no longer relevant because we've decided to roll back the overloaded variants in Jupiter (see junit-team/junit5#1576).