Junit5: Regression: assertThrows() ThrowingSupplier variants break existing code

Created on 5 Sep 2018 · 18Comments · Source: junit-team/junit5

Overview

JUnit Jupiter 5.3.0 introduced new variants of assertThrows() that accept ThrowingSupplier arguments instead of Executable (see #1394). However, this change prevents existing code from compiling against Jupiter 5.3.0 if the code in question used a method reference for an overloaded method with a void return type.

Note that overloaded methods with non-void return types are not affected. For example, even though java.util.concurrent.Future has two get(...) methods, it still can be used as a method reference as future::get without suffering from issues with type inference since such a method can always properly be inferred to be a ThrowingSupplier.

Example

Note that java.lang.Object has three overloaded variants of its wait() method. Thus, the following example compiled against Jupiter 5.2.0 but fails to compile against Jupiter 5.3.0.

@Test
void test() {
    var object = new Object();
    // Does NOT compile against Jupiter 5.3.0
    assertThrows(Exception.class, object::wait);
}

Workarounds

The following two workarounds allow affected code to compile against Jupiter 5.3.0; however, this obviously requires a manual change to existing code which is an unacceptable breaking change.

@Test
void test() {
    var object = new Object();

    // Does NOT compile against Jupiter 5.3.0
    // assertThrows(Exception.class, object::wait);

    // Workaround #1
    assertThrows(Exception.class, () -> object.wait());

    // Workaround #2
    assertThrows(Exception.class, (Executable) object::wait);
}

Analysis

Given the following test class, the code compiles "as is", but the invocation of assertThrows(Exception.class, object::wait) will fail to compile if you uncomment the assertThrows() variant that accepts a ThrowingSupplier.

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.function.Executable;

class AssertThrowsTests {

    static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable) {
        return Assertions.assertThrows(expectedType, executable);
    }

    //  static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier) {
    //      return Assertions.assertThrows(expectedType, supplier);
    //  }

    @Test
    void test() {
        var object = new Object();
        assertThrows(Exception.class, () -> object.wait());
        assertThrows(Exception.class, (Executable) object::wait);
        assertThrows(Exception.class, object::wait);
    }

}

Proposal

In other to avoid issues with type inference for overloaded methods used as method references, we should revert the changes to all existing assertThrows() methods and introduce new assertThrowsXyz() variants that accept a ThrowingSupplier.

This will enforce that all existing code that invokes assertThrows() will use an Executable, and all new code compiled against the new assertThrowsXyz() variants will use a ThrowingSupplier.

What Xyz should be is up for debate.

Related Issues

#1394
#1401
#1414

Deliverables

[x] Remove overloaded variants of assertThrows that accept a ThrowingSupplier
[x] Revert changes to ThrowingSupplier
[x] Document bug fix in the Release Notes
[x] Investigate potential issue with overloaded variants of assertDoesNotThrow (and document workarounds if necessary)
[x] Backport fix and release notes to a new 5.3.1 branch.

Jupiter bug

Source

sbrannen

All 18 comments

Slated for 5.3.1

sbrannen on 5 Sep 2018

@junit-team/junit-lambda, I've added the "team discussion" label in order for us to address this as soon as possible.

sbrannen on 5 Sep 2018

/cc @cushon @kevinb9n @cpovirk

sormuras on 5 Sep 2018

Sorry :( And thanks for all the details about the specific problem case.

As @kevinb9n said during the initial overload problems:

Sadly I think most of the value of the idea would be gone if it only benefits the users who know to opt into it. And even for the very-clued-in, it would be a mental burden to constantly have to decide which method to call.

So if the breakage is too big a problem I think we should back the whole thing out. :-(

That would be my inclination, as well: If users have to switch to assertThrowsXyz to get the benefits, it might not be worth having at all. But it's a judgment call.

cpovirk on 5 Sep 2018

We should also consider to keep the assertThrows variants "as-is". Because the "impact or blast radius" of the breakage is a) _very_ limited and b) there is an easy solution: cast to Executable or create an inline lambda () -> object.wait().

Only method references to overloaded methods that return void are affected, like the object::wait variants. Using future::get is safe, as they have non-void return types. Our complete JUnit tests didn't break when the new assertThrows variants were introduced. Yes, half of those affected tests changed the actual method they called, but they compiled and maintained the expected behaviour.

Did you encounter that problem in your test suites?

Am I the only one who had to apply a workaround in his test code? (I assume, I am...)

sormuras on 5 Sep 2018

👍1

Did you encounter that problem in your test suites?

I'll find out how often this comes up in our code and report back. (I was waiting for https://github.com/junit-team/junit4/pull/1537 to be resolved before using the latest version of the patch.)

cushon on 5 Sep 2018

I'll find out how often this comes up in our code and report back.

Thanks, @cushon

sbrannen on 5 Sep 2018

We should also consider to keep the assertThrows variants "as-is".

That's of course also an option, one of three I suppose.

I'll update the _Proposals_ section of this issue.

sbrannen on 5 Sep 2018

If users have to switch to assertThrowsXyz to get the benefits, it might not be worth having at all. But it's a judgment call.

That's also worth considering.

sbrannen on 5 Sep 2018

OK... We now have three proposals to choose from.

sbrannen on 5 Sep 2018

Food for thought:

Another downside to keeping it "as is" is that user code would also break (in the future) if it uses a method reference for a void-returning method _now_ that _later_ gets overloaded.

And... that would be totally out of JUnit's control.

sbrannen on 5 Sep 2018

👍1

I'm not sure if you're aware: The _Eclipse IDE_ project seems to make upgrading its JUnit support (from currently 5.1) to 5.3(.1) dependent on this issue.

MarkusMit on 6 Sep 2018

No, we were not aware of that potential holdup in Eclipse IDE.

So, thank you for bringing it to our attention!

sbrannen on 6 Sep 2018

👍1

Team Decision:

Remove overloaded variants of assertThrows that accept a ThrowingSupplier
Revert changes to ThrowingSupplier
Investigate potential issue with overloaded variants of assertDoesNotThrow (and document workarounds if necessary)