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

Migrate Hamcrest to JUnit 5 #343

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

matusmatokpt
Copy link
Contributor

@matusmatokpt matusmatokpt commented May 23, 2023

This is a draft pull request for #212

matusmatokpt and others added 3 commits May 22, 2023 12:46
Work in progress implementation of a recipe which migrates Hamcrest test matchers to JUnit5 test assertions.

Signed-off-by: matus.matok <matus.matok@pantheon.tech>
@timtebeek timtebeek marked this pull request as draft May 23, 2023 12:23
@timtebeek timtebeek changed the title WIP: Add MigrateFromHamcrest recipe Migrate Hamcrest to AssertJ May 23, 2023
@timtebeek timtebeek changed the title Migrate Hamcrest to AssertJ Migrate Hamcrest to JUnit 5 May 23, 2023
@timtebeek timtebeek added the recipe Recipe request label May 23, 2023
timtebeek and others added 5 commits May 23, 2023 15:28
Added a prototype-y implementation of translation from hamcrest's equalTo to JUnit5's assertEquals. Should be easy to add more of the simple hamcrest matchers to this implementation.

\TODO the import is not being added

Signed-off-by: matus.matok <matus.matok@pantheon.tech>
Comment on lines 104 to 105
case "equalTo":
return "assertEquals";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than statically defining the various options here, we could also look at turning the hamcrestMatcher and targetAssertion into @Option arguments to the recipe, such that we can configure this repeatedly from a declarative Yaml description file. We already do something similar for Mockito, except in thise case you'd be configuring this new recipe rather than ChangeMethodName. Such an approach pull those replacements out of code and into configuration, which is typically easier to expand on, even locally, for our users.

Comment on lines 78 to 85
case 0:
return method.withTemplate(template, method.getCoordinates().replace(), firstArg);
case 1:
return method.withTemplate(template, method.getCoordinates().replace(), firstArg, matcherArgs.get(0));
case 2 :
return method.withTemplate(template, method.getCoordinates().replace(), firstArg, matcherArgs.get(0), matcherArgs.get(1));
case 3 :
return method.withTemplate(template, method.getCoordinates().replace(), firstArg, matcherArgs.get(0), matcherArgs.get(1), matcherArgs.get(2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last argument in withTemplate is actually a varargs. So you could reduce this when opting for a Object[] args as seen here.

Object[] args;
if (mi.getArguments().size() == 2) {
args = new Object[]{s.getSelect(), s.getArguments().get(0), mi.getArguments().get(1)};
sb.append(", #{any()}");
} else {
args = new Object[]{s.getSelect(), s.getArguments().get(0)};

//to be replaced with a static map
switch (translatedAssertion) {
case "assertEquals":
return "assertEquals(#{any(java.lang.Object)}, #{any(java.lang.Object)})";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the recipe will fail currently with any 3/4 method arguments, as the template hardcodes the expected number of arguments to two; Might be best to add tests for various amounts of arguments and work from there to repeatedly append , #{any(java.lang.Object)} as needed, as also seen here.

sb.append("assertEquals(#{any(java.lang.Object)},#{any(java.lang.Object)}");
Object[] args;
if (mi.getArguments().size() == 2) {
args = new Object[]{s.getSelect(), s.getArguments().get(0), mi.getArguments().get(1)};
sb.append(", #{any()}");
} else {
args = new Object[]{s.getSelect(), s.getArguments().get(0)};
}
sb.append(")");

Comment on lines 40 to 45
class Biscuit {
String name;
Biscuit(String name) {
this.name = name;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also want to verify our use of equalTo with Strings and other primitives, just to be sure we don't inadvertently produce errors there.

spec
.parser(JavaParser.fromJavaVersion()
.classpathFromResources(new InMemoryExecutionContext(), "junit-jupiter-api-5.9", "hamcrest-2.2"))
.recipe(new MigrateFromHamcrest());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do end up going for declarative yaml recipes for the various matcher-to-method replacements, then we will likely want to import yaml recipes and run those instead of this direct invocation. Here's an example.

.recipe(Environment.builder()
.scanRuntimeClasspath("org.openrewrite.java.testing.mockito")
.build()
.activateRecipes("org.openrewrite.java.testing.mockito.AnyToNullable"));

matusmatokpt and others added 2 commits June 13, 2023 10:50
Another iteration of the prototype. Adapted it to be
based on the up-to-date main.
Added a proposal of how similar simple matchers could be
translated to junit assertion methods.

Signed-off-by: matus.matok <matus.matok@pantheon.tech>
@matusmatokpt
Copy link
Contributor Author

matusmatokpt commented Jun 13, 2023

Hello, got back to this. Noticed that the code base updated a little so I ammended my prototype. The tests dont run however, I am getting a bunch of error messages and I have not figured out how they are related to my changes, so if anyone could take a look, that would be appreciated.
Secondly, I addressed Tim's hints from the comments above, and added a proposal of how to add further simple matcher-to-method translations. Looking forward to your feedback. If there will be a desire to split each case (by matchers) into separate recipes then I will of course do so.
Another remark: for these simple matchers we do not need vararg methods as mentioned here #343 (comment) , due to their simple nature. Might be needed for matchers like anyOf, but we are not there yet. Those will probably need a different approach.

@timtebeek
Copy link
Contributor

Appreciate you getting back to this! I've asked @knutwannheden to have a look as I'm caught up in other work this week; from the failures it appears your recipe might not apply or make changes. Did you already try to step through with the debugger?

@matusmatokpt
Copy link
Contributor Author

Yes I did. Seemingly, the recipe instantiates alright, but during the run phase it fails and I don't understand why. Wanted to double check it so I inserted some console prints into the recipe and they are printed as expected when the test is run. I will add some more simple matcher-to-method cases while I wait for the review.

public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) {
J.MethodInvocation mi = super.visitMethodInvocation(method, executionContext);
MethodMatcher matcherAssertTrue = new MethodMatcher("org.hamcrest.MatchAssert assertThat(String, boolean)");
MethodMatcher matcherAssertMatcher = new MethodMatcher("org.hamcrest.MatcherAssert assertThat(*, org.hamcrest.Matcher)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this matcher is failing, but you can work around it by for now adopting:

Suggested change
MethodMatcher matcherAssertMatcher = new MethodMatcher("org.hamcrest.MatcherAssert assertThat(*, org.hamcrest.Matcher)");
MethodMatcher matcherAssertMatcher = new MethodMatcher("org.hamcrest.MatcherAssert assertThat(..)");

You might need to match the three different MethodMatcher assertThat methods in a specific order with this .. variant last, but at least that will get you going.

Copy link
Contributor Author

@matusmatokpt matusmatokpt Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seemingly has something to do with matcherMethod being called on primitives, for example equalTo(5).

Comment on lines 77 to 78
template.apply(getCursor(), method.getCoordinates().replace(), mi.getArguments().get(0), strippedMatcher);
maybeAddImport("org.junit.jupiter.api.Assertions", targetAssertion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to return the changed template! With this return a first of your tests now passes 🎉

Suggested change
template.apply(getCursor(), method.getCoordinates().replace(), mi.getArguments().get(0), strippedMatcher);
maybeAddImport("org.junit.jupiter.api.Assertions", targetAssertion);
maybeAddImport("org.junit.jupiter.api.Assertions", targetAssertion);
return template.apply(getCursor(), method.getCoordinates().replace(), mi.getArguments().get(0), strippedMatcher);

@timtebeek
Copy link
Contributor

Yes I did. Seemingly, the recipe instantiates alright, but during the run phase it fails and I don't understand why. Wanted to double check it so I inserted some console prints into the recipe and they are printed as expected when the test is run. I will add some more simple matcher-to-method cases while I wait for the review.

Sorry about the delays in getting you feedback; It's been pretty busy with the new 8.0 release coming up.

I've posted two suggestions to get your first tests passing; That should help to start iterating by adding options, configuring those from Yaml, and see how far we can get with covering different Hamcrest matchers. Hope that helps!

Added translations for matchers closeTo, containsString, empty, emptyArray, emptyIterable, emptyCollectionOf, emptyIterableOf, endsWith.
Tests need to be added for each matcher.

Signed-off-by: matus.matok <matus.matok@pantheon.tech>
@matusmatokpt
Copy link
Contributor Author

No problem with the delays. Here is some further progress.

Some tests -- those that are supposed to add import of ..Assertions.assertTrue; do not add it, hence the tests fail.
I added a bunch of simple cases, they will need to have tests added, keeping that in mind.
For simplicity, I am adding all the simple cases into a single recipe. You keep mentioning something with YAML, but I think I will need more explanation in that department.
I will probably get to work on this in the next week, so the review is in no rush.
Thanks

@timtebeek
Copy link
Contributor

Thanks a lot for your continued effort on this! I've moved your classes such that they are more in line with the AssertJ recipes that we have a in parallel. What I like about your approach is that you're better able to handle nested Matchers, which is a bit of a struggle with the alternative implementation for AssertJ.

What I'm wondering with your implementation is if it would make sense to introduce an enum with fields to hold the before, after and template method strings. For instance with some rough pseudo code here

enum Replacements {
    EQUALTO("equalTo", "assertEquals", "#{any(java.lang.Object)}, #{any(java.lang.Object)}",
            matcher -> matcher.getArguments()),
   // TODO More replacements
    ;

    String before, after, template;
    Function<J.MethodInvocation, List<Expression>> arguments;

    Replacements(String before, String after, String template, Function<J.MethodInvocation, List<Expression>> arguments) {
        this.before = before;
        this.after = after;
        this.template = template;
        this.arguments = arguments;
    }
}

Not sure if that would work out for all cases, but it could help group together the logic that's now maintained in three distinct switch statements into a structure that's perhaps easier to read and expand. Would you feel there's any merit to exploring that option?

It would maybe need some special handling up front for is and not and other special cases, perhaps even using what we already have in org.openrewrite.java.testing.hamcrest.RemoveIsMatcher.

@matusmatokpt
Copy link
Contributor Author

The enum would probably have to hold the entire template as well, but sometimes the template is diffrent if the context is negated by not, which would need some headscratching. I am busy in the upcomming days, but I'll let this live at the back of my head and hopefully I ll come up with something.
I was thinking of rehosting the not and is cases to separate class that would remove them and would modify the ExecutionContext variable so that the inheritors of that class would not need to worry about is and not which would get removeved by calling super.visitMethodInvocation(.., ExecutionContext). The only problem is the removal of the information about negation from ExecutionContext. It would be nice, if the super method would remove the information from there so that the callee doesnt need to bother with that. That is some more thinking to be done.
Do these ideas sound reasonable? All suggestions are appreciated.

@matusmatokpt
Copy link
Contributor Author

Obviously, this PR is not yet merged, but it has functionality sufficient for some cases on our project. Therefore I have an offtopic question. Can I setup the recipe for the project from my fork or does it have to be merged into upstream?

@timtebeek
Copy link
Contributor

I was thinking of rehosting the not and is cases to separate class that would remove them and would modify the ExecutionContext variable so that the inheritors of that class would not need to worry about is and not

I think that's a perfectly valid approach; I've split those out in the AssertJ migration as well, as they're quite special. The is case you can work around in a similar manner as seen here

recipeList:
# First remove wrapping `is(Matcher)` calls such that further recipes will match
- org.openrewrite.java.testing.hamcrest.RemoveIsMatcher
# Then remove calls to `MatcherAssert.assertThat(String, boolean)`
- org.openrewrite.java.testing.hamcrest.AssertThatBooleanToAssertJ
- org.openrewrite.java.testing.hamcrest.HamcrestMatcherToAssertJ:
matcher: comparesEqualTo
assertion: isEqualTo

There we first remove any wrapping is before applying further recipes. You can likely use that exact same recipe to simplify the conversion.

Obviously, this PR is not yet merged, but it has functionality sufficient for some cases on our project. Therefore I have an offtopic question. Can I setup the recipe for the project from my fork or does it have to be merged into upstream?

Absolutely; you can install the library to your local Maven repository through ./gradlew publishToMavenLocal, and then pick it up in a similar manner to our snapshots versions, but using your local Maven repository. That's a great way to verify your recipes work in practice as well, and find more edge cases to convert.

@matusmatokpt
Copy link
Contributor Author

matusmatokpt commented Jul 4, 2023

Hello, attempted to apply the recipe to our project, but cannot make it work. First I used the command u provided ./gradlew publishToMavenLocal which amongst everything else yielded:

> Configure project :
Inferred project: rewrite-testing-frameworks, version: 0.1.0-SNAPSHOT

So I went to try it in our local project, and added the plugin as follows:

<plugin>
    <groupId>org.openrewrite.maven</groupId>
    <artifactId>rewrite-maven-plugin</artifactId>
    <version>4.41.1</version>
    <configuration>
        <activeRecipes>
            <recipe>org.openrewrite.java.testing.hamcrest.HamcrestMatcherToJUnit5</recipe>
            <recipe>org.openrewrite.java.testing.hamcrest.AssertThatBooleanToJUnit5</recipe>
        </activeRecipes>
    </configuration>
    <dependencies>
        <dependency>
            <groupId>org.openrewrite.recipe</groupId>
            <artifactId>rewrite-testing-frameworks</artifactId>
            <version>0.1.0-SNAPSHOT</version>
        </dependency>
    </dependencies>
</plugin>

Then I simply ran mvn clean install but none of the test files got refactored, even though they should (they contain hamcrest matchers translatable by my recipe). Is there anything I am doing wrong?

@knutwannheden
Copy link
Contributor

Hello, attempted to apply the recipe to our project, but cannot make it work. First I used the command u provided ./gradlew publishToMavenLocal which amongst everything else yielded:

> Configure project :
Inferred project: rewrite-testing-frameworks, version: 0.1.0-SNAPSHOT

This is very strange. It should instead print Inferred project: rewrite-testing-frameworks, version: 2.1.0-SNAPSHOT. I am not sure where that 0.1.0 version is coming from when you run the command. Possibly this could explain why it then doesn't work. Can you check if your local Maven repo really does contain an artifact with the 0.1.0-SNAPSHOT?

@matusmatokpt
Copy link
Contributor Author

Negative. mvn dependency:get -Dartifact=org.openrewrite.recipe:rewrite-testing-frameworks:0.1.0-SNAPSHOT -o -DrepoUrl=file://~/.m2/repository results in a nasty error message, therefore I assume it does not contain it, unless I butchered the command.

@knutwannheden
Copy link
Contributor

You can also just inspect the contents of your ~/.m2/repository/org/openrewrite/recipe/rewrite-testing-framework/ directory, to check what you find there.

@matusmatokpt
Copy link
Contributor Author

Okay. Found it

@knutwannheden
Copy link
Contributor

I still don't understand why it would publish it to your local Maven repo using the version 0.1.0-SNAPSHOT. 2.1.0-SNAPSHOT would be the expected version.

@matusmatokpt
Copy link
Contributor Author

well, neither do I. I have not touched any config files while working on this PR. Only the recipes, so I do not know. Either way, assume that 0.1.0-SNAPSHOT is the correct version. What could be other factors causing my recipe not to work in our project? (mvn config as above, prints no errors on build)

@knutwannheden
Copy link
Contributor

Hmm. If you run mvn clean install that would typically not cause the rewrite plugin to do anything. Have you tried explicitly running the run or the dryRun goals? See https://docs.openrewrite.org/running-recipes/running-rewrite-on-a-maven-project-without-modifying-the-build and https://docs.openrewrite.org/reference/rewrite-maven-plugin

@timtebeek
Copy link
Contributor

Hi @matusmatokpt ! Was wondering what the current status of the PR is to you; Were you able to apply the changes locally? Are you still aiming to see this through to a merge?

We've made some process since with recipes that prepare a migration such as removing is and flatting allOf; perhaps those could apply here as well?

@matusmatokpt
Copy link
Contributor Author

Hi. I have been swamped with different tasks. I want to eventually finish this in my free time, but I am not sure when I ll have chance to get back to this. Maybe next week.
I have not noticed any new recipes, will take a look. A link to the related PR(s) please?

@timtebeek
Copy link
Contributor

Hi. I have been swamped with different tasks. I want to eventually finish this in my free time, but I am not sure when I ll have chance to get back to this.

All good! No rush; I'll mark the PR as a draft and move it to in progress then, just so that it's clear when you'd want to start a new round of review.

I have not noticed any new recipes, will take a look. A link to the related PR(s) please?

Sure! Specifically these two I thought could be interesting to reuse to simplify your migration recipes.

public class RemoveIsMatcher extends Recipe {
@Override
public String getDisplayName() {
return "Remove Hamcrest `is(Matcher)`";

public class FlattenAllOf extends Recipe {
@Override
public String getDisplayName() {
return "Convert Hamcrest `allOf(Matcher...)` to individual `assertThat` statements";

We have similar plans still for anyOf, the concepts of which can again be reused here.

@timtebeek timtebeek marked this pull request as draft July 20, 2023 11:37
@matusmatokpt
Copy link
Contributor Author

Looks neat. However, the the effort to migrate anyOf as described above will not be of much use to the hamcrest -> JUnit5 case.

When it comes to this recipe, I will try to wire it up using enums as you suggested, or maybe I will create a nested class called for example RecipeCase, which will contain the necessary fields. Will see what works the best. Either way, I can probably remove the is matcher case from the recipe.

@timtebeek
Copy link
Contributor

Thanks for your continued plans to work on this! In general I would say try to break the recipe up into smaller reusable components that we can then wire together in a declarative yaml file; that way it's easier to implement, reason about and maintain.

matusmatokpt and others added 3 commits August 2, 2023 18:30
Refactored HamcrestMatcherToJUnit5 recipe, so now the whole translation is stored in one place, not scatter amongst three methods. Given the existence of RemoveIsMatcher Recipe, this recipe relies that it will never encounter is() matcher.
In similar fashion as RemoveIsMatcher, RemoveNotMatcher was added, which does exactly the same as RemoveIsMatcher, but it also stores the logical context for the nested matcher (so that it knows it was negated) in execution context.
Matchers instanceOf and isA were difficult to handle within the HamcrestMatcherToJUnit5 recipe, therefore these cases were moved to a newly added HamcrestInstanceOfToJUnit5 recipe.

Signed-off-by: matus.matok <matus.matok@pantheon.tech>
Forgot, added now

Signed-off-by: matus.matok <matus.matok@pantheon.tech>
@matusmatokpt matusmatokpt marked this pull request as ready for review August 3, 2023 09:22
timtebeek and others added 3 commits June 15, 2024 10:40
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants