From 6caccb4a776e90954833898e4fdd0cd4df0fa0c6 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 1 Jan 2023 10:46:56 +0100 Subject: [PATCH] Suggestions --- .../refasterrules/JUnitToAssertJRules.java | 74 +++++++++++++------ .../JUnitToAssertJRulesTestInput.java | 61 +++++++-------- .../JUnitToAssertJRulesTestOutput.java | 61 +++++++-------- 3 files changed, 108 insertions(+), 88 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java index 038572075c..7d89b6ec29 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java @@ -16,6 +16,7 @@ import static org.junit.jupiter.api.Assertions.assertThrowsExactly; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.DoNotCall; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; @@ -27,11 +28,38 @@ import org.junit.jupiter.api.function.ThrowingSupplier; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; -/** Refaster rules to replace JUnit assertions with AssertJ equivalents. */ +/** + * Refaster rules to replace JUnit assertions with AssertJ equivalents. + * + *

Note that, while both libraries throw an {@link AssertionError} in case of an assertion + * failure, the exact subtype used generally differs. + */ +// XXX: Not all `org.assertj.core.api.Assertions` methods have an associated Refaster rule yet; +// expand this class. +// XXX: Introduce a `@Matcher` on `Executable` and `ThrowingSupplier` expressions, such that they +// are only matched if they are also compatible with the `ThrowingCallable` functional interface. +// When implementing such a matcher, note that expressions with a non-void return type such as +// `() -> toString()` match both `ThrowingSupplier` and `ThrowingCallable`, but `() -> "constant"` +// is only compatible with the former. @OnlineDocumentation final class JUnitToAssertJRules { private JUnitToAssertJRules() {} + public ImmutableSet elidedTypesAndStaticImports() { + return ImmutableSet.of( + Assertions.class, + assertDoesNotThrow(() -> null), + assertInstanceOf(null, null), + assertThrows(null, null), + assertThrowsExactly(null, null), + (Runnable) () -> assertFalse(true), + (Runnable) () -> assertNotNull(null), + (Runnable) () -> assertNotSame(null, null), + (Runnable) () -> assertNull(null), + (Runnable) () -> assertSame(null, null), + (Runnable) () -> assertTrue(true)); + } + static final class ThrowNewAssertionError { @BeforeTemplate void before() { @@ -45,29 +73,42 @@ void after() { } } - static final class FailWithMessage { + static final class FailWithMessage { @BeforeTemplate - void before(String message) { - Assertions.fail(message); + T before(String message) { + return Assertions.fail(message); } @AfterTemplate @UseImportPolicy(STATIC_IMPORT_ALWAYS) - void after(String message) { - fail(message); + T after(String message) { + return fail(message); } } - static final class FailWithMessageAndThrowable { + static final class FailWithMessageAndThrowable { @BeforeTemplate - void before(String message, Throwable throwable) { - Assertions.fail(message, throwable); + T before(String message, Throwable throwable) { + return Assertions.fail(message, throwable); } @AfterTemplate @UseImportPolicy(STATIC_IMPORT_ALWAYS) - void after(String message, Throwable throwable) { - fail(message, throwable); + T after(String message, Throwable throwable) { + return fail(message, throwable); + } + } + + static final class FailWithThrowable { + @BeforeTemplate + void before(Throwable throwable) { + Assertions.fail(throwable); + } + + @AfterTemplate + @DoNotCall + void after(Throwable throwable) { + throw new AssertionError(throwable); } } @@ -227,12 +268,6 @@ void after(Object actual, Supplier supplier) { } } - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertEquals`. - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertArrayEquals`. - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertIterableEquals`. - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertLinesMatch`. - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertNotEquals`. - static final class AssertThatIsSameAs { @BeforeTemplate void before(Object actual, Object expected) { @@ -311,8 +346,6 @@ void after(Object actual, Object expected, Supplier supplier) { } } - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertAll`. - static final class AssertThatThrownByIsExactlyInstanceOf { @BeforeTemplate void before(Executable throwingCallable, Class clazz) { @@ -447,9 +480,6 @@ void after(ThrowingCallable throwingCallable, Supplier supplier) { } } - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertTimeout`. - // XXX: Rewrite `org.junit.jupiter.api.Assertions.assertTimeoutPreemptively`. - static final class AssertThatIsInstanceOf { @BeforeTemplate void before(Object actual, Class clazz) { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java index 9fdcd9433b..d1a68ad7a9 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestInput.java @@ -19,29 +19,33 @@ final class JUnitToAssertJRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of( - (Runnable) () -> assertDoesNotThrow(() -> null), - () -> assertFalse(true), - () -> assertInstanceOf(null, null), - () -> assertNotNull(null), - () -> assertNotSame(null, null), - () -> assertNull(null), - () -> assertSame(null, null), - () -> assertThrows(null, null), - () -> assertThrowsExactly(null, null), - () -> assertTrue(true), - () -> Assertions.fail()); + Assertions.class, + assertDoesNotThrow(() -> null), + assertInstanceOf(null, null), + assertThrows(null, null), + assertThrowsExactly(null, null), + (Runnable) () -> assertFalse(true), + (Runnable) () -> assertNotNull(null), + (Runnable) () -> assertNotSame(null, null), + (Runnable) () -> assertNull(null), + (Runnable) () -> assertSame(null, null), + (Runnable) () -> assertTrue(true)); } void testThrowNewAssertionError() { Assertions.fail(); } - void testFailWithMessage() { - Assertions.fail("foo"); + Object testFailWithMessage() { + return Assertions.fail("foo"); } - void testFailWithMessageAndThrowable() { - Assertions.fail("foo", new IllegalStateException()); + Object testFailWithMessageAndThrowable() { + return Assertions.fail("foo", new IllegalStateException()); + } + + void testFailWithThrowable() { + Assertions.fail(new IllegalStateException()); } void testAssertThatIsTrue() { @@ -93,39 +97,27 @@ void testAssertThatWithFailMessageSupplierIsNotNull() { } void testAssertThatIsSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertSame(expected, actual); + assertSame("foo", "bar"); } void testAssertThatWithFailMessageStringIsSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertSame(expected, actual, "foo"); + assertSame("foo", "bar", "baz"); } void testAssertThatWithFailMessageSupplierIsSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertSame(expected, actual, () -> "foo"); + assertSame("foo", "bar", () -> "baz"); } void testAssertThatIsNotSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertNotSame(expected, actual); + assertNotSame("foo", "bar"); } void testAssertThatWithFailMessageStringIsNotSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertNotSame(expected, actual, "foo"); + assertNotSame("foo", "bar", "baz"); } void testAssertThatWithFailMessageSupplierIsNotSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertNotSame(expected, actual, () -> "foo"); + assertNotSame("foo", "bar", () -> "baz"); } void testAssertThatThrownByIsExactlyInstanceOf() { @@ -154,14 +146,17 @@ void testAssertThatThrownByWithFailMessageSupplierIsInstanceOf() { void testAssertThatCodeDoesNotThrowAnyException() { assertDoesNotThrow(() -> {}); + assertDoesNotThrow(() -> toString()); } void testAssertThatCodeWithFailMessageStringDoesNotThrowAnyException() { assertDoesNotThrow(() -> {}, "foo"); + assertDoesNotThrow(() -> toString(), "bar"); } void testAssertThatCodeWithFailMessageSupplierDoesNotThrowAnyException() { assertDoesNotThrow(() -> {}, () -> "foo"); + assertDoesNotThrow(() -> toString(), () -> "bar"); } void testAssertThatIsInstanceOf() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestOutput.java index b7d3aa159b..c41d0246ac 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/JUnitToAssertJRulesTestOutput.java @@ -23,29 +23,33 @@ final class JUnitToAssertJRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of( - (Runnable) () -> assertDoesNotThrow(() -> null), - () -> assertFalse(true), - () -> assertInstanceOf(null, null), - () -> assertNotNull(null), - () -> assertNotSame(null, null), - () -> assertNull(null), - () -> assertSame(null, null), - () -> assertThrows(null, null), - () -> assertThrowsExactly(null, null), - () -> assertTrue(true), - () -> Assertions.fail()); + Assertions.class, + assertDoesNotThrow(() -> null), + assertInstanceOf(null, null), + assertThrows(null, null), + assertThrowsExactly(null, null), + (Runnable) () -> assertFalse(true), + (Runnable) () -> assertNotNull(null), + (Runnable) () -> assertNotSame(null, null), + (Runnable) () -> assertNull(null), + (Runnable) () -> assertSame(null, null), + (Runnable) () -> assertTrue(true)); } void testThrowNewAssertionError() { throw new AssertionError(); } - void testFailWithMessage() { - fail("foo"); + Object testFailWithMessage() { + return fail("foo"); } - void testFailWithMessageAndThrowable() { - fail("foo", new IllegalStateException()); + Object testFailWithMessageAndThrowable() { + return fail("foo", new IllegalStateException()); + } + + void testFailWithThrowable() { + throw new AssertionError(new IllegalStateException()); } void testAssertThatIsTrue() { @@ -97,39 +101,27 @@ void testAssertThatWithFailMessageSupplierIsNotNull() { } void testAssertThatIsSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertThat(actual).isSameAs(expected); + assertThat("bar").isSameAs("foo"); } void testAssertThatWithFailMessageStringIsSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertThat(actual).withFailMessage("foo").isSameAs(expected); + assertThat("bar").withFailMessage("baz").isSameAs("foo"); } void testAssertThatWithFailMessageSupplierIsSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertThat(actual).withFailMessage(() -> "foo").isSameAs(expected); + assertThat("bar").withFailMessage(() -> "baz").isSameAs("foo"); } void testAssertThatIsNotSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertThat(actual).isNotSameAs(expected); + assertThat("bar").isNotSameAs("foo"); } void testAssertThatWithFailMessageStringIsNotSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertThat(actual).withFailMessage("foo").isNotSameAs(expected); + assertThat("bar").withFailMessage("baz").isNotSameAs("foo"); } void testAssertThatWithFailMessageSupplierIsNotSameAs() { - Object actual = new Object(); - Object expected = new Object(); - assertThat(actual).withFailMessage(() -> "foo").isNotSameAs(expected); + assertThat("bar").withFailMessage(() -> "baz").isNotSameAs("foo"); } void testAssertThatThrownByIsExactlyInstanceOf() { @@ -164,14 +156,17 @@ void testAssertThatThrownByWithFailMessageSupplierIsInstanceOf() { void testAssertThatCodeDoesNotThrowAnyException() { assertThatCode(() -> {}).doesNotThrowAnyException(); + assertThatCode(() -> toString()).doesNotThrowAnyException(); } void testAssertThatCodeWithFailMessageStringDoesNotThrowAnyException() { assertThatCode(() -> {}).withFailMessage("foo").doesNotThrowAnyException(); + assertThatCode(() -> toString()).withFailMessage("bar").doesNotThrowAnyException(); } void testAssertThatCodeWithFailMessageSupplierDoesNotThrowAnyException() { assertThatCode(() -> {}).withFailMessage(() -> "foo").doesNotThrowAnyException(); + assertThatCode(() -> toString()).withFailMessage(() -> "bar").doesNotThrowAnyException(); } void testAssertThatIsInstanceOf() {