diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnit.java b/src/main/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnit.java index 81aba3ddf..11081bf9b 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnit.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnit.java @@ -16,20 +16,23 @@ package org.openrewrite.java.testing.cleanup; import lombok.Getter; +import lombok.RequiredArgsConstructor; import org.jspecify.annotations.Nullable; import org.openrewrite.*; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.AnnotationMatcher; import org.openrewrite.java.search.UsesType; import org.openrewrite.java.service.AnnotationService; import org.openrewrite.java.tree.*; import org.openrewrite.kotlin.KotlinIsoVisitor; import org.openrewrite.kotlin.KotlinVisitor; -import org.openrewrite.kotlin.marker.KObject; import org.openrewrite.kotlin.marker.SingleExpressionBlock; import org.openrewrite.kotlin.tree.K; import org.openrewrite.kotlin.tree.KotlinTypeUtils; import org.openrewrite.marker.Markers; +import java.util.concurrent.atomic.AtomicBoolean; + import static java.util.Collections.emptyList; public class KotlinTestMethodsShouldReturnUnit extends Recipe { @@ -51,7 +54,7 @@ public class KotlinTestMethodsShouldReturnUnit extends Recipe { @Getter final String description = "Kotlin test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` " + "should have `Unit` return type. Other return types can cause test discovery issues, " + - "and warnings as of JUnit 5.13+. This recipe changes the return type to `Unit` and removes `return` statements."; + "and warnings as of JUnit 5.13+. This recipe changes the return type to `Unit` and drops the returned values. Test methods that override a method or are called elsewhere in the same file are left unchanged."; @Override public TreeVisitor getVisitor() { @@ -61,8 +64,9 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); // Skip invalid signatures or already-correct return types + J.Block body = m.getBody(); JavaType.Method methodType = m.getMethodType(); - if (m.getBody() == null || methodType == null || KotlinTypeUtils.isKotlinUnit(methodType.getReturnType())) { + if (body == null || methodType == null || KotlinTypeUtils.isKotlinUnit(methodType.getReturnType())) { return m; } @@ -74,13 +78,16 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex return m; } - // Update method and method identifier type. - JavaType.Method newMethodType = methodType.withReturnType(KOTLIN_UNIT); - m = m.withMethodType(newMethodType).withName(m.getName().withType(newMethodType)); + if (TypeUtils.isOverride(methodType) || isInvoked(getCursor().firstEnclosingOrThrow(K.CompilationUnit.class), m.getSimpleName())) { + return m; + } + + JavaType.Method unitMethodType = methodType.withReturnType(KOTLIN_UNIT); + J.MethodDeclaration unitMethod = m.withMethodType(unitMethodType).withName(m.getName().withType(unitMethodType)); // Add an explicit Unit return type - if (m.getBody() != null && m.getBody().getMarkers().findFirst(SingleExpressionBlock.class).isPresent()) { - return m.withReturnTypeExpression(new J.Identifier( + if (body.getMarkers().findFirst(SingleExpressionBlock.class).isPresent()) { + return unitMethod.withReturnTypeExpression(new J.Identifier( Tree.randomId(), Space.SINGLE_SPACE, Markers.EMPTY, @@ -90,41 +97,96 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex null)); } - // Otherwise, there's no need for a return type expression at all. - return m.withReturnTypeExpression(null) - // Remove return statements that are not in nested classes, objects, or lambdas. - .withBody((J.Block) new RemoveDirectReturns().visit(m.getBody(), ctx)); + AtomicBoolean unsafeReturn = new AtomicBoolean(); + J.Block withoutLastReturn = body.withStatements(ListUtils.mapLast(body.getStatements(), KotlinTestMethodsShouldReturnUnit::removeLastReturn)); + J.Block newBody = (J.Block) new RemoveEarlyReturnValues(m.getSimpleName()).visitNonNull(withoutLastReturn, unsafeReturn, getCursor()); + if (unsafeReturn.get()) { + return m; + } + return unitMethod.withReturnTypeExpression(null).withBody(newBody); } }); } - private static class RemoveDirectReturns extends KotlinVisitor { + private static @Nullable Statement removeLastReturn(Statement statement) { + if (!(statement instanceof K.Return)) { + return statement; + } + Expression expression = ((K.Return) statement).getExpression().getExpression(); + return expression instanceof Statement ? expression.withPrefix(statement.getPrefix()) : null; + } + + @RequiredArgsConstructor + private static class RemoveEarlyReturnValues extends KotlinVisitor { + private final String methodName; + @Override - public J visitClassDeclaration(J.ClassDeclaration classDeclaration, ExecutionContext ctx) { - return classDeclaration.getMarkers().findFirst(KObject.class).isPresent() ? - classDeclaration : // Retain nested object expressions - super.visitClassDeclaration(classDeclaration, ctx); + public J visitClassDeclaration(J.ClassDeclaration classDeclaration, AtomicBoolean unsafeReturn) { + return classDeclaration; } @Override - public J.Lambda visitLambda(J.Lambda lambda, ExecutionContext ctx) { - return lambda; // Retain nested returns + public J visitMethodDeclaration(J.MethodDeclaration method, AtomicBoolean unsafeReturn) { + return method; } @Override - public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext ctx) { - return newClass; // Retain nested returns + public J visitNewClass(J.NewClass newClass, AtomicBoolean unsafeReturn) { + return newClass; } @Override - public @Nullable J visitReturn(K.Return return_, ExecutionContext ctx) { - Expression returnExpr = return_.getExpression().getExpression(); - //noinspection DataFlowIssue - return returnExpr instanceof Statement ? - // Retain any side effects from statements in return expressions - returnExpr.withPrefix(return_.getPrefix()) : - // Remove any other return statements entirely - null; + public J visitReturn(K.Return return_, AtomicBoolean unsafeReturn) { + if (getCursor().dropParentUntil(t -> t instanceof J.Lambda || t instanceof J.MethodDeclaration).getValue() instanceof J.Lambda) { + J.Identifier label = return_.getLabel(); + if (label == null || methodName.equals(label.getSimpleName())) { + unsafeReturn.set(true); + } + return return_; + } + Expression expression = return_.getExpression().getExpression(); + if (expression == null) { + return return_; + } + if (hasSideEffects(expression)) { + unsafeReturn.set(true); + } + return return_.withExpression(return_.getExpression().withExpression(null)); } } + + private static boolean hasSideEffects(Expression expression) { + return new KotlinIsoVisitor() { + @Override + public @Nullable J preVisit(J tree, AtomicBoolean found) { + if (tree instanceof J.MethodInvocation || tree instanceof J.NewClass || + tree instanceof J.Assignment || tree instanceof J.AssignmentOperation || + tree instanceof J.Unary && ((J.Unary) tree).getOperator().isModifying()) { + found.set(true); + stopAfterPreVisit(); + } + return tree; + } + }.reduce(expression, new AtomicBoolean()).get(); + } + + private static boolean isInvoked(K.CompilationUnit cu, String methodName) { + return new KotlinIsoVisitor() { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicBoolean found) { + if (method.getSimpleName().equals(methodName)) { + found.set(true); + } + return super.visitMethodInvocation(method, found); + } + + @Override + public J.MemberReference visitMemberReference(J.MemberReference memberRef, AtomicBoolean found) { + if (memberRef.getReference().getSimpleName().equals(methodName)) { + found.set(true); + } + return super.visitMemberReference(memberRef, found); + } + }.reduce(cu, new AtomicBoolean()).get(); + } } diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java b/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java index 92b6e219f..aef6dbfae 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java @@ -21,13 +21,19 @@ import org.openrewrite.Preconditions; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.search.UsesType; +import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.Statement; +import org.openrewrite.java.tree.TypeTree; import org.openrewrite.java.tree.TypeUtils; +import org.openrewrite.staticanalysis.java.JavaFileChecker; + +import java.util.concurrent.atomic.AtomicBoolean; import static java.util.Objects.requireNonNull; @@ -41,64 +47,114 @@ public class TestMethodsShouldBeVoid extends Recipe { @Getter final String description = "Test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` " + "should have `void` return type. Non-void return types can cause test discovery issues, " + - "and warnings as of JUnit 5.13+. This recipe changes the return type to `void` and removes `return` statements."; + "and warnings as of JUnit 5.13+. This recipe changes the return type to `void` and drops the returned values. Test methods that override a method or are called elsewhere in the same file are left unchanged."; @Override public TreeVisitor getVisitor() { - return Preconditions.check(new UsesType<>(TEST_ANNOTATION_TYPE_PATTERN, true), new JavaIsoVisitor() { + return Preconditions.check(Preconditions.and(new JavaFileChecker<>(), new UsesType<>(TEST_ANNOTATION_TYPE_PATTERN, true)), new JavaIsoVisitor() { @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); - if (!hasJUnit5MethodAnnotation(m)) { + J.Block body = m.getBody(); + TypeTree returnType = m.getReturnTypeExpression(); + if (body == null || returnType == null || TypeUtils.isOfType(returnType.getType(), JavaType.Primitive.Void) || + !hasJUnit5MethodAnnotation(m) || TypeUtils.isOverride(m.getMethodType()) || + isInvoked(getCursor().firstEnclosingOrThrow(J.CompilationUnit.class), m.getSimpleName())) { return m; } - // Check if return type is already void - JavaType.Primitive voidType = JavaType.Primitive.Void; - if (m.getReturnTypeExpression() == null || TypeUtils.isOfType(m.getReturnTypeExpression().getType(), voidType)) { + AtomicBoolean earlyReturnHasSideEffects = new AtomicBoolean(); + J.Block withoutLastReturn = body.withStatements(ListUtils.mapLast(body.getStatements(), TestMethodsShouldBeVoid::removeLastReturn)); + J.Block newBody = (J.Block) new RemoveEarlyReturnValues().visitNonNull(withoutLastReturn, earlyReturnHasSideEffects, getCursor()); + if (earlyReturnHasSideEffects.get()) { return m; } - // Change return type to void - m = m.withReturnTypeExpression(new J.Primitive( - m.getReturnTypeExpression().getId(), - m.getReturnTypeExpression().getPrefix(), - m.getReturnTypeExpression().getMarkers(), - voidType - )); - - // Update method type + m = m.withReturnTypeExpression(new J.Primitive(returnType.getId(), returnType.getPrefix(), returnType.getMarkers(), JavaType.Primitive.Void)); if (m.getMethodType() != null) { - m = m.withMethodType(m.getMethodType().withReturnType(voidType)); + m = m.withMethodType(m.getMethodType().withReturnType(JavaType.Primitive.Void)); } - - // Remove return statements that are not in nested classes or lambdas - return m.withBody((J.Block) new RemoveDirectReturns().visit(requireNonNull(m.getBody()), ctx, getCursor().getParentTreeCursor())); + return m.withBody(newBody); } }); } - private static class RemoveDirectReturns extends JavaVisitor { + private static @Nullable Statement removeLastReturn(Statement statement) { + if (!(statement instanceof J.Return) || ((J.Return) statement).getExpression() == null) { + return statement; + } + Expression expression = requireNonNull(((J.Return) statement).getExpression()).unwrap(); + return isExpressionStatement(expression) ? expression.withPrefix(statement.getPrefix()) : null; + } + + private static class RemoveEarlyReturnValues extends JavaVisitor { + @Override + public J visitLambda(J.Lambda lambda, AtomicBoolean earlyReturnHasSideEffects) { + return lambda; + } + @Override - public J visitLambda(J.Lambda lambda, ExecutionContext ctx) { - return lambda; // Retain nested returns + public J visitNewClass(J.NewClass newClass, AtomicBoolean earlyReturnHasSideEffects) { + return newClass; } @Override - public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { - return newClass; // Retain nested returns + public J visitClassDeclaration(J.ClassDeclaration classDecl, AtomicBoolean earlyReturnHasSideEffects) { + return classDecl; } @Override - public @Nullable J visitReturn(J.Return retrn, ExecutionContext ctx) { - return retrn.getExpression() instanceof Statement ? - // Retain any side effects from expressions in return statements - retrn.getExpression().withPrefix(retrn.getPrefix()) : - // Remove any other return statements - null; + public J visitReturn(J.Return retrn, AtomicBoolean earlyReturnHasSideEffects) { + Expression expression = retrn.getExpression(); + if (expression == null) { + return retrn; + } + if (hasSideEffects(expression)) { + earlyReturnHasSideEffects.set(true); + } + return retrn.withExpression(null); } } + private static boolean isExpressionStatement(Expression expression) { + return expression instanceof J.MethodInvocation || expression instanceof J.NewClass || + expression instanceof J.Assignment || expression instanceof J.AssignmentOperation || + expression instanceof J.Unary && ((J.Unary) expression).getOperator().isModifying(); + } + + private static boolean hasSideEffects(Expression expression) { + return new JavaIsoVisitor() { + @Override + public @Nullable J preVisit(J tree, AtomicBoolean found) { + if (tree instanceof Expression && isExpressionStatement((Expression) tree)) { + found.set(true); + stopAfterPreVisit(); + } + return tree; + } + }.reduce(expression, new AtomicBoolean()).get(); + } + + private static boolean isInvoked(J.CompilationUnit cu, String methodName) { + return new JavaIsoVisitor() { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicBoolean found) { + if (method.getSimpleName().equals(methodName)) { + found.set(true); + } + return super.visitMethodInvocation(method, found); + } + + @Override + public J.MemberReference visitMemberReference(J.MemberReference memberRef, AtomicBoolean found) { + if (memberRef.getReference().getSimpleName().equals(methodName)) { + found.set(true); + } + return super.visitMemberReference(memberRef, found); + } + }.reduce(cu, new AtomicBoolean()).get(); + } + private static boolean hasJUnit5MethodAnnotation(J.MethodDeclaration method) { for (J.Annotation a : method.getLeadingAnnotations()) { if (TypeUtils.isOfClassType(a.getType(), "org.junit.jupiter.api.Test") || diff --git a/src/main/resources/META-INF/rewrite/recipes.csv b/src/main/resources/META-INF/rewrite/recipes.csv index f898bffaf..05e4b50fe 100644 --- a/src/main/resources/META-INF/rewrite/recipes.csv +++ b/src/main/resources/META-INF/rewrite/recipes.csv @@ -100,11 +100,11 @@ maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.tes maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.AssertTrueNullToAssertNull,Replace JUnit `assertTrue(a == null)` to `assertNull(a)`,Using `assertNull(a)` is simpler and more clear.,1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.AssertionsArgumentOrder,Assertion arguments should be passed in the correct order,"Assertions such as `org.junit.Assert.assertEquals` expect the first argument to be the expected value and the second argument to be the actual value; for `org.testng.Assert`, it’s the other way around. This recipe detects `J.Literal`, `J.NewArray`, and `java.util.Iterable` arguments swapping them if necessary so that the error messages won't be confusing.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.BestPractices,Testing best practices,Applies best practices to tests.,3,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, -maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.KotlinTestMethodsShouldReturnUnit,Kotlin test methods should have return type `Unit`,"Kotlin test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` should have `Unit` return type. Other return types can cause test discovery issues, and warnings as of JUnit 5.13+. This recipe changes the return type to `Unit` and removes `return` statements.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, +maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.KotlinTestMethodsShouldReturnUnit,Kotlin test methods should have return type `Unit`,"Kotlin test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` should have `Unit` return type. Other return types can cause test discovery issues, and warnings as of JUnit 5.13+. This recipe changes the return type to `Unit` and drops the returned values. Test methods that override a method or are called elsewhere in the same file are left unchanged.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.RemoveEmptyTests,Remove empty tests without comments,Removes empty methods with a `@Test` annotation if the body does not have comments.,1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.RemoveTestPrefix,Remove `test` prefix from JUnit 5 tests,"Remove `test` from methods with `@Test`, `@ParameterizedTest`, `@RepeatedTest` or `@TestFactory`. They no longer have to prefix test to be usable by JUnit 5.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.SimplifyTestThrows,Simplify `throws` statements of tests,Replace all thrown exception classes of test method signatures by `Exception`.,1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, -maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.TestMethodsShouldBeVoid,Test methods should have void return type,"Test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` should have `void` return type. Non-void return types can cause test discovery issues, and warnings as of JUnit 5.13+. This recipe changes the return type to `void` and removes `return` statements.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, +maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.TestMethodsShouldBeVoid,Test methods should have void return type,"Test methods annotated with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestTemplate` should have `void` return type. Non-void return types can cause test discovery issues, and warnings as of JUnit 5.13+. This recipe changes the return type to `void` and drops the returned values. Test methods that override a method or are called elsewhere in the same file are left unchanged.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.TestsShouldIncludeAssertions,Include an assertion in tests,"For tests not having any assertions, wrap the statements with JUnit Jupiter's `Assertions#assertDoesNotThrow(..)`. Tests that make an assumption are left unchanged, as wrapping them would report a skipped test as a failure.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,"[{""name"":""additionalAsserts"",""type"":""String"",""displayName"":""Additional assertions"",""description"":""A comma delimited list of packages and/or classes that will be identified as assertions. I.E. a common assertion utility `org.foo.TestUtil`."",""example"":""org.foo.TestUtil, org.bar""}]", maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.cleanup.TestsShouldNotBePublic,Remove `public` visibility of JUnit 5 tests,"Remove `public` and optionally `protected` modifiers from methods with `@Test`, `@ParameterizedTest`, `@RepeatedTest`, `@TestFactory`, `@BeforeEach`, `@AfterEach`, `@BeforeAll`, or `@AfterAll`. They no longer have to be public visibility to be usable by JUnit 5.",1,Test cleanup,Testing,Java,Recipes to simplify assertions and clean up test code.,,Basic building blocks for transforming Java code.,"[{""name"":""removeProtectedModifiers"",""type"":""Boolean"",""displayName"":""Remove protected modifiers"",""description"":""Also remove protected modifiers from test methods"",""example"":""true""}]", maven,org.openrewrite.recipe:rewrite-testing-frameworks,org.openrewrite.java.testing.datafaker.JavaFakerToDataFaker,Migrate from Java Faker to Datafaker,Change imports and dependencies related to Java Faker to Datafaker replacements.,6,DataFaker,Testing,Java,Recipes for migrating from JavaFaker to [DataFaker](https://www.datafaker.net/).,,Basic building blocks for transforming Java code.,, diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnitTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnitTest.java index e66ff19c0..ab3187500 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnitTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/KotlinTestMethodsShouldReturnUnitTest.java @@ -172,7 +172,9 @@ class ATest { @Test fun myTest() { if (true) { + return } else { + return } } } @@ -182,7 +184,7 @@ fun myTest() { } @Test - void preserveReturnInLambda() { + void preserveLabeledReturnInLambda() { //language=kotlin rewriteRun( kotlin( @@ -192,8 +194,8 @@ void preserveReturnInLambda() { class ATest { @Test fun myTest() : Int { - val supplier = { return 42 } - return supplier() + val value = run { return@run 42 } + return value } } """, @@ -203,8 +205,150 @@ fun myTest() : Int { class ATest { @Test fun myTest() { - val supplier = { return 42 } - supplier() + val value = run { return@run 42 } + } + } + """ + ) + ); + } + + @Test + void preserveReturnInLocalFunction() { + //language=kotlin + rewriteRun( + kotlin( + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun myTest() : Int { + fun helper(): Int { + return 42 + } + return helper() + } + } + """, + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun myTest() { + fun helper(): Int { + return 42 + } + helper() + } + } + """ + ) + ); + } + + @Test + void keepEarlyReturnAsGuard() { + //language=kotlin + rewriteRun( + kotlin( + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun myTest(): Int { + if (System.getenv("SKIP") != null) { + return 0 + } + println("test") + return 1 + } + } + """, + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun myTest() { + if (System.getenv("SKIP") != null) { + return + } + println("test") + } + } + """ + ) + ); + } + + @Test + void doNotChangeNonLocalReturnFromInlineLambda() { + //language=kotlin + rewriteRun( + kotlin( + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun myTest(): Int { + listOf(1, 2).forEach { + if (it == 2) return it + } + return 0 + } + } + """ + ) + ); + } + + @Test + void doNotChangeEarlyReturnWithSideEffects() { + //language=kotlin + rewriteRun( + kotlin( + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun myTest(): Int { + if (System.getenv("SKIP") != null) { + return cleanUp() + } + return 1 + } + + fun cleanUp(): Int { + return 0 + } + } + """ + ) + ); + } + + @Test + void doNotChangeTestInvokedAsHelper() { + //language=kotlin + rewriteRun( + kotlin( + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun compute(): Int { + return 42 + } + + @Test + fun reuse() { + println(compute()) } } """ diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoidTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoidTest.java index 01f1fa8b2..e842dffbc 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoidTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoidTest.java @@ -315,7 +315,9 @@ class ATest { @Test void testMethod() { if (true) { + return; } else { + return; } } } @@ -324,6 +326,195 @@ void testMethod() { ); } + @Test + void keepEarlyReturnAsGuard() { + //language=java + rewriteRun( + java( + """ + import org.junit.jupiter.api.Test; + + import static org.junit.jupiter.api.Assertions.fail; + + class ATest { + @Test + int testMethod() { + if (System.getenv("SKIP") != null) { + return 0; + } + fail(); + return 1; + } + } + """, + """ + import org.junit.jupiter.api.Test; + + import static org.junit.jupiter.api.Assertions.fail; + + class ATest { + @Test + void testMethod() { + if (System.getenv("SKIP") != null) { + return; + } + fail(); + } + } + """ + ) + ); + } + + @Test + void preserveReturnInLocalClass() { + //language=java + rewriteRun( + java( + """ + import org.junit.jupiter.api.Test; + + class ATest { + @Test + int testMethod() { + class Local { + int value() { + return 42; + } + } + return new Local().value(); + } + } + """, + """ + import org.junit.jupiter.api.Test; + + class ATest { + @Test + void testMethod() { + class Local { + int value() { + return 42; + } + } + new Local().value(); + } + } + """ + ) + ); + } + + @Test + void doNotChangeEarlyReturnWithSideEffects() { + //language=java + rewriteRun( + java( + """ + import org.junit.jupiter.api.Test; + + class ATest { + @Test + int testMethod() { + if (System.getenv("SKIP") != null) { + return cleanUp(); + } + return 1; + } + + private int cleanUp() { + return 0; + } + } + """ + ) + ); + } + + @Test + void doNotChangeTestInvokedAsHelper() { + //language=java + rewriteRun( + java( + """ + import org.junit.jupiter.api.Test; + + import static org.junit.jupiter.api.Assertions.assertEquals; + + class ATest { + @Test + int compute() { + return 42; + } + + @Test + void reuse() { + assertEquals(42, compute()); + } + } + """ + ) + ); + } + + @Test + void doNotChangeOverride() { + //language=java + rewriteRun( + java( + """ + import java.util.concurrent.Callable; + + import org.junit.jupiter.api.Test; + + class ATest implements Callable { + @Override + @Test + public Integer call() { + return 42; + } + } + """ + ) + ); + } + + @Test + void doNotChangeAbstractTestMethod() { + //language=java + rewriteRun( + java( + """ + import org.junit.jupiter.api.Test; + + abstract class ATest { + @Test + abstract int testMethod(); + } + """ + ) + ); + } + + @Test + void doNotChangeKotlin() { + rewriteRun( + //language=kotlin + kotlin( + """ + import org.junit.jupiter.api.Test + + class ATest { + @Test + fun testMethod(): Int { + return 42 + } + } + """ + ) + ); + } + @Test void preserveExpressionFromReturn() { //language=java