Recipe test shows no changes, while debugger does

[SanderKnauff]

I am writing a test to automatically solve a sonar issue.
For this test I have to remove the wrapper eq() from some Mockito verifications.

I have written some tests to see if my recipe works as intended, however I see some differences between what I expected to happen and what the results of the tests are.

My recipe and tests are as follows:

import org.jetbrains.annotations.NotNull;
import org.openrewrite.ExecutionContext;
import org.openrewrite.NlsRewrite;
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.TypeUtils;

import java.util.ArrayList;

public class SonarS6068Fixer extends Recipe {
    @NlsRewrite.DisplayName
    @NotNull
    @Override
    public String getDisplayName() {
        return "java:S6068";
    }

    @NotNull
    @NlsRewrite.Description
    @Override
    public String getDescription() {
        return "Fixes Sonar Issue [java:S6068: Call to Mockito method \"verify\", \"when\" or \"given\" should be simplified].";
    }

    @NotNull
    @Override
    public TreeVisitor<?, ExecutionContext> getVisitor() {
        return new SonarS6068FixerVisitor();
    }

    static class SonarS6068FixerVisitor extends JavaIsoVisitor<ExecutionContext> {
        @Override
        public J.MethodInvocation visitMethodInvocation(J.MethodInvocation originalInvocation, ExecutionContext executionContext) {
            final var methodInvocation = super.visitMethodInvocation(originalInvocation, executionContext);

            if (isMockitoWhen(methodInvocation) && methodInvocation.getArguments().getFirst() instanceof J.MethodInvocation whenArgument) {
                final var originalArguments = methodInvocation.getArguments();
                final var updatedInvocation = checkAndUpdateEq(whenArgument);
                final var updatedArguments = new ArrayList<>(originalArguments);
                updatedArguments.set(0, updatedInvocation);

                return methodInvocation.withArguments(updatedArguments);
            } else if (isInvokedOnVerify(methodInvocation)) {
                return checkAndUpdateEq(methodInvocation);
            }

            return originalInvocation;
        }

        private boolean isMockitoWhen(J.MethodInvocation methodInvocation) {
            return methodInvocation.getMethodType() != null
                    && TypeUtils.isOfType(methodInvocation.getMethodType().getDeclaringType(), JavaType.buildType("org.mockito.Mockito"))
                    && methodInvocation.getSimpleName().equals("when");
        }

        private boolean isInvokedOnVerify(J.MethodInvocation methodInvocation) {
            if (methodInvocation.getSelect() == null) {
                return false;
            }
            final var select = methodInvocation.getSelect();

            if (!(select instanceof J.MethodInvocation selectInvocation) || selectInvocation.getMethodType() == null) {
                return false;
            }

            return TypeUtils.isOfType(selectInvocation.getMethodType().getDeclaringType(), JavaType.buildType("org.mockito.Mockito"))
                   && selectInvocation.getSimpleName().equals("verify");
        }

        private J.MethodInvocation checkAndUpdateEq(J.MethodInvocation methodInvocation) {
            final var originalArguments = methodInvocation.getArguments();
            final var onlyEqArguments = originalArguments.stream().allMatch(this::isEqExpression);
            if (!onlyEqArguments) {
                return methodInvocation;
            }

            final var updatedArguments = originalArguments.stream()
                    .map(J.MethodInvocation.class::cast)
                    .map(invocation -> invocation.getArguments().getFirst())
                    .map(Expression.class::cast)
                    .toList();

            return methodInvocation.withArguments(updatedArguments);
        }

        private boolean isEqExpression(Expression expression) {
            if (!(expression instanceof J.MethodInvocation invocation)) {
                return false;
            }

            if (invocation.getMethodType() == null || !TypeUtils.isOfType(invocation.getMethodType().getDeclaringType(), JavaType.buildType("org.mockito.ArgumentMatchers"))) {
                return false;
            }

            return invocation.getSimpleName().equals("eq");
        }
    }
}

import org.junit.jupiter.api.Test;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.test.TypeValidation;

import static org.openrewrite.java.Assertions.java;

class SonarS6068FixerTest implements RewriteTest {
    @Test
    void verify_Simple_ShouldUpdate() {
        rewriteRun(
                spec -> spec.recipe(new SonarS6068Fixer())
                        .typeValidationOptions(TypeValidation.builder().build()),
                //language=Java
                java(
                        """
                        import static org.mockito.Mockito.verify;
                        import static org.mockito.Mockito.mock;
                        import static org.mockito.ArgumentMatchers.eq;
                        class Test {
                            public void test() {
                                final var mockString = mock(String.class);
                                verify(mockString).replace(eq("foo"), eq("bar"));
                            }
                        }
                        """,
                        """
                        import static org.mockito.Mockito.verify;
                        import static org.mockito.Mockito.mock;
                        import static org.mockito.ArgumentMatchers.eq;
                        class Test {
                            public void test() {
                                final var mockString = mock(String.class);
                                verify(mockString).replace("foo","bar");
                            }
                        }
                        """
                )
        );
    }

    @Test
    void when_Simple_ShouldUpdate() {
        rewriteRun(
                spec -> spec.recipe(new SonarS6068Fixer())
                        .typeValidationOptions(TypeValidation.builder().build()),
                //language=Java
                java(
                        """
                        import static org.mockito.Mockito.mock;
                        import static org.mockito.Mockito.when;
                        import static org.mockito.ArgumentMatchers.eq;
                        class Test {
                            public void test() {
                                final var mockString = mock(String.class);
                                when(mockString.replace(eq("foo"), eq("bar"))).thenReturn("bar");
                            }
                        }
                        """,
                        """
                        import static org.mockito.Mockito.mock;
                        import static org.mockito.Mockito.when;
                        import static org.mockito.ArgumentMatchers.eq;
                        class Test {
                            public void test() {
                                final var mockString = mock(String.class);
                                when(mockString.replace("foo", "bar")).thenReturn("bar");
                            }
                        }
                        """
                )
        );
    }

    @Test
    void when_MoreComplexMatchers_ShouldNotUpdate() {
        rewriteRun(
                spec -> spec.recipe(new SonarS6068Fixer())
                        .typeValidationOptions(TypeValidation.builder().build()),
                //language=Java
                java(
                        """
                        import static org.mockito.Mockito.mock;
                        import static org.mockito.Mockito.when;
                        import static org.mockito.ArgumentMatchers.eq;
                        import static org.mockito.ArgumentMatchers.anyString;
                        class Test {
                            public void test() {
                                final var mockString = mock(String.class);
                                when(mockString.replace(eq("foo"), anyString())).thenReturn("bar");
                            }
                        }
                        """
                )
        );
    }
}

Notably, when_Simple_ShouldUpdate fails without changes. What should have been when(mockString.replace("foo","bar")).thenReturn("bar"); supposedly still is when(mockString.replace(eq("foo"), eq("bar"))).thenReturn("bar");.

When I look in the debugger however, I can see that I do return the expected changes to the method:

The method returns the updated MethodInvocation, where the arguments list of the when call is updated.
The debugger also hits this line twice with this data. I assume once for the actual change, and then a second time to see if my changes are stable between multiple runs.

I am quite new to writing these kinds of recipes, so I may have missed something fairly obvious. Are there any glaring issues with my approach?

[timtebeek]

Hoi @SanderKnauff ; great to see you're writing a recipe to resolve java:S6068. Would you be willing to contribute this recipe to rewrite-testing-frameworks? That way it's easier for me to check out your changes from a draft PR and verify what changes are needed to make this work. Once fixed others can then also benefit from your work. :)

[SanderKnauff]

Certainly! I've created openrewrite/rewrite-testing-frameworks#615. It is still in the same state as in this post, but I hope to update it soon to more match the standards of the other recipes.