From 8638eb4083d95d9cf4094b92ea520533205a856e Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 2 Apr 2025 16:55:37 +0200 Subject: [PATCH 01/20] Added recipe to remove @VisibleForTesting annotation when used from production code --- ...TestingAnnotationWhenUsedInProduction.java | 181 ++++++ ...ingAnnotationWhenUsedInProductionTest.java | 562 ++++++++++++++++++ 2 files changed, 743 insertions(+) create mode 100644 src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java create mode 100644 src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java new file mode 100644 index 000000000..87aafc77f --- /dev/null +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -0,0 +1,181 @@ +package org.openrewrite.java.testing.cleanup; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.ScanningRecipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.RemoveAnnotation; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; + +public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { + + public static class Scanned { + List methods = new ArrayList<>(); + List fields = new ArrayList<>(); + List classes = new ArrayList<>(); + } + + @Override + public String getDisplayName() { + return "Remove `@VisibleForTesting` annotation when target is used in production"; + } + + @Override + public String getDescription() { + return "The `@VisibleForTesting` annotation is used when a method or a field has been made more accessible then it would normally be, solely for testing purposes. " + + "This recipe removes the annotation where such an element is used from production classes. It identifies production classes as classes in `src/main` and test classes as classes in `src/test`. " + + "It will remove the `@VisibleForTesting` from methods, fields (both member fields and constants), constructors and inner classes. " + + "This recipe should not be used in an environment where QA tooling acts on the `@VisibleForTesting` annotation."; + } + + @Override + public Scanned getInitialValue(ExecutionContext ctx) { + return new Scanned(); + } + + @Override + public TreeVisitor getScanner(Scanned acc) { + return new JavaIsoVisitor() { + + @Override + public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext executionContext) { + // Mark classes + if (fieldAccess.getTarget().getType() instanceof JavaType.Class) { + JavaType.Class type = (JavaType.Class) fieldAccess.getTarget().getType(); + if (!acc.classes.contains(type)) { + type.getAnnotations().forEach(annotation -> { + if ("VisibleForTesting".equals(annotation.getClassName())) { + J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); + if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { + acc.classes.add(type); + } + } + }); + } + } + // Mark fields + if(fieldAccess.getName().getFieldType() != null && !acc.fields.contains(fieldAccess.getName().getFieldType())) { + fieldAccess.getName().getFieldType().getAnnotations().forEach(annotation -> { + if ("VisibleForTesting".equals(annotation.getClassName())) { + J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); + if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { + acc.fields.add(fieldAccess.getName().getFieldType()); + } + } + }); + } + + return super.visitFieldAccess(fieldAccess, executionContext); + } + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { + if (method.getMethodType() != null) { + if (!acc.methods.contains(method.getMethodType())) { + method.getMethodType().getAnnotations().forEach(annotation -> { + if ("VisibleForTesting".equals(annotation.getClassName())) { + J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); + if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { + acc.methods.add(method.getMethodType()); + } + } + }); + } + } + return super.visitMethodInvocation(method, executionContext); + } + + @Override + public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext executionContext) { + // Mark constructors + if (newClass.getConstructorType() != null) { + if(!acc.methods.contains(newClass.getConstructorType())) { + newClass.getConstructorType().getAnnotations().forEach(annotation -> { + if ("VisibleForTesting".equals(annotation.getClassName())) { + J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); + if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { + acc.methods.add(newClass.getConstructorType()); + } + } + }); + } + } + // Mark classes + if (newClass.getClazz() != null && newClass.getClazz().getType() != null && newClass.getClazz().getType() instanceof JavaType.Class) { + JavaType.Class clazzType = (JavaType.Class) newClass.getClazz().getType(); + if (!acc.classes.contains(clazzType)) { + clazzType.getAnnotations().forEach(annotation -> { + if ("VisibleForTesting".equals(annotation.getClassName())) { + J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); + if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { + acc.classes.add(clazzType); + } + } + }); + } + } + return super.visitNewClass(newClass, executionContext); + } + }; + } + + @Override + public TreeVisitor getVisitor(Scanned acc) { + return new JavaIsoVisitor() { + + @Override + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext executionContext) { + J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(multiVariable, executionContext); + if(!variableDeclarations.getVariables().isEmpty()) { + // if none of the variables in the declaration are used from production code, the annotation should be kept + boolean keepAnnotation = variableDeclarations.getVariables().stream().noneMatch(elem -> acc.fields.contains(elem.getVariableType())); + if (!keepAnnotation) { + Optional annotation = variableDeclarations.getLeadingAnnotations().stream() + .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) + .findFirst(); + if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { + JavaType.Class type = (JavaType.Class) annotation.get().getType(); + return (J.VariableDeclarations) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(variableDeclarations, executionContext, getCursor().getParentOrThrow()); + } + } + } + return variableDeclarations; + } + + @Override + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext executionContext) { + J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(method, executionContext); + if (acc.methods.contains(methodDeclaration.getMethodType())) { + Optional annotation = methodDeclaration.getLeadingAnnotations().stream() + .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) + .findFirst(); + if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { + JavaType.Class type = (JavaType.Class) annotation.get().getType(); + return (J.MethodDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(methodDeclaration, executionContext, getCursor().getParentOrThrow()); + } + } + return methodDeclaration; + } + + @Override + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext executionContext) { + J.ClassDeclaration classDeclaration = super.visitClassDeclaration(classDecl, executionContext); + if (acc.classes.contains(classDeclaration.getType())) { + Optional annotation = classDeclaration.getLeadingAnnotations().stream() + .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) + .findFirst(); + if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { + JavaType.Class type = (JavaType.Class) annotation.get().getType(); + return (J.ClassDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(classDeclaration, executionContext, getCursor().getParentOrThrow()); + } + } + return classDeclaration; + } + }; + } +} diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java new file mode 100644 index 000000000..5d35f1803 --- /dev/null +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -0,0 +1,562 @@ +package org.openrewrite.java.testing.cleanup; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.*; + +class RemoveVisibleForTestingAnnotationWhenUsedInProductionTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec + .recipe(new RemoveVisibleForTestingAnnotationWhenUsedInProduction()); + } + + @DocumentExample + @Test + void document() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + + @VisibleForTesting + public String internalState; + @VisibleForTesting + public String externalState; + + @VisibleForTesting + public String getInternalState() { + return internalState; + } + + @VisibleForTesting + public String getExternalState() { + return externalState; + } + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + + @VisibleForTesting + public String internalState; + public String externalState; + + @VisibleForTesting + public String getInternalState() { + return internalState; + } + + public String getExternalState() { + return externalState; + } + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + String variableOne = production.externalState; + String variableTwo = production.getExternalState(); + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + String variableOne = production.externalState; + String variableTwo = production.getExternalState(); + String variableThree = production.internalState; + String variableFour = production.getInternalState(); + } + } + """ + ) + ) + ); + } + + @Test + void fieldAccess() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public String internalState; + @VisibleForTesting + public String externalState; + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public String internalState; + public String externalState; + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + String variableOne = production.externalState; + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + String variableOne = production.externalState; + String variableTwo = production.internalState; + } + } + """ + ) + ) + ); + } + + @Test + void fieldsAccess() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public String externalState1, externalState2; + @VisibleForTesting + public String internalState1, externalState3; + @VisibleForTesting + public String internalState2, internalState3; + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + public String externalState1, externalState2; + public String internalState1, externalState3; + @VisibleForTesting + public String internalState2, internalState3; + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + String variableOne = production.externalState1; + String variableTwo = production.externalState2; + String variableThree = production.externalState3; + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + String variableOne = production.externalState1; + String variableTwo = production.externalState2; + String variableThree = production.externalState3; + String variableFour = production.internalState1; + String variableFive = production.internalState2; + String variableSix = production.internalState3; + } + } + """ + ) + ) + ); + } + + @Test + void methodInvocation() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public void internalCall(){} + @VisibleForTesting + public void externalCall(){} + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public void internalCall(){} + public void externalCall(){} + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + production.externalCall(); + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + production.internalCall(); + production.externalCall(); + } + } + """ + ) + ) + ); + } + + @Test + void constructor() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public Production(){} + @VisibleForTesting + public Production(int debugLevel) {} + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + public Production(){} + @VisibleForTesting + public Production(int debugLevel) {} + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call() { + new Production(); + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + new Production(); + new Production(1); + } + } + """ + ) + ) + ); + } + + @Test + void referenceStaticInnerClass() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static class InternalInner { + @VisibleForTesting + public static String internalState; + } + + @VisibleForTesting + public static class ExternalInner { + @VisibleForTesting + public static String externalState; + } + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static class InternalInner { + @VisibleForTesting + public static String internalState; + } + + public static class ExternalInner { + public static String externalState; + } + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + String variableOne = Production.ExternalInner.externalState; + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + String variableOne = Production.ExternalInner.externalState; + String variableTwo = Production.InternalInner.internalState; + } + } + """ + ) + ) + ); + } + + @Test + void instantiateStaticInnerClass() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static class InternalInner {} + + @VisibleForTesting + public static class ExternalInner {} + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static class InternalInner {} + + public static class ExternalInner {} + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + new Production.ExternalInner(); + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + new Production.InternalInner(); + new Production.ExternalInner(); + } + } + """ + ) + ) + ); + } + + @Test + void referenceConstant() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static final long INTERNAL_CONSTANT = 1L; + @VisibleForTesting + public static final long EXTERNAL_CONSTANT = 2L; + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static final long INTERNAL_CONSTANT = 1L; + public static final long EXTERNAL_CONSTANT = 2L; + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + long variableOne = Production.EXTERNAL_CONSTANT; + } + } + """) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + long variableOne = Production.INTERNAL_CONSTANT; + long variableTwo = Production.EXTERNAL_CONSTANT; + } + } + """ + ) + ) + ); + } +} From 6610f25d8e831412d4c52aecf247b469e9381c70 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 2 Apr 2025 17:01:22 +0200 Subject: [PATCH 02/20] Added license --- ...eForTestingAnnotationWhenUsedInProduction.java | 15 +++++++++++++++ ...TestingAnnotationWhenUsedInProductionTest.java | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 87aafc77f..0eb88d6d9 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -1,3 +1,18 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.java.testing.cleanup; import org.openrewrite.ExecutionContext; diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index 5d35f1803..29614c5fa 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -1,3 +1,18 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.java.testing.cleanup; import org.junit.jupiter.api.Test; From 03926fae398f02d73d33990133dc4a8fa10b4121 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 2 Apr 2025 17:02:58 +0200 Subject: [PATCH 03/20] Renamed executionContext to ctx --- ...TestingAnnotationWhenUsedInProduction.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 0eb88d6d9..eebf63f1c 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -58,7 +58,7 @@ public TreeVisitor getScanner(Scanned acc) { return new JavaIsoVisitor() { @Override - public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext executionContext) { + public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { // Mark classes if (fieldAccess.getTarget().getType() instanceof JavaType.Class) { JavaType.Class type = (JavaType.Class) fieldAccess.getTarget().getType(); @@ -85,11 +85,11 @@ public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContex }); } - return super.visitFieldAccess(fieldAccess, executionContext); + return super.visitFieldAccess(fieldAccess, ctx); } @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { if (method.getMethodType() != null) { if (!acc.methods.contains(method.getMethodType())) { method.getMethodType().getAnnotations().forEach(annotation -> { @@ -102,11 +102,11 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu }); } } - return super.visitMethodInvocation(method, executionContext); + return super.visitMethodInvocation(method, ctx); } @Override - public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext executionContext) { + public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext ctx) { // Mark constructors if (newClass.getConstructorType() != null) { if(!acc.methods.contains(newClass.getConstructorType())) { @@ -134,7 +134,7 @@ public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext executionC }); } } - return super.visitNewClass(newClass, executionContext); + return super.visitNewClass(newClass, ctx); } }; } @@ -144,8 +144,8 @@ public TreeVisitor getVisitor(Scanned acc) { return new JavaIsoVisitor() { @Override - public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext executionContext) { - J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(multiVariable, executionContext); + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { + J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(multiVariable, ctx); if(!variableDeclarations.getVariables().isEmpty()) { // if none of the variables in the declaration are used from production code, the annotation should be kept boolean keepAnnotation = variableDeclarations.getVariables().stream().noneMatch(elem -> acc.fields.contains(elem.getVariableType())); @@ -155,7 +155,7 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m .findFirst(); if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return (J.VariableDeclarations) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(variableDeclarations, executionContext, getCursor().getParentOrThrow()); + return (J.VariableDeclarations) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(variableDeclarations, ctx, getCursor().getParentOrThrow()); } } } @@ -163,30 +163,30 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m } @Override - public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext executionContext) { - J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(method, executionContext); + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { + J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(method, ctx); if (acc.methods.contains(methodDeclaration.getMethodType())) { Optional annotation = methodDeclaration.getLeadingAnnotations().stream() .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) .findFirst(); if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return (J.MethodDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(methodDeclaration, executionContext, getCursor().getParentOrThrow()); + return (J.MethodDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(methodDeclaration, ctx, getCursor().getParentOrThrow()); } } return methodDeclaration; } @Override - public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext executionContext) { - J.ClassDeclaration classDeclaration = super.visitClassDeclaration(classDecl, executionContext); + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { + J.ClassDeclaration classDeclaration = super.visitClassDeclaration(classDecl, ctx); if (acc.classes.contains(classDeclaration.getType())) { Optional annotation = classDeclaration.getLeadingAnnotations().stream() .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) .findFirst(); if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return (J.ClassDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(classDeclaration, executionContext, getCursor().getParentOrThrow()); + return (J.ClassDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(classDeclaration, ctx, getCursor().getParentOrThrow()); } } return classDeclaration; From 5ea0619b5dccfcde7241cf47a0b04a6ebe5b1a31 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 2 Apr 2025 19:21:31 +0200 Subject: [PATCH 04/20] Improvements based on review --- ...TestingAnnotationWhenUsedInProduction.java | 142 ++++++++---------- ...ingAnnotationWhenUsedInProductionTest.java | 112 +++++++------- 2 files changed, 122 insertions(+), 132 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index eebf63f1c..2c338b0a4 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -15,24 +15,29 @@ */ package org.openrewrite.java.testing.cleanup; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.ScanningRecipe; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.RemoveAnnotation; +import org.openrewrite.java.marker.JavaSourceSet; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.TypeUtils; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Optional; public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { public static class Scanned { - List methods = new ArrayList<>(); - List fields = new ArrayList<>(); - List classes = new ArrayList<>(); + List methods = new ArrayList<>(); + List fields = new ArrayList<>(); + List classes = new ArrayList<>(); } @Override @@ -61,46 +66,19 @@ public TreeVisitor getScanner(Scanned acc) { public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { // Mark classes if (fieldAccess.getTarget().getType() instanceof JavaType.Class) { - JavaType.Class type = (JavaType.Class) fieldAccess.getTarget().getType(); - if (!acc.classes.contains(type)) { - type.getAnnotations().forEach(annotation -> { - if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); - if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { - acc.classes.add(type); - } - } - }); - } + checkAndRegister(acc.classes, fieldAccess.getTarget().getType()); } // Mark fields - if(fieldAccess.getName().getFieldType() != null && !acc.fields.contains(fieldAccess.getName().getFieldType())) { - fieldAccess.getName().getFieldType().getAnnotations().forEach(annotation -> { - if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); - if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { - acc.fields.add(fieldAccess.getName().getFieldType()); - } - } - }); + if (fieldAccess.getName().getFieldType() != null) { + checkAndRegister(acc.fields, fieldAccess.getName().getFieldType()); } - return super.visitFieldAccess(fieldAccess, ctx); } @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { if (method.getMethodType() != null) { - if (!acc.methods.contains(method.getMethodType())) { - method.getMethodType().getAnnotations().forEach(annotation -> { - if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); - if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { - acc.methods.add(method.getMethodType()); - } - } - }); - } + checkAndRegister(acc.methods, method.getMethodType()); } return super.visitMethodInvocation(method, ctx); } @@ -109,33 +87,42 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext ctx) { // Mark constructors if (newClass.getConstructorType() != null) { - if(!acc.methods.contains(newClass.getConstructorType())) { - newClass.getConstructorType().getAnnotations().forEach(annotation -> { - if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); - if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { - acc.methods.add(newClass.getConstructorType()); - } - } - }); - } + checkAndRegister(acc.methods, newClass.getConstructorType()); } // Mark classes if (newClass.getClazz() != null && newClass.getClazz().getType() != null && newClass.getClazz().getType() instanceof JavaType.Class) { - JavaType.Class clazzType = (JavaType.Class) newClass.getClazz().getType(); - if (!acc.classes.contains(clazzType)) { - clazzType.getAnnotations().forEach(annotation -> { - if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); - if (compilationUnit != null && compilationUnit.getSourcePath().startsWith("src/main")) { - acc.classes.add(clazzType); - } - } - }); - } + checkAndRegister(acc.classes, newClass.getClazz().getType()); } return super.visitNewClass(newClass, ctx); } + + private void checkAndRegister(List target, JavaType type) { + if (!target.contains(TypeUtils.toString(type))) { + getAnnotations(type).forEach(annotation -> { + if ("VisibleForTesting".equals(annotation.getClassName())) { + J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); + if (compilationUnit != null) { + compilationUnit + .getMarkers() + .findFirst(JavaSourceSet.class) + .filter(elem -> "main".equals(elem.getName())) + .ifPresent(sourceSet -> target.add(TypeUtils.toString(type))); + } + } + }); + } + } + + private @NotNull List getAnnotations(JavaType type) { + if (type instanceof JavaType.Class) { + return ((JavaType.Class) type).getAnnotations(); + } else if (type instanceof JavaType.Variable) { + return ((JavaType.Variable) type).getAnnotations(); + } else if (type instanceof JavaType.Method) { + return ((JavaType.Method) type).getAnnotations(); + } + return Collections.emptyList(); + } }; } @@ -146,17 +133,13 @@ public TreeVisitor getVisitor(Scanned acc) { @Override public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(multiVariable, ctx); - if(!variableDeclarations.getVariables().isEmpty()) { + if (!variableDeclarations.getVariables().isEmpty()) { // if none of the variables in the declaration are used from production code, the annotation should be kept - boolean keepAnnotation = variableDeclarations.getVariables().stream().noneMatch(elem -> acc.fields.contains(elem.getVariableType())); + boolean keepAnnotation = variableDeclarations.getVariables().stream() + .filter(elem -> elem.getVariableType() != null) + .noneMatch(elem -> acc.fields.contains(TypeUtils.toString(elem.getVariableType()))); if (!keepAnnotation) { - Optional annotation = variableDeclarations.getLeadingAnnotations().stream() - .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) - .findFirst(); - if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { - JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return (J.VariableDeclarations) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(variableDeclarations, ctx, getCursor().getParentOrThrow()); - } + return (J.VariableDeclarations) getElement(ctx, variableDeclarations.getLeadingAnnotations(), variableDeclarations); } } return variableDeclarations; @@ -165,14 +148,8 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(method, ctx); - if (acc.methods.contains(methodDeclaration.getMethodType())) { - Optional annotation = methodDeclaration.getLeadingAnnotations().stream() - .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) - .findFirst(); - if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { - JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return (J.MethodDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(methodDeclaration, ctx, getCursor().getParentOrThrow()); - } + if (methodDeclaration.getMethodType() != null && acc.methods.contains(TypeUtils.toString(methodDeclaration.getMethodType()))) { + return (J.MethodDeclaration) getElement(ctx, methodDeclaration.getLeadingAnnotations(), methodDeclaration); } return methodDeclaration; } @@ -180,17 +157,22 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex @Override public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { J.ClassDeclaration classDeclaration = super.visitClassDeclaration(classDecl, ctx); - if (acc.classes.contains(classDeclaration.getType())) { - Optional annotation = classDeclaration.getLeadingAnnotations().stream() - .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) - .findFirst(); - if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { - JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return (J.ClassDeclaration) new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(classDeclaration, ctx, getCursor().getParentOrThrow()); - } + if (classDeclaration.getType() != null && acc.classes.contains(TypeUtils.toString(classDeclaration.getType()))) { + return (J.ClassDeclaration) getElement(ctx, classDeclaration.getLeadingAnnotations(), classDeclaration); } return classDeclaration; } + + private <@Nullable T extends J> J getElement(ExecutionContext ctx, List leadingAnnotations, T target) { + Optional annotation = leadingAnnotations.stream() + .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) + .findFirst(); + if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { + JavaType.Class type = (JavaType.Class) annotation.get().getType(); + return new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(target, ctx, getCursor().getParentOrThrow()); + } + return target; + } }; } } diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index 29614c5fa..507a11628 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -94,23 +94,24 @@ void call(Production production) { String variableTwo = production.getExternalState(); } } - """) + """ + ) ), srcTestJava( java( """ - package com.example.test; + package com.example.test; - import com.example.domain.Production; + import com.example.domain.Production; - class ProductionTest { - void test(Production production) { - String variableOne = production.externalState; - String variableTwo = production.getExternalState(); - String variableThree = production.internalState; - String variableFour = production.getInternalState(); - } + class ProductionTest { + void test(Production production) { + String variableOne = production.externalState; + String variableTwo = production.getExternalState(); + String variableThree = production.internalState; + String variableFour = production.getInternalState(); } + } """ ) ) @@ -158,21 +159,22 @@ void call(Production production) { String variableOne = production.externalState; } } - """) + """ + ) ), srcTestJava( java( """ - package com.example.test; + package com.example.test; - import com.example.domain.Production; + import com.example.domain.Production; - class ProductionTest { - void test(Production production) { - String variableOne = production.externalState; - String variableTwo = production.internalState; - } + class ProductionTest { + void test(Production production) { + String variableOne = production.externalState; + String variableTwo = production.internalState; } + } """ ) ) @@ -225,7 +227,8 @@ void call(Production production) { String variableThree = production.externalState3; } } - """) + """ + ) ), srcTestJava( java( @@ -291,21 +294,22 @@ void call(Production production) { production.externalCall(); } } - """) + """ + ) ), srcTestJava( java( """ - package com.example.test; + package com.example.test; - import com.example.domain.Production; + import com.example.domain.Production; - class ProductionTest { - void test(Production production) { - production.internalCall(); - production.externalCall(); - } + class ProductionTest { + void test(Production production) { + production.internalCall(); + production.externalCall(); } + } """ ) ) @@ -353,21 +357,22 @@ void call() { new Production(); } } - """) + """ + ) ), srcTestJava( java( """ - package com.example.test; + package com.example.test; - import com.example.domain.Production; + import com.example.domain.Production; - class ProductionTest { - void test(Production production) { - new Production(); - new Production(1); - } + class ProductionTest { + void test(Production production) { + new Production(); + new Production(1); } + } """ ) ) @@ -428,7 +433,8 @@ void call(Production production) { String variableOne = Production.ExternalInner.externalState; } } - """) + """ + ) ), srcTestJava( java( @@ -492,21 +498,22 @@ void call(Production production) { new Production.ExternalInner(); } } - """) + """ + ) ), srcTestJava( java( """ - package com.example.test; + package com.example.test; - import com.example.domain.Production; + import com.example.domain.Production; - class ProductionTest { - void test(Production production) { - new Production.InternalInner(); - new Production.ExternalInner(); - } + class ProductionTest { + void test(Production production) { + new Production.InternalInner(); + new Production.ExternalInner(); } + } """ ) ) @@ -554,21 +561,22 @@ void call(Production production) { long variableOne = Production.EXTERNAL_CONSTANT; } } - """) + """ + ) ), srcTestJava( java( """ - package com.example.test; + package com.example.test; - import com.example.domain.Production; + import com.example.domain.Production; - class ProductionTest { - void test(Production production) { - long variableOne = Production.INTERNAL_CONSTANT; - long variableTwo = Production.EXTERNAL_CONSTANT; - } + class ProductionTest { + void test(Production production) { + long variableOne = Production.INTERNAL_CONSTANT; + long variableTwo = Production.EXTERNAL_CONSTANT; } + } """ ) ) From 76e54c2c218950a3201642cdec6cfc76ee536ee6 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 2 Apr 2025 19:28:14 +0200 Subject: [PATCH 05/20] Removed JetBrains annotations --- ...oveVisibleForTestingAnnotationWhenUsedInProduction.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 2c338b0a4..e39bde00e 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -15,8 +15,7 @@ */ package org.openrewrite.java.testing.cleanup; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; +import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.ScanningRecipe; import org.openrewrite.TreeVisitor; @@ -113,7 +112,7 @@ private void checkAndRegister(List target, JavaType type) { } } - private @NotNull List getAnnotations(JavaType type) { + private List getAnnotations(JavaType type) { if (type instanceof JavaType.Class) { return ((JavaType.Class) type).getAnnotations(); } else if (type instanceof JavaType.Variable) { @@ -163,7 +162,7 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Ex return classDeclaration; } - private <@Nullable T extends J> J getElement(ExecutionContext ctx, List leadingAnnotations, T target) { + private J getElement(ExecutionContext ctx, List leadingAnnotations, T target) { Optional annotation = leadingAnnotations.stream() .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) .findFirst(); From 47b9f7d96f252cde5d2247fc558484ff10945cd9 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 2 Apr 2025 19:29:56 +0200 Subject: [PATCH 06/20] Removed unused import --- .../RemoveVisibleForTestingAnnotationWhenUsedInProduction.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index e39bde00e..387d782ed 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -15,7 +15,6 @@ */ package org.openrewrite.java.testing.cleanup; -import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.ScanningRecipe; import org.openrewrite.TreeVisitor; From 918489b8a982023a955a0d1106ca714aa6fe21ea Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Thu, 3 Apr 2025 11:45:52 +0200 Subject: [PATCH 07/20] Review, TestCases generics and method references, Fix method references --- ...TestingAnnotationWhenUsedInProduction.java | 75 +++--- ...ingAnnotationWhenUsedInProductionTest.java | 237 ++++++++++++++---- 2 files changed, 226 insertions(+), 86 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 387d782ed..60d3c2fb3 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -25,10 +25,7 @@ import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Optional; +import java.util.*; public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { @@ -45,7 +42,8 @@ public String getDisplayName() { @Override public String getDescription() { - return "The `@VisibleForTesting` annotation is used when a method or a field has been made more accessible then it would normally be, solely for testing purposes. " + + return "The `@VisibleForTesting` annotation marks a method or field that is intentionally made more accessible (e.g., changing its visibility from private or package-private to public or protected) solely for testing purposes." + + "The annotation serves as an indicator that the increased visibility is not part of the intended public API but exists only to support testability. " + "This recipe removes the annotation where such an element is used from production classes. It identifies production classes as classes in `src/main` and test classes as classes in `src/test`. " + "It will remove the `@VisibleForTesting` from methods, fields (both member fields and constants), constructors and inner classes. " + "This recipe should not be used in an environment where QA tooling acts on the `@VisibleForTesting` annotation."; @@ -61,50 +59,51 @@ public TreeVisitor getScanner(Scanned acc) { return new JavaIsoVisitor() { @Override - public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { - // Mark classes - if (fieldAccess.getTarget().getType() instanceof JavaType.Class) { - checkAndRegister(acc.classes, fieldAccess.getTarget().getType()); + public J.FieldAccess visitFieldAccess(J.FieldAccess fa, ExecutionContext ctx) { + if (fa.getTarget().getType() instanceof JavaType.Class) { + checkAndRegister(acc.classes, fa.getTarget().getType()); } - // Mark fields - if (fieldAccess.getName().getFieldType() != null) { - checkAndRegister(acc.fields, fieldAccess.getName().getFieldType()); + if (fa.getName().getFieldType() != null) { + checkAndRegister(acc.fields, fa.getName().getFieldType()); } - return super.visitFieldAccess(fieldAccess, ctx); + return super.visitFieldAccess(fa, ctx); } @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { - if (method.getMethodType() != null) { - checkAndRegister(acc.methods, method.getMethodType()); + public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionContext executionContext) { + if (mr.getMethodType() != null) { + checkAndRegister(acc.methods, mr.getMethodType()); } - return super.visitMethodInvocation(method, ctx); + return super.visitMemberReference(mr, executionContext); } @Override - public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext ctx) { - // Mark constructors - if (newClass.getConstructorType() != null) { - checkAndRegister(acc.methods, newClass.getConstructorType()); + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx) { + if (mi.getMethodType() != null) { + checkAndRegister(acc.methods, mi.getMethodType()); } - // Mark classes - if (newClass.getClazz() != null && newClass.getClazz().getType() != null && newClass.getClazz().getType() instanceof JavaType.Class) { - checkAndRegister(acc.classes, newClass.getClazz().getType()); + return super.visitMethodInvocation(mi, ctx); + } + + @Override + public J.NewClass visitNewClass(J.NewClass nc, ExecutionContext ctx) { + if (nc.getConstructorType() != null) { + checkAndRegister(acc.methods, nc.getConstructorType()); + } + if (nc.getClazz() != null && nc.getClazz().getType() instanceof JavaType.Class) { + checkAndRegister(acc.classes, nc.getClazz().getType()); } - return super.visitNewClass(newClass, ctx); + return super.visitNewClass(nc, ctx); } private void checkAndRegister(List target, JavaType type) { if (!target.contains(TypeUtils.toString(type))) { getAnnotations(type).forEach(annotation -> { if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit compilationUnit = getCursor().firstEnclosing(J.CompilationUnit.class); - if (compilationUnit != null) { - compilationUnit - .getMarkers() - .findFirst(JavaSourceSet.class) - .filter(elem -> "main".equals(elem.getName())) - .ifPresent(sourceSet -> target.add(TypeUtils.toString(type))); + J.CompilationUnit cu = getCursor().firstEnclosing(J.CompilationUnit.class); + if (cu != null && + cu.getMarkers().findFirst(JavaSourceSet.class).filter(s -> "main".equals(s.getName().toLowerCase(Locale.ROOT))).isPresent()){ + target.add(TypeUtils.toString(type)); } } }); @@ -129,8 +128,8 @@ public TreeVisitor getVisitor(Scanned acc) { return new JavaIsoVisitor() { @Override - public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { - J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(multiVariable, ctx); + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations vd, ExecutionContext ctx) { + J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(vd, ctx); if (!variableDeclarations.getVariables().isEmpty()) { // if none of the variables in the declaration are used from production code, the annotation should be kept boolean keepAnnotation = variableDeclarations.getVariables().stream() @@ -144,8 +143,8 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m } @Override - public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { - J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(method, ctx); + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { + J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(md, ctx); if (methodDeclaration.getMethodType() != null && acc.methods.contains(TypeUtils.toString(methodDeclaration.getMethodType()))) { return (J.MethodDeclaration) getElement(ctx, methodDeclaration.getLeadingAnnotations(), methodDeclaration); } @@ -153,8 +152,8 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex } @Override - public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { - J.ClassDeclaration classDeclaration = super.visitClassDeclaration(classDecl, ctx); + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration cd, ExecutionContext ctx) { + J.ClassDeclaration classDeclaration = super.visitClassDeclaration(cd, ctx); if (classDeclaration.getType() != null && acc.classes.contains(TypeUtils.toString(classDeclaration.getType()))) { return (J.ClassDeclaration) getElement(ctx, classDeclaration.getLeadingAnnotations(), classDeclaration); } diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index 507a11628..d4b007770 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -118,6 +118,69 @@ void test(Production production) { ); } + @Test + void constructor() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public Production(){} + @VisibleForTesting + public Production(int debugLevel) {} + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + public Production(){} + @VisibleForTesting + public Production(int debugLevel) {} + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call() { + new Production(); + } + } + """ + ) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + new Production(); + new Production(1); + } + } + """ + ) + ) + ); + } + @Test void fieldAccess() { //language=java @@ -253,6 +316,69 @@ void test(Production production) { ); } + @Test + void referenceConstant() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static final long INTERNAL_CONSTANT = 1L; + @VisibleForTesting + public static final long EXTERNAL_CONSTANT = 2L; + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + @VisibleForTesting + public static final long INTERNAL_CONSTANT = 1L; + public static final long EXTERNAL_CONSTANT = 2L; + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + + class ProductionCaller { + void call(Production production) { + long variableOne = Production.EXTERNAL_CONSTANT; + } + } + """ + ) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + + class ProductionTest { + void test(Production production) { + long variableOne = Production.INTERNAL_CONSTANT; + long variableTwo = Production.EXTERNAL_CONSTANT; + } + } + """ + ) + ) + ); + } + @Test void methodInvocation() { //language=java @@ -316,8 +442,8 @@ void test(Production production) { ); } - @Test - void constructor() { + + void genericMethodInvocation() { //language=java rewriteRun( srcMainJava( @@ -329,9 +455,9 @@ void constructor() { public class Production { @VisibleForTesting - public Production(){} + public void internalCall(T param){} @VisibleForTesting - public Production(int debugLevel) {} + public void externalCall(T param){} } """, """ @@ -340,9 +466,9 @@ public Production(int debugLevel) {} import org.jetbrains.annotations.VisibleForTesting; public class Production { - public Production(){} @VisibleForTesting - public Production(int debugLevel) {} + public void internalCall(T param){} + public void externalCall(T param){} } """ ), @@ -353,8 +479,8 @@ public Production(int debugLevel) {} import com.example.domain.Production; class ProductionCaller { - void call() { - new Production(); + void call(Production production, Object arg) { + production.externalCall(arg); } } """ @@ -368,9 +494,9 @@ void call() { import com.example.domain.Production; class ProductionTest { - void test(Production production) { - new Production(); - new Production(1); + void test(Production production, Object arg) { + production.internalCall(arg); + production.externalCall(arg); } } """ @@ -380,7 +506,7 @@ void test(Production production) { } @Test - void referenceStaticInnerClass() { + void methodReference() { //language=java rewriteRun( srcMainJava( @@ -392,16 +518,13 @@ void referenceStaticInnerClass() { public class Production { @VisibleForTesting - public static class InternalInner { - @VisibleForTesting - public static String internalState; - } - + public String internalMethodReferenceWithInvocation(){} @VisibleForTesting - public static class ExternalInner { - @VisibleForTesting - public static String externalState; - } + public String internalMethodReferenceWithoutInvocation(){} + @VisibleForTesting + public String externalMethodReferenceWithInvocation(){} + @VisibleForTesting + public String externalMethodReferenceWithoutInvocation(){} } """, """ @@ -411,14 +534,11 @@ public static class ExternalInner { public class Production { @VisibleForTesting - public static class InternalInner { - @VisibleForTesting - public static String internalState; - } - - public static class ExternalInner { - public static String externalState; - } + public String internalMethodReferenceWithInvocation(){} + @VisibleForTesting + public String internalMethodReferenceWithoutInvocation(){} + public String externalMethodReferenceWithInvocation(){} + public String externalMethodReferenceWithoutInvocation(){} } """ ), @@ -427,10 +547,13 @@ public static class ExternalInner { package com.example.caller; import com.example.domain.Production; + import java.util.function.Supplier; class ProductionCaller { void call(Production production) { - String variableOne = Production.ExternalInner.externalState; + Supplier supplierOne = production::externalMethodReferenceWithInvocation; + Supplier supplierTwo = production::externalMethodReferenceWithoutInvocation; + String supplierOneResult = supplierOne.get(); } } """ @@ -442,11 +565,16 @@ void call(Production production) { package com.example.test; import com.example.domain.Production; + import java.util.function.Supplier; class ProductionTest { void test(Production production) { - String variableOne = Production.ExternalInner.externalState; - String variableTwo = Production.InternalInner.internalState; + Supplier supplierOne = production::externalMethodReferenceWithInvocation; + Supplier supplierTwo = production::externalMethodReferenceWithoutInvocation; + String supplierOneResult = supplierOne.get(); + Supplier supplierThree = production::internalMethodReferenceWithInvocation; + Supplier supplierFour = production::internalMethodReferenceWithoutInvocation; + String supplierThreeResult = supplierThree.get(); } } """ @@ -456,7 +584,7 @@ void test(Production production) { } @Test - void instantiateStaticInnerClass() { + void referenceStaticInnerClass() { //language=java rewriteRun( srcMainJava( @@ -468,10 +596,16 @@ void instantiateStaticInnerClass() { public class Production { @VisibleForTesting - public static class InternalInner {} + public static class InternalInner { + @VisibleForTesting + public static String internalState; + } @VisibleForTesting - public static class ExternalInner {} + public static class ExternalInner { + @VisibleForTesting + public static String externalState; + } } """, """ @@ -481,9 +615,14 @@ public static class ExternalInner {} public class Production { @VisibleForTesting - public static class InternalInner {} + public static class InternalInner { + @VisibleForTesting + public static String internalState; + } - public static class ExternalInner {} + public static class ExternalInner { + public static String externalState; + } } """ ), @@ -495,7 +634,7 @@ public static class ExternalInner {} class ProductionCaller { void call(Production production) { - new Production.ExternalInner(); + String variableOne = Production.ExternalInner.externalState; } } """ @@ -510,8 +649,8 @@ void call(Production production) { class ProductionTest { void test(Production production) { - new Production.InternalInner(); - new Production.ExternalInner(); + String variableOne = Production.ExternalInner.externalState; + String variableTwo = Production.InternalInner.internalState; } } """ @@ -521,7 +660,7 @@ void test(Production production) { } @Test - void referenceConstant() { + void instantiateStaticInnerClass() { //language=java rewriteRun( srcMainJava( @@ -533,9 +672,10 @@ void referenceConstant() { public class Production { @VisibleForTesting - public static final long INTERNAL_CONSTANT = 1L; + public static class InternalInner {} + @VisibleForTesting - public static final long EXTERNAL_CONSTANT = 2L; + public static class ExternalInner {} } """, """ @@ -545,8 +685,9 @@ public class Production { public class Production { @VisibleForTesting - public static final long INTERNAL_CONSTANT = 1L; - public static final long EXTERNAL_CONSTANT = 2L; + public static class InternalInner {} + + public static class ExternalInner {} } """ ), @@ -558,7 +699,7 @@ public class Production { class ProductionCaller { void call(Production production) { - long variableOne = Production.EXTERNAL_CONSTANT; + new Production.ExternalInner(); } } """ @@ -573,8 +714,8 @@ void call(Production production) { class ProductionTest { void test(Production production) { - long variableOne = Production.INTERNAL_CONSTANT; - long variableTwo = Production.EXTERNAL_CONSTANT; + new Production.InternalInner(); + new Production.ExternalInner(); } } """ From 7eac32136190d5a80e4f39629aea53c23388397d Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Thu, 3 Apr 2025 11:50:03 +0200 Subject: [PATCH 08/20] rename to ctx --- ...RemoveVisibleForTestingAnnotationWhenUsedInProduction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 60d3c2fb3..6ab93ab30 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -70,11 +70,11 @@ public J.FieldAccess visitFieldAccess(J.FieldAccess fa, ExecutionContext ctx) { } @Override - public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionContext executionContext) { + public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionContext ctx) { if (mr.getMethodType() != null) { checkAndRegister(acc.methods, mr.getMethodType()); } - return super.visitMemberReference(mr, executionContext); + return super.visitMemberReference(mr, ctx); } @Override From 68ac4548d3b82748479d602df77ab75da218d153 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Thu, 3 Apr 2025 15:44:30 +0200 Subject: [PATCH 09/20] Added generic test --- ...ingAnnotationWhenUsedInProductionTest.java | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index d4b007770..f17d8de21 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -442,8 +442,8 @@ void test(Production production) { ); } - - void genericMethodInvocation() { + @Test + void methodInvocationWithParametersWithGenericTypeAndReturnTypeWithGenericType() { //language=java rewriteRun( srcMainJava( @@ -452,23 +452,41 @@ void genericMethodInvocation() { package com.example.domain; import org.jetbrains.annotations.VisibleForTesting; + import java.util.List; + import java.util.ArrayList; public class Production { @VisibleForTesting - public void internalCall(T param){} + public List internalCallWithParamAndReturnType(List list){ return list; } + @VisibleForTesting + public void internalCallWithParam(List list){} + @VisibleForTesting + public List internalCallWithReturnType(){ return new ArrayList<>(); } @VisibleForTesting - public void externalCall(T param){} + public List externalCallWithParamAndReturnType(List list){ return list; } + @VisibleForTesting + public void externalCallWithParam(List list){} + @VisibleForTesting + public List externalCallWithReturnType(){ return new ArrayList<>(); } } """, """ package com.example.domain; import org.jetbrains.annotations.VisibleForTesting; + import java.util.List; + import java.util.ArrayList; public class Production { @VisibleForTesting - public void internalCall(T param){} - public void externalCall(T param){} + public List internalCallWithParamAndReturnType(List list){ return list; } + @VisibleForTesting + public void internalCallWithParam(List list){} + @VisibleForTesting + public List internalCallWithReturnType(){ return new ArrayList<>(); } + public List externalCallWithParamAndReturnType(List list){ return list; } + public void externalCallWithParam(List list){} + public List externalCallWithReturnType(){ return new ArrayList<>(); } } """ ), @@ -477,10 +495,13 @@ public void externalCall(T param){} package com.example.caller; import com.example.domain.Production; + import java.util.List; class ProductionCaller { - void call(Production production, Object arg) { - production.externalCall(arg); + void call(Production production, List list) { + List listOne = production.externalCallWithParamAndReturnType(list); + production.externalCallWithParam(list); + List listTwo = production.externalCallWithReturnType(); } } """ @@ -492,11 +513,16 @@ void call(Production production, Object arg) { package com.example.test; import com.example.domain.Production; + import java.util.List; class ProductionTest { - void test(Production production, Object arg) { - production.internalCall(arg); - production.externalCall(arg); + void test(Production production, List list) { + List listOne = production.externalCallWithParamAndReturnType(list); + production.externalCallWithParam(list); + List listTwo = production.externalCallWithReturnType(); + List listThree = production.internalCallWithParamAndReturnType(list); + production.internalCallWithParam(list); + List listFour = production.internalCallWithReturnType(); } } """ From 83a021eb21466da671b83fbd48715b7116387166 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Fri, 4 Apr 2025 09:33:16 +0200 Subject: [PATCH 10/20] Added tests proving scope of support for generics, added scope to description --- ...TestingAnnotationWhenUsedInProduction.java | 3 +- ...ingAnnotationWhenUsedInProductionTest.java | 125 +++++++++++++++++- 2 files changed, 125 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 6ab93ab30..7a383a1af 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -42,10 +42,11 @@ public String getDisplayName() { @Override public String getDescription() { - return "The `@VisibleForTesting` annotation marks a method or field that is intentionally made more accessible (e.g., changing its visibility from private or package-private to public or protected) solely for testing purposes." + + return "The `@VisibleForTesting` annotation marks a method or field that is intentionally made more accessible (e.g., changing its visibility from private or package-private to public or protected) solely for testing purposes. " + "The annotation serves as an indicator that the increased visibility is not part of the intended public API but exists only to support testability. " + "This recipe removes the annotation where such an element is used from production classes. It identifies production classes as classes in `src/main` and test classes as classes in `src/test`. " + "It will remove the `@VisibleForTesting` from methods, fields (both member fields and constants), constructors and inner classes. " + + "It does not support generic methods (e.g. ` T method(T);`. " + "This recipe should not be used in an environment where QA tooling acts on the `@VisibleForTesting` annotation."; } diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index f17d8de21..f5cd9e8e9 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -15,6 +15,7 @@ */ package org.openrewrite.java.testing.cleanup; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; import org.openrewrite.test.RecipeSpec; @@ -463,11 +464,15 @@ public void internalCallWithParam(List list){} @VisibleForTesting public List internalCallWithReturnType(){ return new ArrayList<>(); } @VisibleForTesting + public List> internalCallWithGenericType(List> l) { return new ArrayList<>(l); } + @VisibleForTesting public List externalCallWithParamAndReturnType(List list){ return list; } @VisibleForTesting public void externalCallWithParam(List list){} @VisibleForTesting public List externalCallWithReturnType(){ return new ArrayList<>(); } + @VisibleForTesting + public List> externalCallWithGenericType(List> l) { return new ArrayList<>(l); } } """, """ @@ -484,9 +489,12 @@ public class Production { public void internalCallWithParam(List list){} @VisibleForTesting public List internalCallWithReturnType(){ return new ArrayList<>(); } + @VisibleForTesting + public List> internalCallWithGenericType(List> l) { return new ArrayList<>(l); } public List externalCallWithParamAndReturnType(List list){ return list; } public void externalCallWithParam(List list){} public List externalCallWithReturnType(){ return new ArrayList<>(); } + public List> externalCallWithGenericType(List> l) { return new ArrayList<>(l); } } """ ), @@ -496,12 +504,14 @@ public void externalCallWithParam(List list){} import com.example.domain.Production; import java.util.List; + import java.util.ArrayList; class ProductionCaller { void call(Production production, List list) { List listOne = production.externalCallWithParamAndReturnType(list); production.externalCallWithParam(list); List listTwo = production.externalCallWithReturnType(); + List> listThree = production.externalCallWithGenericType(new ArrayList>(new ArrayList<>())); } } """ @@ -514,15 +524,126 @@ void call(Production production, List list) { import com.example.domain.Production; import java.util.List; + import java.util.ArrayList; class ProductionTest { void test(Production production, List list) { List listOne = production.externalCallWithParamAndReturnType(list); production.externalCallWithParam(list); List listTwo = production.externalCallWithReturnType(); - List listThree = production.internalCallWithParamAndReturnType(list); + List> listThree = production.externalCallWithGenericType(new ArrayList>(new ArrayList<>())); + List listFour = production.internalCallWithParamAndReturnType(list); production.internalCallWithParam(list); - List listFour = production.internalCallWithReturnType(); + List listFive = production.internalCallWithReturnType(); + List> listSix = production.internalCallWithGenericType(new ArrayList>(new ArrayList<>())); + } + } + """ + ) + ) + ); + } + + @Test + @Disabled("implement when engine supports generic method declaration reference from invocation") + void genericMethodInvocation() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + import java.util.List; + import java.util.ArrayList; + import java.util.Map; + + public class Production { + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void externalCall(T param){} + @VisibleForTesting + public void externalCall(T param){} + @VisibleForTesting + public void externalCall(T param){} + @VisibleForTesting + public void externalCall(T param){} + } + """, + """ + package com.example.domain; + + import org.jetbrains.annotations.VisibleForTesting; + import java.util.List; + import java.util.ArrayList; + import java.util.Map; + + public class Production { + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void internalCall(T param){} + @VisibleForTesting + public void internalCall(T param){} + public void externalCall(T param){} + public void externalCall(T param){} + public void externalCall(T param){} + public void externalCall(T param){} + } + """ + ), + java( + """ + package com.example.caller; + + import com.example.domain.Production; + import java.util.List; + import java.util.ArrayList; + import java.util.LinkedList; + import java.util.HashMap; + + class ProductionCaller { + void call(Production production, Object arg, List list, ArrayList list2, HashMap map) { + production.externalCall(arg); + production.externalCall(list); + production.externalCall(list2); + production.externalCall(map); + } + } + """ + ) + ), + srcTestJava( + java( + """ + package com.example.test; + + import com.example.domain.Production; + import java.util.List; + import java.util.ArrayList; + import java.util.LinkedList; + import java.util.HashMap; + + class ProductionTest { + void test(Production production, Object arg, List list, ArrayList list2, HashMap map) { + production.internalCall(arg); + production.internalCall(list); + production.internalCall(list2); + production.internalCall(map); + production.externalCall(arg); + production.externalCall(list); + production.externalCall(list2); + production.externalCall(map); } } """ From ab231a01773a8dff970ad65d36439e9d6c87baf1 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Fri, 4 Apr 2025 13:23:30 +0200 Subject: [PATCH 11/20] Review --- ...TestingAnnotationWhenUsedInProduction.java | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 7a383a1af..d13eb7d8d 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -16,23 +16,26 @@ package org.openrewrite.java.testing.cleanup; import org.openrewrite.ExecutionContext; +import org.openrewrite.Preconditions; import org.openrewrite.ScanningRecipe; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.RemoveAnnotation; -import org.openrewrite.java.marker.JavaSourceSet; +import org.openrewrite.java.search.IsLikelyNotTest; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; import java.util.*; -public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { +import static org.openrewrite.Preconditions.and; - public static class Scanned { - List methods = new ArrayList<>(); - List fields = new ArrayList<>(); - List classes = new ArrayList<>(); +public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { + + public static class VisibleForTesting { + Set methodPatterns = new HashSet<>(); + Set fieldPatterns = new HashSet<>(); + Set classPatterns = new HashSet<>(); } @Override @@ -51,21 +54,21 @@ public String getDescription() { } @Override - public Scanned getInitialValue(ExecutionContext ctx) { - return new Scanned(); + public VisibleForTesting getInitialValue(ExecutionContext ctx) { + return new VisibleForTesting(); } @Override - public TreeVisitor getScanner(Scanned acc) { - return new JavaIsoVisitor() { + public TreeVisitor getScanner(VisibleForTesting acc) { + JavaIsoVisitor scanningVisitor = new JavaIsoVisitor() { @Override public J.FieldAccess visitFieldAccess(J.FieldAccess fa, ExecutionContext ctx) { if (fa.getTarget().getType() instanceof JavaType.Class) { - checkAndRegister(acc.classes, fa.getTarget().getType()); + checkAndRegister(acc.classPatterns, fa.getTarget().getType()); } if (fa.getName().getFieldType() != null) { - checkAndRegister(acc.fields, fa.getName().getFieldType()); + checkAndRegister(acc.fieldPatterns, fa.getName().getFieldType()); } return super.visitFieldAccess(fa, ctx); } @@ -73,7 +76,7 @@ public J.FieldAccess visitFieldAccess(J.FieldAccess fa, ExecutionContext ctx) { @Override public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionContext ctx) { if (mr.getMethodType() != null) { - checkAndRegister(acc.methods, mr.getMethodType()); + checkAndRegister(acc.methodPatterns, mr.getMethodType()); } return super.visitMemberReference(mr, ctx); } @@ -81,7 +84,7 @@ public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionCon @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx) { if (mi.getMethodType() != null) { - checkAndRegister(acc.methods, mi.getMethodType()); + checkAndRegister(acc.methodPatterns, mi.getMethodType()); } return super.visitMethodInvocation(mi, ctx); } @@ -89,23 +92,19 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Execution @Override public J.NewClass visitNewClass(J.NewClass nc, ExecutionContext ctx) { if (nc.getConstructorType() != null) { - checkAndRegister(acc.methods, nc.getConstructorType()); + checkAndRegister(acc.methodPatterns, nc.getConstructorType()); } if (nc.getClazz() != null && nc.getClazz().getType() instanceof JavaType.Class) { - checkAndRegister(acc.classes, nc.getClazz().getType()); + checkAndRegister(acc.classPatterns, nc.getClazz().getType()); } return super.visitNewClass(nc, ctx); } - private void checkAndRegister(List target, JavaType type) { + private void checkAndRegister(Set target, JavaType type) { if (!target.contains(TypeUtils.toString(type))) { getAnnotations(type).forEach(annotation -> { if ("VisibleForTesting".equals(annotation.getClassName())) { - J.CompilationUnit cu = getCursor().firstEnclosing(J.CompilationUnit.class); - if (cu != null && - cu.getMarkers().findFirst(JavaSourceSet.class).filter(s -> "main".equals(s.getName().toLowerCase(Locale.ROOT))).isPresent()){ - target.add(TypeUtils.toString(type)); - } + target.add(TypeUtils.toString(type)); } }); } @@ -122,11 +121,12 @@ private List getAnnotations(JavaType type) { return Collections.emptyList(); } }; + return Preconditions.check(new IsLikelyNotTest().getVisitor(), scanningVisitor); } @Override - public TreeVisitor getVisitor(Scanned acc) { - return new JavaIsoVisitor() { + public TreeVisitor getVisitor(VisibleForTesting acc) { + JavaIsoVisitor visitor = new JavaIsoVisitor() { @Override public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations vd, ExecutionContext ctx) { @@ -134,8 +134,8 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations v if (!variableDeclarations.getVariables().isEmpty()) { // if none of the variables in the declaration are used from production code, the annotation should be kept boolean keepAnnotation = variableDeclarations.getVariables().stream() - .filter(elem -> elem.getVariableType() != null) - .noneMatch(elem -> acc.fields.contains(TypeUtils.toString(elem.getVariableType()))); + .filter(elem -> elem.getVariableType() != null) + .noneMatch(elem -> acc.fieldPatterns.contains(TypeUtils.toString(elem.getVariableType()))); if (!keepAnnotation) { return (J.VariableDeclarations) getElement(ctx, variableDeclarations.getLeadingAnnotations(), variableDeclarations); } @@ -146,7 +146,7 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations v @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(md, ctx); - if (methodDeclaration.getMethodType() != null && acc.methods.contains(TypeUtils.toString(methodDeclaration.getMethodType()))) { + if (methodDeclaration.getMethodType() != null && acc.methodPatterns.contains(TypeUtils.toString(methodDeclaration.getMethodType()))) { return (J.MethodDeclaration) getElement(ctx, methodDeclaration.getLeadingAnnotations(), methodDeclaration); } return methodDeclaration; @@ -155,7 +155,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, Execut @Override public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration cd, ExecutionContext ctx) { J.ClassDeclaration classDeclaration = super.visitClassDeclaration(cd, ctx); - if (classDeclaration.getType() != null && acc.classes.contains(TypeUtils.toString(classDeclaration.getType()))) { + if (classDeclaration.getType() != null && acc.classPatterns.contains(TypeUtils.toString(classDeclaration.getType()))) { return (J.ClassDeclaration) getElement(ctx, classDeclaration.getLeadingAnnotations(), classDeclaration); } return classDeclaration; @@ -163,8 +163,8 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration cd, Execution private J getElement(ExecutionContext ctx, List leadingAnnotations, T target) { Optional annotation = leadingAnnotations.stream() - .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) - .findFirst(); + .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) + .findFirst(); if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { JavaType.Class type = (JavaType.Class) annotation.get().getType(); return new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(target, ctx, getCursor().getParentOrThrow()); @@ -172,5 +172,7 @@ private J getElement(ExecutionContext ctx, List lead return target; } }; + + return Preconditions.check(new IsLikelyNotTest().getVisitor(), visitor); } } From 0005511c8c6be59e2de91f1989d62dc2c6fe5bfa Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Fri, 4 Apr 2025 13:26:16 +0200 Subject: [PATCH 12/20] remove newline --- .../RemoveVisibleForTestingAnnotationWhenUsedInProduction.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index d13eb7d8d..a6ae79c30 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -172,7 +172,6 @@ private J getElement(ExecutionContext ctx, List lead return target; } }; - return Preconditions.check(new IsLikelyNotTest().getVisitor(), visitor); } } From 8e076d86f81aa57da7a5bf3c66be84ad02edf758 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Fri, 4 Apr 2025 13:27:05 +0200 Subject: [PATCH 13/20] removed import --- .../RemoveVisibleForTestingAnnotationWhenUsedInProduction.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index a6ae79c30..6ea7f19de 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -28,8 +28,6 @@ import java.util.*; -import static org.openrewrite.Preconditions.and; - public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { public static class VisibleForTesting { From f63c13b81f0cfba980dbbcb25a615bbaa7d3a9fa Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 4 Apr 2025 14:44:45 +0200 Subject: [PATCH 14/20] Remove `forEach` and `stream()` to avoid object allocations --- ...TestingAnnotationWhenUsedInProduction.java | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 6ea7f19de..ac5e61595 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -26,7 +26,10 @@ import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; -import java.util.*; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; public class RemoveVisibleForTestingAnnotationWhenUsedInProduction extends ScanningRecipe { @@ -99,12 +102,13 @@ public J.NewClass visitNewClass(J.NewClass nc, ExecutionContext ctx) { } private void checkAndRegister(Set target, JavaType type) { - if (!target.contains(TypeUtils.toString(type))) { - getAnnotations(type).forEach(annotation -> { + String typeString = TypeUtils.toString(type); + if (!target.contains(typeString)) { + for (JavaType.FullyQualified annotation : getAnnotations(type)) { if ("VisibleForTesting".equals(annotation.getClassName())) { - target.add(TypeUtils.toString(type)); + target.add(typeString); } - }); + } } } @@ -131,11 +135,13 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations v J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(vd, ctx); if (!variableDeclarations.getVariables().isEmpty()) { // if none of the variables in the declaration are used from production code, the annotation should be kept - boolean keepAnnotation = variableDeclarations.getVariables().stream() - .filter(elem -> elem.getVariableType() != null) - .noneMatch(elem -> acc.fieldPatterns.contains(TypeUtils.toString(elem.getVariableType()))); - if (!keepAnnotation) { - return (J.VariableDeclarations) getElement(ctx, variableDeclarations.getLeadingAnnotations(), variableDeclarations); + for (J.VariableDeclarations.NamedVariable elem : variableDeclarations.getVariables()) { + if (elem.getVariableType() != null) { + if (acc.fieldPatterns.contains(TypeUtils.toString(elem.getVariableType()))) { + return (J.VariableDeclarations) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() + .visitNonNull(variableDeclarations, ctx, getCursor().getParentOrThrow()); + } + } } } return variableDeclarations; @@ -145,7 +151,8 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations v public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(md, ctx); if (methodDeclaration.getMethodType() != null && acc.methodPatterns.contains(TypeUtils.toString(methodDeclaration.getMethodType()))) { - return (J.MethodDeclaration) getElement(ctx, methodDeclaration.getLeadingAnnotations(), methodDeclaration); + return (J.MethodDeclaration) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() + .visitNonNull(methodDeclaration, ctx, getCursor().getParentOrThrow()); } return methodDeclaration; } @@ -154,21 +161,11 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, Execut public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration cd, ExecutionContext ctx) { J.ClassDeclaration classDeclaration = super.visitClassDeclaration(cd, ctx); if (classDeclaration.getType() != null && acc.classPatterns.contains(TypeUtils.toString(classDeclaration.getType()))) { - return (J.ClassDeclaration) getElement(ctx, classDeclaration.getLeadingAnnotations(), classDeclaration); + return (J.ClassDeclaration) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() + .visitNonNull(classDeclaration, ctx, getCursor().getParentOrThrow()); } return classDeclaration; } - - private J getElement(ExecutionContext ctx, List leadingAnnotations, T target) { - Optional annotation = leadingAnnotations.stream() - .filter(elem -> "VisibleForTesting".equals(elem.getSimpleName())) - .findFirst(); - if (annotation.isPresent() && annotation.get().getType() instanceof JavaType.Class) { - JavaType.Class type = (JavaType.Class) annotation.get().getType(); - return new RemoveAnnotation("@" + type.getFullyQualifiedName()).getVisitor().visitNonNull(target, ctx, getCursor().getParentOrThrow()); - } - return target; - } }; return Preconditions.check(new IsLikelyNotTest().getVisitor(), visitor); } From bd2d2bfa980b03cf810cf10fee64bbfb6ca18008 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 4 Apr 2025 14:47:58 +0200 Subject: [PATCH 15/20] Add UsesType precondition on `getVisitor` --- ...oveVisibleForTestingAnnotationWhenUsedInProduction.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index ac5e61595..846255203 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -22,6 +22,7 @@ import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.RemoveAnnotation; import org.openrewrite.java.search.IsLikelyNotTest; +import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; @@ -167,6 +168,10 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration cd, Execution return classDeclaration; } }; - return Preconditions.check(new IsLikelyNotTest().getVisitor(), visitor); + return Preconditions.check( + Preconditions.and( + new IsLikelyNotTest().getVisitor(), + new UsesType<>("*..VisibleForTesting", true)), + visitor); } } From 6973da12f81c7b86ee9c6efdd43a78c7a9547cb7 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 4 Apr 2025 15:06:02 +0200 Subject: [PATCH 16/20] Verify that we remove the import when last annotation removed --- ...ingAnnotationWhenUsedInProductionTest.java | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index f5cd9e8e9..68d5910bc 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -27,8 +27,7 @@ class RemoveVisibleForTestingAnnotationWhenUsedInProductionTest implements Rewri @Override public void defaults(RecipeSpec spec) { - spec - .recipe(new RemoveVisibleForTestingAnnotationWhenUsedInProduction()); + spec.recipe(new RemoveVisibleForTestingAnnotationWhenUsedInProduction()); } @DocumentExample @@ -119,6 +118,45 @@ void test(Production production) { ); } + @Test + void removeImport() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + + @VisibleForTesting + public String getExternalState() { + return "foo"; + } + } + """, + """ + public class Production { + + public String getExternalState() { + return "foo"; + } + } + """ + ), + java( + """ + class ProductionCaller { + void call(Production production) { + String variableTwo = production.getExternalState(); + } + } + """ + ) + ) + ); + } + @Test void constructor() { //language=java From 7832d931ed12f856e478b85418ec8a81a8ef9141 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Mon, 7 Apr 2025 09:00:43 +0200 Subject: [PATCH 17/20] Added maybeRemoveImport --- ...TestingAnnotationWhenUsedInProduction.java | 71 +++++++++++-------- ...ingAnnotationWhenUsedInProductionTest.java | 35 ++++++++- 2 files changed, 76 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index 846255203..a12ba5fa9 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -38,6 +38,7 @@ public static class VisibleForTesting { Set methodPatterns = new HashSet<>(); Set fieldPatterns = new HashSet<>(); Set classPatterns = new HashSet<>(); + Set fullyQualifiedVisibleForTestingAnnotationPatterns = new HashSet<>(); } @Override @@ -48,11 +49,11 @@ public String getDisplayName() { @Override public String getDescription() { return "The `@VisibleForTesting` annotation marks a method or field that is intentionally made more accessible (e.g., changing its visibility from private or package-private to public or protected) solely for testing purposes. " + - "The annotation serves as an indicator that the increased visibility is not part of the intended public API but exists only to support testability. " + - "This recipe removes the annotation where such an element is used from production classes. It identifies production classes as classes in `src/main` and test classes as classes in `src/test`. " + - "It will remove the `@VisibleForTesting` from methods, fields (both member fields and constants), constructors and inner classes. " + - "It does not support generic methods (e.g. ` T method(T);`. " + - "This recipe should not be used in an environment where QA tooling acts on the `@VisibleForTesting` annotation."; + "The annotation serves as an indicator that the increased visibility is not part of the intended public API but exists only to support testability. " + + "This recipe removes the annotation where such an element is used from production classes. It identifies production classes as classes in `src/main` and test classes as classes in `src/test`. " + + "It will remove the `@VisibleForTesting` from methods, fields (both member fields and constants), constructors and inner classes. " + + "It does not support generic methods (e.g. ` T method(T);`. " + + "This recipe should not be used in an environment where QA tooling acts on the `@VisibleForTesting` annotation."; } @Override @@ -108,6 +109,7 @@ private void checkAndRegister(Set target, JavaType type) { for (JavaType.FullyQualified annotation : getAnnotations(type)) { if ("VisibleForTesting".equals(annotation.getClassName())) { target.add(typeString); + acc.fullyQualifiedVisibleForTestingAnnotationPatterns.add(annotation.getFullyQualifiedName()); } } } @@ -130,48 +132,59 @@ private List getAnnotations(JavaType type) { @Override public TreeVisitor getVisitor(VisibleForTesting acc) { JavaIsoVisitor visitor = new JavaIsoVisitor() { - @Override - public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations vd, ExecutionContext ctx) { - J.VariableDeclarations variableDeclarations = super.visitVariableDeclarations(vd, ctx); - if (!variableDeclarations.getVariables().isEmpty()) { + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations variableDeclarations, ExecutionContext ctx) { + J.VariableDeclarations vd = super.visitVariableDeclarations(variableDeclarations, ctx); + if (!vd.getVariables().isEmpty()) { // if none of the variables in the declaration are used from production code, the annotation should be kept - for (J.VariableDeclarations.NamedVariable elem : variableDeclarations.getVariables()) { + for (J.VariableDeclarations.NamedVariable elem : vd.getVariables()) { if (elem.getVariableType() != null) { if (acc.fieldPatterns.contains(TypeUtils.toString(elem.getVariableType()))) { - return (J.VariableDeclarations) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() - .visitNonNull(variableDeclarations, ctx, getCursor().getParentOrThrow()); + J.VariableDeclarations newVd = (J.VariableDeclarations) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() + .visitNonNull(vd, ctx, getCursor().getParentOrThrow()); + if (vd != newVd) { + acc.fullyQualifiedVisibleForTestingAnnotationPatterns.forEach(this::maybeRemoveImport); + } + return newVd; } } } } - return variableDeclarations; + return vd; } @Override - public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { - J.MethodDeclaration methodDeclaration = super.visitMethodDeclaration(md, ctx); - if (methodDeclaration.getMethodType() != null && acc.methodPatterns.contains(TypeUtils.toString(methodDeclaration.getMethodType()))) { - return (J.MethodDeclaration) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() - .visitNonNull(methodDeclaration, ctx, getCursor().getParentOrThrow()); + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDeclaration, ExecutionContext ctx) { + J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); + if (md.getMethodType() != null && acc.methodPatterns.contains(TypeUtils.toString(md.getMethodType()))) { + J.MethodDeclaration newMd = (J.MethodDeclaration) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() + .visitNonNull(md, ctx, getCursor().getParentOrThrow()); + if (md != newMd) { + acc.fullyQualifiedVisibleForTestingAnnotationPatterns.forEach(this::maybeRemoveImport); + } + return newMd; } - return methodDeclaration; + return md; } @Override - public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration cd, ExecutionContext ctx) { - J.ClassDeclaration classDeclaration = super.visitClassDeclaration(cd, ctx); - if (classDeclaration.getType() != null && acc.classPatterns.contains(TypeUtils.toString(classDeclaration.getType()))) { - return (J.ClassDeclaration) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() - .visitNonNull(classDeclaration, ctx, getCursor().getParentOrThrow()); + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDeclaration, ExecutionContext ctx) { + J.ClassDeclaration cd = super.visitClassDeclaration(classDeclaration, ctx); + if (cd.getType() != null && acc.classPatterns.contains(TypeUtils.toString(cd.getType()))) { + J.ClassDeclaration newCd = (J.ClassDeclaration) new RemoveAnnotation("@*..VisibleForTesting").getVisitor() + .visitNonNull(cd, ctx, getCursor().getParentOrThrow()); + if (cd != newCd) { + acc.fullyQualifiedVisibleForTestingAnnotationPatterns.forEach(this::maybeRemoveImport); + } + return newCd; } - return classDeclaration; + return cd; } }; return Preconditions.check( - Preconditions.and( - new IsLikelyNotTest().getVisitor(), - new UsesType<>("*..VisibleForTesting", true)), - visitor); + Preconditions.and( + new IsLikelyNotTest().getVisitor(), + new UsesType<>("*..VisibleForTesting", true)), + visitor); } } diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index 68d5910bc..179e66449 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -157,6 +157,39 @@ void call(Production production) { ); } + @Test + void leaveImport() { + //language=java + rewriteRun( + srcMainJava( + java( + """ + import org.jetbrains.annotations.VisibleForTesting; + + public class Production { + + @VisibleForTesting + public String getExternalState() { + return "foo"; + } + } + """ + ) + ), + srcTestJava( + java( + """ + class ProductionTest { + void test(Production production) { + production.getExternalState(); + } + } + """ + ) + ) + ); + } + @Test void constructor() { //language=java @@ -209,7 +242,7 @@ void call() { import com.example.domain.Production; class ProductionTest { - void test(Production production) { + void test() { new Production(); new Production(1); } From d0413dd7ab848ff15cd6a9308d8b06f89c2c81e6 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Mon, 7 Apr 2025 09:14:45 +0200 Subject: [PATCH 18/20] Removed wrong test --- ...ingAnnotationWhenUsedInProductionTest.java | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java index 179e66449..951010329 100644 --- a/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java +++ b/src/test/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProductionTest.java @@ -157,39 +157,6 @@ void call(Production production) { ); } - @Test - void leaveImport() { - //language=java - rewriteRun( - srcMainJava( - java( - """ - import org.jetbrains.annotations.VisibleForTesting; - - public class Production { - - @VisibleForTesting - public String getExternalState() { - return "foo"; - } - } - """ - ) - ), - srcTestJava( - java( - """ - class ProductionTest { - void test(Production production) { - production.getExternalState(); - } - } - """ - ) - ) - ); - } - @Test void constructor() { //language=java From f53a28b736d5b674eb1cf24de3cf98e10c6270ad Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 7 Apr 2025 13:26:36 +0200 Subject: [PATCH 19/20] Run `RemoveVisibleForTestingAnnotationWhenUsedInProduction` as part of cleanup.yml --- src/main/resources/META-INF/rewrite/cleanup.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/resources/META-INF/rewrite/cleanup.yml b/src/main/resources/META-INF/rewrite/cleanup.yml index a8f309c12..4890dc85d 100644 --- a/src/main/resources/META-INF/rewrite/cleanup.yml +++ b/src/main/resources/META-INF/rewrite/cleanup.yml @@ -24,3 +24,5 @@ tags: recipeList: - org.openrewrite.java.testing.cleanup.TestsShouldIncludeAssertions - org.openrewrite.java.testing.cleanup.RemoveTestPrefix + - org.openrewrite.java.testing.cleanup.RemoveVisibleForTestingAnnotationWhenUsedInProduction + - org.openrewrite.java.testing.cleanup.SimplifyTestThrows From d11ef6304f60f86cdf0f66671ddac371d1abaeb3 Mon Sep 17 00:00:00 2001 From: Johan Kragt Date: Wed, 9 Apr 2025 13:56:20 +0200 Subject: [PATCH 20/20] Added private filter --- ...bleForTestingAnnotationWhenUsedInProduction.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java index a12ba5fa9..a0f9bc77f 100644 --- a/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java +++ b/src/main/java/org/openrewrite/java/testing/cleanup/RemoveVisibleForTestingAnnotationWhenUsedInProduction.java @@ -23,6 +23,7 @@ import org.openrewrite.java.RemoveAnnotation; import org.openrewrite.java.search.IsLikelyNotTest; import org.openrewrite.java.search.UsesType; +import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; @@ -67,10 +68,10 @@ public TreeVisitor getScanner(VisibleForTesting acc) { @Override public J.FieldAccess visitFieldAccess(J.FieldAccess fa, ExecutionContext ctx) { - if (fa.getTarget().getType() instanceof JavaType.Class) { + if (fa.getTarget().getType() instanceof JavaType.Class && !((JavaType.Class) fa.getTarget().getType()).hasFlags(Flag.Private)) { checkAndRegister(acc.classPatterns, fa.getTarget().getType()); } - if (fa.getName().getFieldType() != null) { + if (fa.getName().getFieldType() != null && !fa.getName().getFieldType().hasFlags(Flag.Private)) { checkAndRegister(acc.fieldPatterns, fa.getName().getFieldType()); } return super.visitFieldAccess(fa, ctx); @@ -78,7 +79,7 @@ public J.FieldAccess visitFieldAccess(J.FieldAccess fa, ExecutionContext ctx) { @Override public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionContext ctx) { - if (mr.getMethodType() != null) { + if (mr.getMethodType() != null && !mr.getMethodType().hasFlags(Flag.Private)) { checkAndRegister(acc.methodPatterns, mr.getMethodType()); } return super.visitMemberReference(mr, ctx); @@ -86,7 +87,7 @@ public J.MemberReference visitMemberReference(J.MemberReference mr, ExecutionCon @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx) { - if (mi.getMethodType() != null) { + if (mi.getMethodType() != null && !mi.getMethodType().hasFlags(Flag.Private)) { checkAndRegister(acc.methodPatterns, mi.getMethodType()); } return super.visitMethodInvocation(mi, ctx); @@ -94,10 +95,10 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Execution @Override public J.NewClass visitNewClass(J.NewClass nc, ExecutionContext ctx) { - if (nc.getConstructorType() != null) { + if (nc.getConstructorType() != null && !nc.getConstructorType().hasFlags(Flag.Private)) { checkAndRegister(acc.methodPatterns, nc.getConstructorType()); } - if (nc.getClazz() != null && nc.getClazz().getType() instanceof JavaType.Class) { + if (nc.getClazz() != null && nc.getClazz().getType() instanceof JavaType.Class && !((JavaType.Class) nc.getClazz().getType()).hasFlags(Flag.Private)) { checkAndRegister(acc.classPatterns, nc.getClazz().getType()); } return super.visitNewClass(nc, ctx);