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

Add recipe for replacing unnecesary Mockito#eq with direct parameters #615

Merged
merged 15 commits into from
Oct 13, 2024

Conversation

SanderKnauff
Copy link
Contributor

@SanderKnauff SanderKnauff commented Oct 10, 2024

What's changed?

A recipe has been added to update unnecessary usages of Mockito#eq in favour of directly passing through parameters.

What's your motivation?

This updates code to comply with SonarQube rule java:S6068.

Anything in particular you'd like reviewers to focus on?

This is my first recipe that is authored for public use. Any tips and improvements are very welcome as I do not have a great feel yet for the best practices and standards in this community.

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

N/A. A search and replace could work, but you'd have to be very careful not to change any wrong places.
Having a recipe to fix this is saver

Any additional context

What is the Java language level used for writing recipes in this project? Judging by the code, it seems that 8 is the standard. Are there any plans to update that? I notice quite some friction with instanceof checks and casting for example as more recent Java versions have improved the ergonomics a lot.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

This adds a basic recipe for replacing Mockito#eq with direct parameters in when this is supported.
This change should resolve Sonar issue java:S6068
@SanderKnauff SanderKnauff changed the title Add recipe for unnecesary Mockito#eq Add recipe for replacing unnecesary Mockito#eq with direct parameters Oct 10, 2024
@timtebeek timtebeek self-requested a review October 10, 2024 20:57
@timtebeek timtebeek added the recipe Recipe request label Oct 10, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Also tests methods using Method Matchers.
Also removes the `Mockito#eq` import if it is not required anymore.
@SanderKnauff SanderKnauff marked this pull request as ready for review October 12, 2024 15:52
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great to see this first contribution @SanderKnauff ! Especially like how thorough you are with your tests. Great to see this Sonar rule automatically resolved going forward. I've made it part of org.openrewrite.java.testing.mockito.MockitoBestPractices such that it runs automatically for folks using that, as opposed to having to run this separately.

I've applied some small polishing fixed to you PR, mostly to simplify things. In adopting MethodMatcher you can immediately pass that an expression, which removed the need for a few of the methods you'd spawned off from when you were still checking the method name manually. I've also adopted a few other conventions, like using ListUtils instead of the streams API, and adding a Precondition such that the recipe only visits classes that are using eq at all. Those help performance when running at scale.

As to your question about the Java level: for now we keep recipes at Java 8, but the tests can use Java 17. That way folks coming from older versions of Java can still run recipes while they gradually modernize their applications, as some projects will fail to build when you try to run them on newer Java versions.

I hope that answers your questions; I look forward to seeing what other ideas you have for further improvements.

@timtebeek timtebeek merged commit 3c92c0f into openrewrite:main Oct 13, 2024
1 of 2 checks passed
@SanderKnauff
Copy link
Contributor Author

Great to see this first contribution @SanderKnauff ! Especially like how thorough you are with your tests. Great to see this Sonar rule automatically resolved going forward. I've made it part of org.openrewrite.java.testing.mockito.MockitoBestPractices such that it runs automatically for folks using that, as opposed to having to run this separately.

I've applied some small polishing fixed to you PR, mostly to simplify things. In adopting MethodMatcher you can immediately pass that an expression, which removed the need for a few of the methods you'd spawned off from when you were still checking the method name manually. I've also adopted a few other conventions, like using ListUtils instead of the streams API, and adding a Precondition such that the recipe only visits classes that are using eq at all. Those help performance when running at scale.

As to your question about the Java level: for now we keep recipes at Java 8, but the tests can use Java 17. That way folks coming from older versions of Java can still run recipes while they gradually modernize their applications, as some projects will fail to build when you try to run them on newer Java versions.

I hope that answers your questions; I look forward to seeing what other ideas you have for further improvements.

Thank you for your help on this as well. I've written some smaller recipes that were specific only to internal projects in the past, so it was nice to get some tips on how to make things better.
I'll be sure to create another PR the next time I create a similar recipe :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants