Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"MigrateHamcrestToAssertJ" recipe does not fully migrate comparesEqualTo matcher #647

Closed
jtama opened this issue Nov 27, 2024 · 3 comments
Closed
Labels
assertj bug Something isn't working good first issue Good for newcomers recipe Recipe request

Comments

@jtama
Copy link

jtama commented Nov 27, 2024

What version of OpenRewrite are you using?

I am using

  • OpenRewrite v2.22.0
  • Maven/Gradle plugin ?
  • rewrite-testing-frameworks v2.22.0

How are you running OpenRewrite?

I am using executing openrewrite through the shell, 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.junit5.JUnit4to5Migration,org.openrewrite.java.testing.assertj.Assertj -Drewrite.exportDatatables=true

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

import java.math.BigDecimal;
import static org.hamcrest.Matchers.comparesEqualTo;

public class DummyTest {

    @Test
    public void dummyTest() {
        assertThat(new BigDecimal("2"), comparesEqualTo(new BigDecimal("2.0")));
    }
}

What did you expect to see?

import java.math.BigDecimal;
import static org.assertj.core.api.Assertions.assertThat;

public class DummyTest {

    @Test
    public void dummyTest() {
        assertThat(new BigDecimal("2")).isEqualByComparingTo(new BigDecimal("2.0"));
    }
}

What did you see instead?

import java.math.BigDecimal;
import static org.hamcrest.Matchers.comparesEqualTo;

public class DummyTest {

    @Test
    public void dummyTest() {
        assertThat(new BigDecimal("2"), comparesEqualTo(new BigDecimal("2.0")));
    }
}

I think it's all linked to to this line https://github.com/openrewrite/rewrite-testing-frameworks/blob/main/src/main/resources/META-INF/rewrite/hamcrest.yml#L65 that limits the type argument to String.

I can provide a PR if you like.

@jtama jtama added the bug Something isn't working label Nov 27, 2024
@jtama jtama changed the title "MigrateHamcrestToAssertJ" recipe does not migrate comparesEqualTo matcher "MigrateHamcrestToAssertJ" recipe does not fully migrate comparesEqualTo matcher Nov 27, 2024
@timtebeek
Copy link
Contributor

Hi @jtama ; thanks for calling that out! We had indeed added that quick limit to String type following

I hadn't since taken the time to reevaluate which other types are fine to support there as well; I'd welcome a PR that adds a few more allowed types to be converted, thanks!

@timtebeek timtebeek moved this to Recipes Wanted in OpenRewrite Nov 29, 2024
@timtebeek timtebeek added recipe Recipe request assertj good first issue Good for newcomers labels Nov 29, 2024
@jtama
Copy link
Author

jtama commented Nov 30, 2024

Ok I'll do that.

@jtama
Copy link
Author

jtama commented Dec 4, 2024

Ok I tried very hard to reproduce the behaviour but never succeded, the recipe does exists, the unit test pass. And we reran the MigrateHamcrestToAssertJ recipe the project on which we had the issue, and everything is has expected. Sorry for false issue.

@jtama jtama closed this as completed Dec 4, 2024
@github-project-automation github-project-automation bot moved this from Recipes Wanted to Done in OpenRewrite Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assertj bug Something isn't working good first issue Good for newcomers recipe Recipe request
Projects
Status: Done
Development

No branches or pull requests

2 participants