Remove variants of assertThrows accepting ThrowingSupplier

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. Consequently, this was a breaking change.

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

This commit fixes the breaking change by removing all variants of `assertThrows` that accept a `ThrowingSupplier`. In addition, this commit reverts the related changes to `ThrowingSupplier` (i.e., it no longer implements `Executable` via a `default` interface method).

Since the breaking change brought it to our attention that issues can arise when overloading methods that differ only by the functional interface they accept, we noticed that some restrictions apply to the use of method references for the existing overloaded `assertDoesNotThrow()` variants. This commit therefore also adds notes to the JavaDoc for the affected methods.

Issue: #1576

documentation/src/docs/asciidoc/release-notes/release-notes-5.3.1.adoc

@@ -1,7 +1,7 @@
 [[release-notes-5.3.1]]
 == 5.3.1
 
-*Date of Release:* ❓
+*Date of Release:* September ❓, 2018
 
 *Scope:* Bug fixes since 5.3.0
 
@@ -30,7 +30,16 @@ repository on GitHub.
 
 ==== Bug Fixes
 
-* ❓
+* Invocations of `assertThrows()` that are passed a method reference for an overloaded
+  method with a `void` return type once again compile.
+  - For example, given an instance of `java.lang.Object` stored in a variable named
+    `object`, `assertThrows(Exception.class, object::wait)` compiled against JUnit 5.2.0,
+    failed to compile against JUnit 5.3.0, but now compiles against JUnit 5.3.1.
+
+==== Breaking Changes
+
+* In order to revert the aforementioned breaking change, variants of `assertThrows()`
+  introduced in JUnit 5.3.0 that accept `ThrowingSupplier` arguments have been removed.
 
 
 [[release-notes-5.3.1-junit-vintage]]

junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java

@@ -18,8 +18,6 @@
 import java.util.function.Supplier;
 
 import org.junit.jupiter.api.function.Executable;
-import org.junit.jupiter.api.function.ThrowingSupplier;
-import org.junit.platform.commons.util.StringUtils;
 import org.opentest4j.AssertionFailedError;
 
 /**
@@ -35,49 +33,25 @@ private AssertThrows() {
 	}
 
 	static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable) {
-		return assertThrows(expectedType, asSupplier(executable), (Object) null);
+		return assertThrows(expectedType, executable, (Object) null);
 	}
 
 	static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable, String message) {
-		return assertThrows(expectedType, asSupplier(executable), (Object) message);
+		return assertThrows(expectedType, executable, (Object) message);
 	}
 
 	static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable,
 			Supplier<String> messageSupplier) {
 
-		return assertThrows(expectedType, asSupplier(executable), (Object) messageSupplier);
-	}
-
-	/**
-	 * @since 5.3
-	 */
-	static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier) {
-		return assertThrows(expectedType, supplier::get, (Object) null);
-	}
-
-	/**
-	 * @since 5.3
-	 */
-	static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier, String message) {
-		return assertThrows(expectedType, supplier::get, (Object) message);
-	}
-
-	/**
-	 * @since 5.3
-	 */
-	static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier,
-			Supplier<String> messageSupplier) {
-
-		return assertThrows(expectedType, supplier::get, (Object) messageSupplier);
+		return assertThrows(expectedType, executable, (Object) messageSupplier);
 	}
 
 	@SuppressWarnings("unchecked")
-	private static <T extends Throwable> T assertThrows(Class<T> expectedType, ResultAwareThrowingSupplier<?> supplier,
+	private static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable,
 			Object messageOrSupplier) {
 
-		Object result;
 		try {
-			result = supplier.get();
+			executable.execute();
 		}
 		catch (Throwable actualException) {
 			if (expectedType.isInstance(actualException)) {
@@ -90,54 +64,9 @@ private static <T extends Throwable> T assertThrows(Class<T> expectedType, Resul
 			}
 		}
 
-		String includedResult = supplier.includeResult()
-				? String.format(" (returned %s).", StringUtils.nullSafeToString(result))
-				: ".";
 		String message = buildPrefix(nullSafeGet(messageOrSupplier))
-				+ String.format("Expected %s to be thrown, but nothing was thrown", getCanonicalName(expectedType))
-				+ includedResult;
+				+ String.format("Expected %s to be thrown, but nothing was thrown.", getCanonicalName(expectedType));
 		throw new AssertionFailedError(message);
 	}
 
-	private interface ResultAwareThrowingSupplier<T> extends ThrowingSupplier<T> {
-
-		/**
-		 * Determine if the result of invoking {@link #get()} should be included
-		 * in the assertion failure message if this supplier returns an actual
-		 * result instead of throwing an exception.
-		 *
-		 * @return {@code true} by default; can be overridden in concrete implementations
-		 */
-		default boolean includeResult() {
-			return true;
-		}
-	}
-
-	private static ResultAwareThrowingSupplier<Void> asSupplier(Executable executable) {
-		return new ResultAwareThrowingSupplierAdapter(executable);
-	}
-
-	/**
-	 * Adapts an {@link Executable} to the {@link ResultAwareThrowingSupplier} API.
-	 */
-	private static class ResultAwareThrowingSupplierAdapter implements ResultAwareThrowingSupplier<Void> {
-
-		private final Executable executable;
-
-		ResultAwareThrowingSupplierAdapter(Executable executable) {
-			this.executable = executable;
-		}
-
-		@Override
-		public Void get() throws Throwable {
-			executable.execute();
-			return null;
-		}
-
-		@Override
-		public boolean includeResult() {
-			return false;
-		}
-	}
-
 }

junit-jupiter-api/src/main/java/org/junit/jupiter/api/Assertions.java

@@ -1211,75 +1211,6 @@ public static <T extends Throwable> T assertThrows(Class<T> expectedType, Execut
 		return AssertThrows.assertThrows(expectedType, executable, messageSupplier);
 	}
 
-	// --- supplier ---
-
-	/**
-	 * <em>Asserts</em> that execution of the given {@code supplier} throws
-	 * an exception of the {@code expectedType} and returns the exception.
-	 *
-	 * <p>If no exception is thrown, or if an exception of a different type is
-	 * thrown, this method will fail.
-	 *
-	 * <p>If the given {@link ThrowingSupplier} returns a result instead of
-	 * throwing an exception, the result will be included in the failure message.
-	 *
-	 * <p>If you do not want to perform additional checks on the exception instance,
-	 * simply ignore the return value.
-	 *
-	 * @since 5.3
-	 */
-	@API(status = STABLE, since = "5.3")
-	public static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier) {
-		return AssertThrows.assertThrows(expectedType, supplier);
-	}
-
-	/**
-	 * <em>Asserts</em> that execution of the given {@code supplier} throws
-	 * an exception of the {@code expectedType} and returns the exception.
-	 *
-	 * <p>If no exception is thrown, or if an exception of a different type is
-	 * thrown, this method will fail.
-	 *
-	 * <p>If the given {@link ThrowingSupplier} returns a result instead of
-	 * throwing an exception, the result will be included in the failure message.
-	 *
-	 * <p>If you do not want to perform additional checks on the exception instance,
-	 * simply ignore the return value.
-	 *
-	 * @since 5.3
-	 */
-	@API(status = STABLE, since = "5.3")
-	public static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier,
-			String message) {
-
-		return AssertThrows.assertThrows(expectedType, supplier, message);
-	}
-
-	/**
-	 * <em>Asserts</em> that execution of the given {@code supplier} throws
-	 * an exception of the {@code expectedType} and returns the exception.
-	 *
-	 * <p>If no exception is thrown, or if an exception of a different type is
-	 * thrown, this method will fail.
-	 *
-	 * <p>If necessary, the failure message will be retrieved lazily from the
-	 * supplied {@code messageSupplier}.
-	 *
-	 * <p>If the given {@link ThrowingSupplier} returns a result instead of
-	 * throwing an exception, the result will be included in the failure message.
-	 *
-	 * <p>If you do not want to perform additional checks on the exception instance,
-	 * simply ignore the return value.
-	 *
-	 * @since 5.3
-	 */
-	@API(status = STABLE, since = "5.3")
-	public static <T extends Throwable> T assertThrows(Class<T> expectedType, ThrowingSupplier<?> supplier,
-			Supplier<String> messageSupplier) {
-
-		return AssertThrows.assertThrows(expectedType, supplier, messageSupplier);
-	}
-
 	// --- executable ---
 
 	/**

junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/ThrowingSupplier.java

@@ -23,15 +23,6 @@
  * {@link java.util.function.Supplier}, except that a {@code ThrowingSupplier}
  * can throw any kind of exception, including checked exceptions.
  *
- * <p>As of JUnit Jupiter 5.3, {@code ThrowingSupplier} extends
- * {@link Executable}, providing a <em>default</em> implementation of
- * {@link #execute()} that delegates to {@link #get()} and ignores the return
- * value. This allows the Java compiler to disambiguate between
- * {@code ThrowingSupplier} and {@code Executable} when performing type
- * inference for a lambda expression or method reference supplied to
- * an overloaded method that accepts either a {@code ThrowingSupplier} or an
- * {@code Executable}.
- *
  * <h4>Rationale for throwing {@code Throwable} instead of {@code Exception}</h4>
  *
  * <p>Although Java applications typically throw exceptions that are instances
@@ -51,23 +42,7 @@
  */
 @FunctionalInterface
 @API(status = STABLE, since = "5.0")
-public interface ThrowingSupplier<T> extends Executable {
-
-	/**
-	 * Delegates to {@link #get()} and ignores the return value.
-	 *
-	 * <p>This default method is not intended to be overridden. See
-	 * {@linkplain ThrowingSupplier class-level documentation} for further
-	 * details.
-	 *
-	 * @since 5.3
-	 * @see #get()
-	 */
-	@Override
-	@API(status = STABLE, since = "5.3")
-	default void execute() throws Throwable {
-		get();
-	}
+public interface ThrowingSupplier<T> {
 
 	/**
 	 * Get a result, potentially throwing an exception.

junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertDoesNotThrowAssertionsTests.java

@@ -35,17 +35,19 @@ class AssertDoesNotThrowAssertionsTests {
 	private static final ThrowingSupplier<String> something = () -> "enigma";
 
 	@Test
-	void assertDoesNotThrowWithFutureMethodReference() {
+	void assertDoesNotThrowWithMethodReferenceForNonVoidReturnType() {
 		FutureTask<String> future = new FutureTask<>(() -> {
 			return "foo";
 		});
 		future.run();
 
 		String result;
 
-		// Current compiler's type inference
-		result = assertDoesNotThrow(future::get);
-		assertEquals("foo", result);
+		// Current compiler's type inference: does NOT compile since the compiler
+		// cannot figure out which overloaded variant of assertDoesNotThrow() to
+		// invoke (i.e., Executable vs. ThrowingSupplier).
+		//
+		// result = assertDoesNotThrow(future::get);
 
 		// Explicitly as an Executable
 		assertDoesNotThrow((Executable) future::get);
@@ -61,7 +63,9 @@ void assertDoesNotThrowWithMethodReferenceForVoidReturnType() {
 
 		// Note: the following does not compile since the compiler cannot properly
 		// perform type inference for a method reference for an overloaded method
-		// that has a void return type such as Foo.overloaded(...)
+		// that has a void return type such as Foo.overloaded(...), IFF the
+		// compiler is simultaneously trying to pick which overloaded variant
+		// of assertDoesNotThrow() to invoke.
 		//
 		// assertDoesNotThrow(foo::overloaded);