Junit5: Improve assertThrows failure message if the lambda returns a result instead of throwing

[cushon]

Overview

When testing code whose expected behavior is to throw an exception, it is sometimes useful to have a custom failure message for the case where the code does not throw an exception but instead returns a value:

try {
  double processedValue = process(bytes);
  fail("Expected an exception, but got " + processedValue);
} catch (IllegalArgumentException expected) {
}

One of the few disadvantages of using assertThrows instead of the try/catch/fail pattern above is that assertThrows does not provide an easy way to customize the failure message to include a result that was returned when an exception was not thrown.

One might be tempted to write something like:

assertThrows(
   IllegalArgumentException.class,
   () -> {
      double processedValue = process(bytes);
      fail("Expected an exception, but got " + processedValue);
   });

... but in general, it's a bad idea to have more than one statement in the body of the lambda passed to assertThrows, since in the case where the test passes those additional assertions will never be executed:

ImmutableList<Integer> xs = ImmutableList.of();
assertThrows(
    UnsupportedOperationException.class,
    () -> {
        xs.add(0);
        assertThat(xs).isEmpty(); // oops, never executed!
    });

What do you think about adding support to assertThrows for a custom failure message that includes the result of the expression that was expected to throw an exception?

Deliverables

This can be achieved without adding an additional parameter to the assertThrows API, e.g.:

public static <T extends Throwable> T assertThrows(
    Class<T> expectedType, ThrowingSupplier<?> supplier) { ... }

If the provided supplier returns a value, the implementation can include its string representation in the failure message.

Adding ThrowingSupplier overloads avoids adding another parameter to the API and allows existing code that is passing ThrowingSupplier-compatible lambdas to the existing assertThrows methods to get the improved failure messages for free.

For an example of what the implementation might look like, see:
https://github.com/junit-team/junit5/compare/master...cushon:assertThrows

[kevinb9n]

I think as a general principle we would like JUnit to never discard information, in the process of failing, that might be useful information to the engineer. So the motivation for this change is clear to me.

My understanding is that for code like

assertThrows(
    UnsupportedOperationException.class,
    () -> xs.add(0));

... if add is void, this code will continue to use the same method it always has, while if add returns any value, I believe this will magically start using Liam's new method, and magically get the more specific failure message. Correct?

If so... hard to see any downside here. :-)

[sbrannen]

Interesting idea.

We'll discuss it internally.

[sbrannen]

In any case, we cannot implement it as you have it currently in your PoC since a method may actually return null.

In other words, we would have to maintain the current behavior for Executable and have different behavior for ThrowingSupplier.

[sbrannen]

Tentatively slated for 5.3 M1 for _team discussion_

[sbrannen]

@kevinb9n,

... if add is void, this code will continue to use the same method it always has, while if add returns any value, I believe this will magically start using Liam's new method, and magically get the more specific failure message. Correct?

Yes, that is correct: Java performs correct type inference based on whether or not the lambda expression returns a value.

[marcphilipp]

Team Decision: We like it, please go ahead and submit your branch as a PR! 🙂