RemoveTestPrefix should not (only) rename methods called from other methods

[timtebeek]

Quickly noted down here, can be tackled individually.

• Should not rename test when there's a static method import of the same name (can lead to conflicts)
• Should not (only) rename test method when called from other methods (which unfortunately happens)

[nmck257]

Note: this issue is a bit more visible now that RemoveTestPrefix is included in JUnit5BestPractices: cb59832

[protocol7]

Just to offer up a test case for the first issue listed above as we ran into this in our codebase:

  @Test
  void testRemoveTestPrefixWithClashingMethod() {
    rewriteRun(
        spec -> spec.recipe(new RemoveTestPrefix()),
        java(
            """
            package com.spotify.helloworld;

            import org.junit.jupiter.api.Test;
            import static java.util.List.of;

            public class FooTest {

                @Test
                void testOf() {
                  of();
                }
            }"""));
  }

I can put this test in a PR if that is preferable. Not sure I'd know how to fix it though.

[timtebeek]

Thanks @protocol7 ; this one was easy it seems;

• Fixed in Skip RemoveTestPrefix when calling a similarly named method #543

[timtebeek]

We could maybe fix the remaining issue (within a single class) by switching to ChangeMethodName in a doAfterVisit instead of using the .withXyz() methods in this recipe directly. But we'd need to correctly construct the method pattern to match from the method declaration to do so, and even then it would only work in a single compilation unit, not across.

[protocol7]

Not sure if the same issue as discussed here, but here's an issue we're seeing when a test method is called by a different test method:

  @Test
  void removeTestPrefixWhenCalled() {
    rewriteRun(
        spec -> spec.recipe(new RemoveTestPrefix()),
        // language=java
        java(
            """
            package com.helloworld;

            import org.junit.jupiter.api.Test;

            public class FooTest {
              @Test
              void bar() {
                  testFoo();
              }

              @Test
              void testFoo() {}
            }
            """,
            """
            package com.helloworld;

            import org.junit.jupiter.api.Test;

            public class FooTest {
              @Test
              void bar() {
                  foo();
              }

              @Test
              void foo() {}
            }
            """));
  }

Unexpected result in "com/helloworld/FooTest.java":

diff --git a/com/helloworld/FooTest.java b/com/helloworld/FooTest.java
index f0cd70f..222415f 100644
--- a/com/helloworld/FooTest.java
+++ b/com/helloworld/FooTest.java
@@ -5,7 +5,7 @@
 public class FooTest {
   @Test
   void bar() {
-      foo();
+      testFoo();
   }

   @Test

In this case, either testFoo should not be renamed, or the invokation of it also needs to be renamed.

[timtebeek]

Thanks for the runnable example @protocol7 ; That's indeed what I had intended with the second not-yet-crossed-off todo item here. I think we'll both agree it's not optimal or expected for test methods to also be invoked directly, but perhaps best to guard against that. This would be relatively straightforward for usages from within the same class; a little more effort for usages from other classes.

For the simplest case I think this still stands:

We could maybe fix the remaining issue (within a single class) by switching to ChangeMethodName in a doAfterVisit instead of using the .withXyz() methods in this recipe directly.

Did you already explore that using the runnable test you have?

[protocol7]

I have not tried to fix this one, and probably won't be able to in a near term future.

And, agree on this been a poor practice, but in large code bases, everything will appear at least once :)