From 381928a59795f19ae1dc05b08d2053bbe7ad3671 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 7 Sep 2018 18:19:31 +0200 Subject: [PATCH] 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). Issue: #1576 --- .../release-notes/release-notes-5.3.1.adoc | 13 ++- .../org/junit/jupiter/api/AssertThrows.java | 83 ++----------------- .../org/junit/jupiter/api/Assertions.java | 69 --------------- .../api/function/ThrowingSupplier.java | 27 +----- .../AssertDoesNotThrowAssertionsTests.java | 14 ++-- .../api/AssertThrowsAssertionsTests.java | 82 +----------------- 6 files changed, 31 insertions(+), 257 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.3.1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.3.1.adoc index 09ae73b36bb2..676427a6dc74 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.3.1.adoc +++ b/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]] diff --git a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java index 3f30547fde4d..99da94bb5fbb 100644 --- a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java +++ b/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 assertThrows(Class expectedType, Executable executable) { - return assertThrows(expectedType, asSupplier(executable), (Object) null); + return assertThrows(expectedType, executable, (Object) null); } static T assertThrows(Class expectedType, Executable executable, String message) { - return assertThrows(expectedType, asSupplier(executable), (Object) message); + return assertThrows(expectedType, executable, (Object) message); } static T assertThrows(Class expectedType, Executable executable, Supplier messageSupplier) { - return assertThrows(expectedType, asSupplier(executable), (Object) messageSupplier); - } - - /** - * @since 5.3 - */ - static T assertThrows(Class expectedType, ThrowingSupplier supplier) { - return assertThrows(expectedType, supplier::get, (Object) null); - } - - /** - * @since 5.3 - */ - static T assertThrows(Class expectedType, ThrowingSupplier supplier, String message) { - return assertThrows(expectedType, supplier::get, (Object) message); - } - - /** - * @since 5.3 - */ - static T assertThrows(Class expectedType, ThrowingSupplier supplier, - Supplier messageSupplier) { - - return assertThrows(expectedType, supplier::get, (Object) messageSupplier); + return assertThrows(expectedType, executable, (Object) messageSupplier); } @SuppressWarnings("unchecked") - private static T assertThrows(Class expectedType, ResultAwareThrowingSupplier supplier, + private static T assertThrows(Class 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 assertThrows(Class 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 extends ThrowingSupplier { - - /** - * 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 asSupplier(Executable executable) { - return new ResultAwareThrowingSupplierAdapter(executable); - } - - /** - * Adapts an {@link Executable} to the {@link ResultAwareThrowingSupplier} API. - */ - private static class ResultAwareThrowingSupplierAdapter implements ResultAwareThrowingSupplier { - - 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; - } - } - } diff --git a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/Assertions.java b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/Assertions.java index 2881dc8991f6..93c028155567 100644 --- a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/Assertions.java +++ b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/Assertions.java @@ -1211,75 +1211,6 @@ public static T assertThrows(Class expectedType, Execut return AssertThrows.assertThrows(expectedType, executable, messageSupplier); } - // --- supplier --- - - /** - * Asserts that execution of the given {@code supplier} throws - * an exception of the {@code expectedType} and returns the exception. - * - *

If no exception is thrown, or if an exception of a different type is - * thrown, this method will fail. - * - *

If the given {@link ThrowingSupplier} returns a result instead of - * throwing an exception, the result will be included in the failure message. - * - *

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 assertThrows(Class expectedType, ThrowingSupplier supplier) { - return AssertThrows.assertThrows(expectedType, supplier); - } - - /** - * Asserts that execution of the given {@code supplier} throws - * an exception of the {@code expectedType} and returns the exception. - * - *

If no exception is thrown, or if an exception of a different type is - * thrown, this method will fail. - * - *

If the given {@link ThrowingSupplier} returns a result instead of - * throwing an exception, the result will be included in the failure message. - * - *

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 assertThrows(Class expectedType, ThrowingSupplier supplier, - String message) { - - return AssertThrows.assertThrows(expectedType, supplier, message); - } - - /** - * Asserts that execution of the given {@code supplier} throws - * an exception of the {@code expectedType} and returns the exception. - * - *

If no exception is thrown, or if an exception of a different type is - * thrown, this method will fail. - * - *

If necessary, the failure message will be retrieved lazily from the - * supplied {@code messageSupplier}. - * - *

If the given {@link ThrowingSupplier} returns a result instead of - * throwing an exception, the result will be included in the failure message. - * - *

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 assertThrows(Class expectedType, ThrowingSupplier supplier, - Supplier messageSupplier) { - - return AssertThrows.assertThrows(expectedType, supplier, messageSupplier); - } - // --- executable --- /** diff --git a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/ThrowingSupplier.java b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/ThrowingSupplier.java index 6298145581e1..57cf934729a1 100644 --- a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/ThrowingSupplier.java +++ b/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. * - *

As of JUnit Jupiter 5.3, {@code ThrowingSupplier} extends - * {@link Executable}, providing a default 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}. - * *

Rationale for throwing {@code Throwable} instead of {@code Exception}

* *

Although Java applications typically throw exceptions that are instances @@ -51,23 +42,7 @@ */ @FunctionalInterface @API(status = STABLE, since = "5.0") -public interface ThrowingSupplier extends Executable { - - /** - * Delegates to {@link #get()} and ignores the return value. - * - *

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 { /** * Get a result, potentially throwing an exception. diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertDoesNotThrowAssertionsTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertDoesNotThrowAssertionsTests.java index 941f8947a06f..303565092f69 100644 --- a/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertDoesNotThrowAssertionsTests.java +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertDoesNotThrowAssertionsTests.java @@ -35,7 +35,7 @@ class AssertDoesNotThrowAssertionsTests { private static final ThrowingSupplier something = () -> "enigma"; @Test - void assertDoesNotThrowWithFutureMethodReference() { + void assertDoesNotThrowWithMethodReferenceForNonVoidReturnType() { FutureTask future = new FutureTask<>(() -> { return "foo"; }); @@ -43,9 +43,11 @@ void assertDoesNotThrowWithFutureMethodReference() { 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); diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertThrowsAssertionsTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertThrowsAssertionsTests.java index 6cd83c0ab9da..ad210d1d26e0 100644 --- a/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertThrowsAssertionsTests.java +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertThrowsAssertionsTests.java @@ -25,7 +25,6 @@ import java.util.concurrent.FutureTask; import org.junit.jupiter.api.function.Executable; -import org.junit.jupiter.api.function.ThrowingSupplier; import org.opentest4j.AssertionFailedError; /** @@ -39,25 +38,13 @@ class AssertThrowsAssertionsTests { }; @Test - void assertThrowsWithFutureMethodReference() { + void assertThrowsWithMethodReferenceForNonVoidReturnType() { FutureTask future = new FutureTask<>(() -> { throw new RuntimeException("boom"); }); future.run(); - ExecutionException exception; - - // Current compiler's type inference - // For rationale, see https://github.com/junit-team/junit5/issues/1414 - exception = assertThrows(ExecutionException.class, future::get); - assertEquals("boom", exception.getCause().getMessage()); - - // Explicitly as an Executable - exception = assertThrows(ExecutionException.class, (Executable) future::get); - assertEquals("boom", exception.getCause().getMessage()); - - // Explicitly as a ThrowingSupplier - exception = assertThrows(ExecutionException.class, (ThrowingSupplier) future::get); + ExecutionException exception = assertThrows(ExecutionException.class, future::get); assertEquals("boom", exception.getCause().getMessage()); } @@ -66,26 +53,14 @@ void assertThrowsWithMethodReferenceForVoidReturnType() { var object = new Object(); IllegalMonitorStateException exception; - // 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 java.lang.Object.wait(...) - // - // exception = assertThrows(IllegalMonitorStateException.class, object::wait); - - // Current compiler's type inference exception = assertThrows(IllegalMonitorStateException.class, object::notify); assertNotNull(exception); - // Explicitly as an Executable - exception = assertThrows(IllegalMonitorStateException.class, (Executable) object::notify); - assertNotNull(exception); - - exception = assertThrows(IllegalMonitorStateException.class, (Executable) object::wait); + // Note that Object.wait(...) is an overloaded method with a void return type + exception = assertThrows(IllegalMonitorStateException.class, object::wait); assertNotNull(exception); } - // --- executable ---------------------------------------------------------- - @Test void assertThrowsWithExecutableThatThrowsThrowable() { EnigmaThrowable enigmaThrowable = assertThrows(EnigmaThrowable.class, (Executable) () -> { @@ -140,7 +115,6 @@ void assertThrowsWithExecutableThatDoesNotThrowAnException() { expectAssertionFailedError(); } catch (AssertionFailedError ex) { - // the following also implicitly verifies that the failure message does not contain "(returned null)". assertMessageEquals(ex, "Expected java.lang.IllegalStateException to be thrown, but nothing was thrown."); } } @@ -288,54 +262,6 @@ void assertThrowsWithExecutableThatThrowsSameExceptionTypeFromDifferentClassLoad } } - // --- supplier ------------------------------------------------------------ - - @Test - void assertThrowsWithThrowingSupplierThatReturns() { - try { - assertThrows(EnigmaThrowable.class, (ThrowingSupplier) () -> 42); - expectAssertionFailedError(); - } - catch (AssertionFailedError ex) { - assertMessageContains(ex, "(returned 42)"); - } - } - - @Test - void assertThrowsWithThrowingSupplierThatReturnsNull() { - try { - assertThrows(EnigmaThrowable.class, (ThrowingSupplier) () -> null); - expectAssertionFailedError(); - } - catch (AssertionFailedError ex) { - assertMessageContains(ex, "(returned null)"); - } - } - - @Test - void assertThrowsWithThrowingSupplierThatReturnsAndWithCustomMessage() { - try { - assertThrows(EnigmaThrowable.class, (ThrowingSupplier) () -> 42, "custom message"); - expectAssertionFailedError(); - } - catch (AssertionFailedError ex) { - assertMessageContains(ex, "(returned 42)"); - assertMessageContains(ex, "custom message"); - } - } - - @Test - void assertThrowsWithThrowingSupplierThatReturnsAndWithCustomMessageSupplier() { - try { - assertThrows(EnigmaThrowable.class, (ThrowingSupplier) () -> 42, () -> "custom message"); - expectAssertionFailedError(); - } - catch (AssertionFailedError ex) { - assertMessageContains(ex, "(returned 42)"); - assertMessageContains(ex, "custom message"); - } - } - // ------------------------------------------------------------------------- @SuppressWarnings("serial")