SimplifyChainedAssertJAssertion drops method call in replacement

[jtymes]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v5.43.3
• Maven/Gradle plugin v5.43.3
• rewrite-testing-frameworks v2.21.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-testing-frameworks:RELEASE -Drewrite.activeRecipes=org.openrewrite.java.testing.assertj.Assertj -Drewrite.exportDatatables=true

What is the smallest, simplest way to reproduce the problem?

I recreated this with a test, basically copying the existing stringContains() test. This shows the expectation:

@Test
void stringContainsObjectMethod() {
    rewriteRun(
      spec -> spec.recipes(
        new SimplifyChainedAssertJAssertion("contains", "isTrue", "contains", "java.lang.String"),
        new SimplifyChainedAssertJAssertion("contains", "isFalse", "doesNotContain", "java.lang.String")
      ),
      //language=java
      java(
        """
          import static org.assertj.core.api.Assertions.assertThat;

          class Pojo {
            public String getString() {
                return "lo wo";
            }
          }

          class MyTest {
              void testMethod() {
                  var pojo = new Pojo();
                  assertThat("hello world".contains(pojo.getString())).isTrue();
                  assertThat("hello world".contains("lll")).isFalse();
              }
          }
          """,
        """
          import static org.assertj.core.api.Assertions.assertThat;

          class Pojo {
            public String getString() {
                return "lo wo";
            }
          }

          class MyTest {
              void testMethod() {
                  var pojo = new Pojo();
                  assertThat("hello world").contains(pojo.getString());
                  assertThat("hello world").doesNotContain("lll");
              }
          }
          """
      )
    );
}

What did you expect to see?

import static org.assertj.core.api.Assertions.assertThat;

class Pojo {
  public String getString() {
    return "lo wo";
  }
}

class MyTest {
  void testMethod() {
      var pojo = new Pojo();
      assertThat("hello world").contains(pojo.getString());
      assertThat("hello world").doesNotContain("lll");
  }
}

What did you see instead?

import static org.assertj.core.api.Assertions.assertThat;

class Pojo {
  public String getString() {
      return "lo wo";
  }
}

class MyTest {
  void testMethod() {
      var pojo = new Pojo();
      assertThat("hello world").contains(pojo);
      assertThat("hello world").doesNotContain("lll");
  }
}

What is the full stack trace of any errors you encountered?

No stack, just incorrect replacement.

It appears to be related to the SimplifyChainedAssertJAssertion.extractEitherArgument method where getSelect() is chosen.

Are you interested in contributing a fix to OpenRewrite?

Yes, if I understand the requirements around the use of getSelect() instead of the full J.MethodInvocation.

[timtebeek]

Thanks for the clear & runnable report! Indeed looks like we ought to be slightly more conservative in reusing the exact argument passed into contains, not just the select that might be in there. Really helpful to have a unit test to step through. Are you already close to a fix? Welcome to open a draft PR even when all you have is a test; that helps us explore & collaborate as well.

[jtymes]

@timtebeek I don't have a fix yet, but I added the test in #629 to get things rolling.