From a13c52cf08f02bf0eec9478f4794c7eb04b011b0 Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Thu, 8 Aug 2024 14:54:47 +0200 Subject: [PATCH 1/5] Solves https://github.com/openrewrite/rewrite-testing-frameworks/issues/547 --- .../junit5/RemoveTryCatchFailBlocks.java | 12 ++- .../junit5/RemoveTryCatchFailBlocksTest.java | 83 ++++++++++++------- 2 files changed, 61 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java b/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java index 7e17762f6..404ba64a7 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java @@ -45,7 +45,7 @@ public String getDisplayName() { @Override public String getDescription() { return "Replace `try-catch` blocks where `catch` merely contains a `fail()` for `fail(String)` statement " + - "with `Assertions.assertDoesNotThrow(() -> { ... })`."; + "with `Assertions.assertDoesNotThrow(() -> { ... })`."; } @Override @@ -67,6 +67,10 @@ public J visitTry(J.Try jtry, ExecutionContext ctx) { return try_; } + if (try_.getBody().getStatements().stream().anyMatch((statement -> statement instanceof J.Return))) { + return try_; + } + /* Only one statement in the catch block, which is a fail(), with no or a simple String argument. We would not want to convert for instance fail(cleanUpAndReturnMessage()) might still have side @@ -82,8 +86,8 @@ We would not want to convert for instance fail(cleanUpAndReturnMessage()) might } J.MethodInvocation failCall = (J.MethodInvocation) statement; if (!ASSERT_FAIL_NO_ARG.matches(failCall) - && !ASSERT_FAIL_STRING_ARG.matches(failCall) - && !ASSERT_FAIL_THROWABLE_ARG.matches(failCall)) { + && !ASSERT_FAIL_STRING_ARG.matches(failCall) + && !ASSERT_FAIL_THROWABLE_ARG.matches(failCall)) { return try_; } @@ -150,4 +154,4 @@ private J.MethodInvocation replaceWithAssertDoesNotThrowWithStringExpression(Exe .apply(getCursor(), try_.getCoordinates().replace(), try_.getBody(), failCallArgument); } } -} +} \ No newline at end of file diff --git a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java index 70afbcac4..1444e2ea3 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java @@ -45,7 +45,7 @@ void removeTryCatchBlock() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -60,7 +60,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -83,7 +83,7 @@ void removeStaticImportFail() { import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.fail; - + class MyTest { @Test public void testMethod() { @@ -98,7 +98,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -120,7 +120,7 @@ void removeTryCatchBlockWithoutMessage() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -135,7 +135,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -157,7 +157,7 @@ void removeTryCatchBlockWithFailString() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -172,7 +172,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -194,7 +194,7 @@ void failMethodArgIsNotGetMessage() { """ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Assertions; - + class MyTest { @Test void aTest() { @@ -204,7 +204,7 @@ void aTest() { Assertions.fail(cleanUpAndReturnMessage()); } } - + String cleanUpAndReturnMessage() { System.out.println("clean up code"); return "Oh no!"; @@ -223,7 +223,7 @@ void doesNotRunWithMultipleCatchBlocks() { """ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Assertions; - + class MyTest { @Test void aTest() { @@ -249,7 +249,7 @@ void catchHasMultipleStatements() { """ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Assertions; - + class MyTest { @Test void aTest() { @@ -275,7 +275,7 @@ void doesNotRunOnTryWithResources() { import org.junit.jupiter.api.Test; import java.io.PrintWriter; import org.junit.jupiter.api.Assertions; - + class MyTest { @Test void aTest() { @@ -299,7 +299,7 @@ void statementsBeforeAndAfterTryBlock() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -320,7 +320,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -348,7 +348,7 @@ void failWithStringThrowableArgs() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test void testMethod() { @@ -363,7 +363,7 @@ void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test void testMethod() { @@ -386,7 +386,7 @@ void failWithSupplierStringAsIdentifier() { import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import java.util.function.Supplier; - + class MyTest { @Test void testMethod() { @@ -412,7 +412,7 @@ void failWithSupplierStringAsLambda() { import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import java.util.function.Supplier; - + class MyTest { @Test void testMethod() { @@ -436,7 +436,7 @@ void failWithThrowable() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test void testMethod() { @@ -451,7 +451,7 @@ void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test void testMethod() { @@ -473,7 +473,7 @@ void multipleTryCatchBlocks() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -493,7 +493,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -518,7 +518,7 @@ void failHasBinaryWithException() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -533,7 +533,7 @@ public void testMethod() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -555,7 +555,7 @@ void failHasBinaryWithMethodCall() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -565,7 +565,7 @@ public void testMethod() { Assertions.fail("The error is: " + anotherMethod()); } } - + public String anotherMethod() { return "anotherMethod"; } @@ -602,7 +602,7 @@ public void testMethod() { import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import java.io.FileOutputStream; - + class MyTest { @Test public void testMethod() { @@ -625,7 +625,7 @@ void doesNotRunonTryFinally() { """ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; - + class MyTest { @Test public void testMethod() { @@ -642,4 +642,27 @@ public void testMethod() { ) ); } -} + + @Test + @Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/547") + void removeTryCatchFailBlocksWithReturningTry() { + rewriteRun( + spec -> spec.recipe(new RemoveTryCatchFailBlocks()), + java( + """ + package com.helloworld; + + import static org.junit.jupiter.api.Assertions.fail; + + public class Foo { + + String getFoo() { + try { + return "foo"; + } catch (RuntimeException e) { + fail(); + } + } + }""")); + } +} \ No newline at end of file From 540497704b092ecdd08a7bc098244be60a14f4e8 Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Thu, 8 Aug 2024 14:56:15 +0200 Subject: [PATCH 2/5] Apply org.openrewrite.recipes.OpenRewriteBestPractices --- .../java/testing/junit5/RemoveTryCatchFailBlocks.java | 2 +- .../java/testing/junit5/RemoveTryCatchFailBlocksTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java b/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java index 404ba64a7..e624ed00d 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java @@ -154,4 +154,4 @@ private J.MethodInvocation replaceWithAssertDoesNotThrowWithStringExpression(Exe .apply(getCursor(), try_.getCoordinates().replace(), try_.getBody(), failCallArgument); } } -} \ No newline at end of file +} diff --git a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java index 1444e2ea3..887be8cc3 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java @@ -665,4 +665,4 @@ String getFoo() { } }""")); } -} \ No newline at end of file +} From 41dd21c0062f0a0c173a8c8920ae7b0376b7971c Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Thu, 8 Aug 2024 15:50:50 +0200 Subject: [PATCH 3/5] Polish: strip unnecessary elements, add highlighting, consistent new lines --- .../testing/junit5/RemoveTryCatchFailBlocksTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java index 887be8cc3..b76aefae3 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java @@ -646,16 +646,13 @@ public void testMethod() { @Test @Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/547") void removeTryCatchFailBlocksWithReturningTry() { + //language=java rewriteRun( - spec -> spec.recipe(new RemoveTryCatchFailBlocks()), java( """ - package com.helloworld; - import static org.junit.jupiter.api.Assertions.fail; - public class Foo { - + class Foo { String getFoo() { try { return "foo"; @@ -663,6 +660,9 @@ String getFoo() { fail(); } } - }""")); + } + """ + ) + ); } } From e5a5e8c465b526e68ac38f1ae722f549816fe724 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Thu, 8 Aug 2024 15:53:42 +0200 Subject: [PATCH 4/5] Illustrate missed scenario with added test --- .../junit5/RemoveTryCatchFailBlocksTest.java | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java index b76aefae3..0c68d508e 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java @@ -645,7 +645,7 @@ public void testMethod() { @Test @Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/547") - void removeTryCatchFailBlocksWithReturningTry() { + void noChangeOnDirectReturn() { //language=java rewriteRun( java( @@ -665,4 +665,29 @@ String getFoo() { ) ); } + + @Test + @Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/547") + void noChangeOnNestedReturn() { + //language=java + rewriteRun( + java( + """ + import static org.junit.jupiter.api.Assertions.fail; + + class Foo { + String getBar(boolean b) { + try { + if (b) { + return "bar"; + } + } catch (RuntimeException e) { + fail(); + } + } + } + """ + ) + ); + } } From 1baf6594880e0ae027fb703302dbfa630befd435 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Thu, 8 Aug 2024 15:58:36 +0200 Subject: [PATCH 5/5] Fix failing scenario with a nested visitor --- .../junit5/RemoveTryCatchFailBlocks.java | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java b/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java index e624ed00d..eba2eca36 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java @@ -19,16 +19,14 @@ import org.openrewrite.Preconditions; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; -import org.openrewrite.java.JavaParser; -import org.openrewrite.java.JavaTemplate; -import org.openrewrite.java.JavaVisitor; -import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.*; import org.openrewrite.java.search.UsesMethod; import org.openrewrite.java.tree.*; import java.util.Collections; import java.util.Objects; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Stream; public class RemoveTryCatchFailBlocks extends Recipe { @@ -45,7 +43,7 @@ public String getDisplayName() { @Override public String getDescription() { return "Replace `try-catch` blocks where `catch` merely contains a `fail()` for `fail(String)` statement " + - "with `Assertions.assertDoesNotThrow(() -> { ... })`."; + "with `Assertions.assertDoesNotThrow(() -> { ... })`."; } @Override @@ -67,7 +65,16 @@ public J visitTry(J.Try jtry, ExecutionContext ctx) { return try_; } - if (try_.getBody().getStatements().stream().anyMatch((statement -> statement instanceof J.Return))) { + // Skip if any return is found, since we can't return from `assertDoesNotThrow` + AtomicBoolean returnFound = new AtomicBoolean(false); + new JavaIsoVisitor() { + @Override + public J.Return visitReturn(J.Return _return, AtomicBoolean atomicBoolean) { + atomicBoolean.set(true); + return _return; + } + }.visit(try_, returnFound, getCursor().getParentOrThrow()); + if (returnFound.get()) { return try_; } @@ -85,9 +92,9 @@ We would not want to convert for instance fail(cleanUpAndReturnMessage()) might return try_; } J.MethodInvocation failCall = (J.MethodInvocation) statement; - if (!ASSERT_FAIL_NO_ARG.matches(failCall) - && !ASSERT_FAIL_STRING_ARG.matches(failCall) - && !ASSERT_FAIL_THROWABLE_ARG.matches(failCall)) { + if (!ASSERT_FAIL_NO_ARG.matches(failCall) && + !ASSERT_FAIL_STRING_ARG.matches(failCall) && + !ASSERT_FAIL_THROWABLE_ARG.matches(failCall)) { return try_; }