JUnitFailToAssertJFail doesn't remove existing Junit import

[timo-abele]

What version of OpenRewrite are you using?

I am using

• Maven plugin v5.18.0
• rewrite-testing-frameworks v2.2.0

How are you running OpenRewrite?

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

<plugin>
    <groupId>org.openrewrite.maven</groupId>
    <artifactId>rewrite-maven-plugin</artifactId>
    <version>5.18.0</version>
    <configuration>
        <activeRecipes>
            <recipe>org.openrewrite.java.testing.assertj.Assertj</recipe>
        </activeRecipes>
    </configuration>
    <dependencies>
        <dependency>
            <groupId>org.openrewrite.recipe</groupId>
            <artifactId>rewrite-testing-frameworks</artifactId>
            <version>2.2.0</version>
        </dependency>
    </dependencies>
</plugin>

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

import static org.junit.jupiter.api.Assertions.fail;
import org.junit.jupiter.api.Test;

class A {
    @Test
    void foo() {
        fail(id + "not found");
        fail("foo")
    }
}

What did you expect to see?

import static org.assertj.core.api.Assertions.fail;
import org.junit.jupiter.api.Test;

class A {
    @Test
    void foo() {
        fail("", id + "not found");
        fail("foo")
    }
}

What did you see instead?

import static org.assertj.core.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.fail;

import org.junit.jupiter.api.Test;

class A {
    @Test
    void foo() {
        fail("", id + "not found");
        fail("foo")
    }
}

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

In the first case (fail(id + "not found")) there are no errors, just an unused import that could be avoided by adding a maybeRemoveImport right after this line

rewrite-testing-frameworks/src/main/java/org/openrewrite/java/testing/assertj/JUnitFailToAssertJFail.java

Line 148 in a426e40

maybeAddImport("org.assertj.core.api.Assertions", "fail");

In the second case, there is actually an error, namely

Ambiguous method call. Both
fail (String) in Assertions and
fail (String) in Assertions match

Are you interested in contributing a fix to OpenRewrite?

no

[timtebeek]

Hi @timo-abele ; I've tried to replicate your case but had to make a few small changes to get the example to compile, after which the test already passed without code changes:

@Test
@Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/467")
void removeImport() {
    //language=java
    rewriteRun(
      java(
        """
          import org.junit.jupiter.api.Test;
          
          import static org.junit.jupiter.api.Assertions.fail;

          class A {
              @Test
              void foo() {
                  String id = "bar";
                  fail(id + "not found");
                  fail("foo");
              }
          }
          """,
        """
          import org.junit.jupiter.api.Test;
          
          import static org.assertj.core.api.Assertions.fail;

          class A {
              @Test
              void foo() {
                  String id = "bar";
                  fail("", id + "not found");
                  fail("foo");
              }
          }
          """
      )
    );
}

Note how I define id before it's used, such that there's no missing types. I also added a missing semicolon after the second fail. I'd added this to

rewrite-testing-frameworks/src/test/java/org/openrewrite/java/testing/assertj/JUnitFailToAssertJFailTest.java

Line 29 in 53cfd4c

The left over import is already cleaned up on this line, provided there's no left over calls to the old method

rewrite-testing-frameworks/src/main/java/org/openrewrite/java/testing/assertj/JUnitFailToAssertJFail.java

Line 115 in 4582163

doAfterVisit(new RemoveUnusedImports().getVisitor());

Could you let me know how we can better replicate what might factor into your case? You might have stripped away some crucial information. Is id really a call to a lombok getter for instance, and thus a source of missing type information?

[timo-abele]

I managed to condense the original to something reproducible:

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.fail;

import lombok.Getter;

class MyTest {

  class DebugEnum {

    @Getter
    private String key = "9";
  }

  void myMethod() {
    String s = new DebugEnum().getKey();
    assertThat(1 + 1).isEqualTo(2);
    fail("foo");
  }
}

DebugEnum was originally an enum in a separate file and even then the getter needed to be there and be called to reproduce this. An explicit getter does not lead to the buggy behavior.
You've mentioned missing type information in relation to lombok above. However, this is a very condensed example. You can put DebugEnum in it's own file and fail("foo") in its own method so that all the bad stuff happens in myMethod and not in the method that holds the only usage of the fail import. The bug will still appear.

[timtebeek]

Thanks for the reduced sample! I've had another look at this after running into the same yesterday. You'll recall that the recipe clears out the imports through RemoveUnusedImports, which itself has a precondition that there should not be any missing types:

https://github.com/openrewrite/rewrite/blob/14a2bc687836c76eb99612bef494a60abc476f6c/rewrite-java/src/main/java/org/openrewrite/java/RemoveUnusedImports.java#L71-L73

This is to prevent the recipe from clearing out lots of necessary imports when for instance Lombok, or mocking is used, as both are a frequent source of missing types as we've also seen here on the generated getKey() method.

I hope to remedy that partly with this change pushed yesterday: 0f7fc95
Beyond that there's not a lot we can do in this particular case without potentially breaking a lot of other cases, as missing types are an unfortunate reality we have to account for, which poses its limitations on what we can achieve through recipes.

Would you be ok with me closing this issue as there's no action we can take in this particular recipe module that would fix this for all possible cases?

[timo-abele]

I'm OK with the issue being closed, I've reported the bug, and leave it to your discretion whether this is appropriately addressed.
I haven't really understood the "missing types" thing at the core of this, and might run into the same issue again in the future. Is there any document where I can read up on that problem?

[timtebeek]

We don't really have a dedicated page on what might cause missing types, or how to explore and resolved such issues unfortunately. Perhaps something for our frequently asked questions. Until then you can explore missing types with this recipe which adds search markers: https://docs.openrewrite.org/recipes/java/search/findmissingtypes
(cc @mike-solomon )

I'll close this issue for now as we can't fix it in this repository. Thanks again for reporting your findings!