Skip to content

Commit

Permalink
Dont convert try catch fail when return (#566)
Browse files Browse the repository at this point in the history 
* Solves #547

* Apply org.openrewrite.recipes.OpenRewriteBestPractices

* Polish: strip unnecessary elements, add highlighting, consistent new lines

* Illustrate missed scenario with added test

* Fix failing scenario with a nested visitor

---------

Co-authored-by: Laurens Westerlaken <[email protected]>
Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@Laurens-W @timtebeek
3 people authored Aug 8, 2024
1 parent ebd24fe commit ba9e367
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 36 deletions.
25 changes: 18 additions & 7 deletions src/main/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -67,6 +65,19 @@ public J visitTry(J.Try jtry, ExecutionContext ctx) {
return try_;
}

// Skip if any return is found, since we can't return from `assertDoesNotThrow`
AtomicBoolean returnFound = new AtomicBoolean(false);
new JavaIsoVisitor<AtomicBoolean>() {
@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_;
}

/*
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
Expand All @@ -81,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_;
}

Expand Down
106 changes: 77 additions & 29 deletions src/test/java/org/openrewrite/java/testing/junit5/RemoveTryCatchFailBlocksTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void removeTryCatchBlock() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -120,7 +120,7 @@ void removeTryCatchBlockWithoutMessage() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -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() {
Expand All @@ -157,7 +157,7 @@ void removeTryCatchBlockWithFailString() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -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() {
Expand All @@ -194,7 +194,7 @@ void failMethodArgIsNotGetMessage() {
"""
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Assertions;
class MyTest {
@Test
void aTest() {
Expand All @@ -204,7 +204,7 @@ void aTest() {
Assertions.fail(cleanUpAndReturnMessage());
}
}
String cleanUpAndReturnMessage() {
System.out.println("clean up code");
return "Oh no!";
Expand All @@ -223,7 +223,7 @@ void doesNotRunWithMultipleCatchBlocks() {
"""
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Assertions;
class MyTest {
@Test
void aTest() {
Expand All @@ -249,7 +249,7 @@ void catchHasMultipleStatements() {
"""
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Assertions;
class MyTest {
@Test
void aTest() {
Expand All @@ -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() {
Expand All @@ -299,7 +299,7 @@ void statementsBeforeAndAfterTryBlock() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -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() {
Expand Down Expand Up @@ -348,7 +348,7 @@ void failWithStringThrowableArgs() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
void testMethod() {
Expand All @@ -363,7 +363,7 @@ void testMethod() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
void testMethod() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -436,7 +436,7 @@ void failWithThrowable() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
void testMethod() {
Expand All @@ -451,7 +451,7 @@ void testMethod() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
void testMethod() {
Expand All @@ -473,7 +473,7 @@ void multipleTryCatchBlocks() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -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() {
Expand All @@ -518,7 +518,7 @@ void failHasBinaryWithException() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -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() {
Expand All @@ -555,7 +555,7 @@ void failHasBinaryWithMethodCall() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -565,7 +565,7 @@ public void testMethod() {
Assertions.fail("The error is: " + anotherMethod());
}
}
public String anotherMethod() {
return "anotherMethod";
}
Expand Down Expand Up @@ -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() {
Expand All @@ -625,7 +625,7 @@ void doesNotRunonTryFinally() {
"""
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class MyTest {
@Test
public void testMethod() {
Expand All @@ -642,4 +642,52 @@ public void testMethod() {
)
);
}

@Test
@Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/547")
void noChangeOnDirectReturn() {
//language=java
rewriteRun(
java(
"""
import static org.junit.jupiter.api.Assertions.fail;
class Foo {
String getFoo() {
try {
return "foo";
} catch (RuntimeException e) {
fail();
}
}
}
"""
)
);
}

@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();
}
}
}
"""
)
);
}
}

0 comments on commit ba9e367

Please sign in to comment.