Regression: assertThrows() ThrowingSupplier variants break existing code

[sbrannen]

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

• Improve assertThrows failure message if the lambda returns a result instead of throwing #1394
• Add assertThrows overloads that take a ThrowingSupplier #1401
• Change to assertThrows in #1394 makes some existing call sites ambiguous #1414

Deliverables

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

[sbrannen]

Slated for 5.3.1

[sbrannen]

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

[sormuras]

/cc @cushon @kevinb9n @cpovirk

[cpovirk]

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.

[sormuras]

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...)

[cushon]

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 junit-team/junit4#1537 to be resolved before using the latest version of the patch.)

[sbrannen]

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

Thanks, @cushon

[sbrannen]

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]

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]

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

[sbrannen]

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.

[MarkusMit]

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.

[sbrannen]

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

So, thank you for bringing it to our attention!

[marcphilipp]

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)

[sbrannen]

FYI: I updated the Deliverables according to the team decision.

[sbrannen]

in progress

[sbrannen]

This has been fixed in 2e496ca.

Note, however, that this work has not yet been backported to the soon-to-be-created 5.3.1 branch.

[sbrannen]

Backported to the 5.3.x release branch: https://github.com/junit-team/junit5/commits/releases/5.3.x

[pavelrappo]

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.

I could not make it work with Future.get either; I believe non-void overloads can also be ambiguous in this case.