openrewrite / rewrite-testing-frameworks

OpenRewrite recipes that perform common Java testing migration tasks.
Apache License 2.0
68 stars 57 forks source link

org.openrewrite.java.testing.junit5.JUnit5BestPractices should fix a `null` as `actual` and flip the parameters #492

Closed timo-abele closed 3 months ago

timo-abele commented 3 months ago

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.23.1</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.4.1</version>
                    </dependency>
                </dependencies>
            </plugin>

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

class A {
    class B {};

    @Test
    public void FlippedParams(){
        B myVariable = new B();

        assertNotEquals(myVariable, null);
    }
}

Or even ``

What did you expect to see?

class A {
    class B {};

    @Test
    public void FlippedParams(){
        B myVariable = new B();

        assertThat(myVariable).isNotEqualTo(null);
    }
}

Or maybe through a dedicated pre-clean recipe that flips them: assertNotEquals(null, myVariable); This would cover more cases

What did you see instead?

class A {
    class B {};

    @Test
    public void FlippedParams(){
        B myVariable = new B();

        assertThat(null).isNotEqualTo(myVariable);
    }
}

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

Ambiguous method call. Both
assertThat
(Predicate<Object>)
in Assertions and
assertThat
(IntPredicate)
in Assertions match

Comments

This is intentionally filed as a bug report and not a feature request because it breaks the build. So this has to be fixed. I see no reason for any user to push an Assertion on null to main.

Are you interested in contributing a fix to OpenRewrite?

timtebeek commented 3 months ago

Thanks for the suggestion @timo-abele ; I think that's suitably covered in 964e677739350ce16cc0b4ee55fb47d152a2baf0 now by reusing a recipe we already had to flip misplaced assertions first before migrating to AssertJ. Hope you agree!