From 496b8ea1e0b85fb93b79980a84e56768a4e08f43 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 24 Oct 2026 12:55:33 +0200 Subject: [PATCH 1/3] Only change Java sources in `TestMethodsShouldBeVoid` The recipe also visited Kotlin files, where it replaced the `Int` return type with a Java `void` primitive and removed the `return` statements, printing `fun testMethod(): void { }`, which does not compile. Kotlin test methods are handled by `KotlinTestMethodsShouldReturnUnit`, so this recipe now only accepts Java compilation units. --- .../cleanup/TestMethodsShouldBeVoid.java | 6 ++++++ .../cleanup/TestMethodsShouldBeVoidTest.java | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+) 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..e3c31de2d 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java @@ -20,6 +20,7 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Preconditions; import org.openrewrite.Recipe; +import org.openrewrite.SourceFile; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; @@ -46,6 +47,11 @@ public class TestMethodsShouldBeVoid extends Recipe { @Override public TreeVisitor getVisitor() { return Preconditions.check(new UsesType<>(TEST_ANNOTATION_TYPE_PATTERN, true), new JavaIsoVisitor() { + @Override + public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) { + return sourceFile instanceof J.CompilationUnit && super.isAcceptable(sourceFile, ctx); + } + @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); 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..0c28435ec 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoidTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoidTest.java @@ -324,6 +324,25 @@ void 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 From 8c83b7c5abc71a4ca60907c28a9ec78907162adb Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 24 Oct 2026 12:56:26 +0200 Subject: [PATCH 2/3] Keep early returns in `TestMethodsShouldBeVoid` and `KotlinTestMethodsShouldReturnUnit` Both recipes deleted every `return` in a test method, so a guard like `if (skip) { return 0; }` stopped exiting and the rest of the test ran anyway. Only a `return` that is the last statement of the method is now removed (keeping its expression when it is a valid expression statement); any other `return` loses just its value. Methods are left unchanged when an early return's value has side effects, when a Kotlin lambda returns from the test method, when the method overrides another, is called or referenced elsewhere in the same file, or has no body (previously an NPE). Returns in local classes and Kotlin local functions are no longer touched. --- .../KotlinTestMethodsShouldReturnUnit.java | 120 +++++++++--- .../cleanup/TestMethodsShouldBeVoid.java | 113 +++++++++--- .../resources/META-INF/rewrite/recipes.csv | 4 +- ...KotlinTestMethodsShouldReturnUnitTest.java | 154 +++++++++++++++- .../cleanup/TestMethodsShouldBeVoidTest.java | 172 ++++++++++++++++++ 5 files changed, 498 insertions(+), 65 deletions(-) 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 e3c31de2d..d7ba8c344 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java @@ -22,14 +22,19 @@ import org.openrewrite.Recipe; import org.openrewrite.SourceFile; 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 java.util.concurrent.atomic.AtomicBoolean; + import static java.util.Objects.requireNonNull; public class TestMethodsShouldBeVoid extends Recipe { @@ -42,7 +47,7 @@ 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() { @@ -55,56 +60,106 @@ public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) { @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 88ea0b407..c339e509b 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(..)`.",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 0c28435ec..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,176 @@ 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( From 6f441149c2ec3a3777d5ab1fdb870a375126ed75 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 24 Oct 2026 13:41:57 +0200 Subject: [PATCH 3/3] Use `JavaFileChecker` precondition in `TestMethodsShouldBeVoid` --- .../java/testing/cleanup/TestMethodsShouldBeVoid.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) 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 d7ba8c344..aef6dbfae 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/TestMethodsShouldBeVoid.java @@ -20,7 +20,6 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Preconditions; import org.openrewrite.Recipe; -import org.openrewrite.SourceFile; import org.openrewrite.TreeVisitor; import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; @@ -32,6 +31,7 @@ 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; @@ -51,12 +51,7 @@ public class TestMethodsShouldBeVoid extends Recipe { @Override public TreeVisitor getVisitor() { - return Preconditions.check(new UsesType<>(TEST_ANNOTATION_TYPE_PATTERN, true), new JavaIsoVisitor() { - @Override - public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) { - return sourceFile instanceof J.CompilationUnit && super.isAcceptable(sourceFile, ctx); - } - + 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);