Skip to content

Migrate Hamcrest Matchers to AssertJ Assertions#355

Merged
timtebeek merged 26 commits intomainfrom
tim/migrate_hamcrest_to_assertj
Jun 22, 2023
Merged

Migrate Hamcrest Matchers to AssertJ Assertions#355
timtebeek merged 26 commits intomainfrom
tim/migrate_hamcrest_to_assertj

Conversation

@timtebeek
Copy link
Copy Markdown
Member

@timtebeek timtebeek commented Jun 15, 2023

What's changed?

Add a generic recipe to replace Hamcrest Matchers with AssertJ Assertions.

What's your motivation?

Hamcrest has not been released in a long time, whereas AssertJ is more actively maintained.
#212

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

If there's any additional matchers we can replace, or argument number corner cases missed.

Anyone you would like to review specifically?

@knutwannheden @kunli2

Have you considered any alternatives or workarounds?

I'd assume it's easier to do it this way rather than go for Java templates for match & replacement, since there's less code to maintain this way and this way there's no templates that depend on multiple libraries. We can still use templates for the more complicated cases.

Any additional context

Similar to #343 ; this is mostly to demonstrate the Yaml recipes I had intended there.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@timtebeek timtebeek added the recipe Recipe request label Jun 15, 2023
@timtebeek timtebeek self-assigned this Jun 15, 2023
Comment thread src/main/resources/META-INF/rewrite/hamcrest.yml
@knutwannheden
Copy link
Copy Markdown
Contributor

@timtebeek I notice this is still in draft. I take it that means it isn't ready for review yet?

@timtebeek
Copy link
Copy Markdown
Member Author

I notice this is still in draft. I take it that means it isn't ready for review yet?

A full review is probably too early indeed, but any critique of the general approach is much appreciated at this early stage, such that we don't spent effort building the wrong thing potentially.

@timtebeek timtebeek marked this pull request as ready for review June 18, 2023 11:17
@timtebeek timtebeek requested a review from knutwannheden June 18, 2023 11:18
@timtebeek
Copy link
Copy Markdown
Member Author

This already migrates some ~50 Hamcrest Matchers. I suggest we pick up any further work separately in #357

@timtebeek timtebeek requested a review from joanvr June 20, 2023 04:34
@kunli2
Copy link
Copy Markdown
Contributor

kunli2 commented Jun 22, 2023

Nice work, it looks good to me!

Copy link
Copy Markdown
Contributor

@joanvr joanvr left a comment

Choose a reason for hiding this comment

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

LGTM from a code style perspective :)

@timtebeek
Copy link
Copy Markdown
Member Author

Thanks both! Then I'll merge and try it out on the platform when I get back Monday.

@timtebeek timtebeek merged commit 86c23dd into main Jun 22, 2023
@timtebeek timtebeek deleted the tim/migrate_hamcrest_to_assertj branch June 22, 2023 15:40
@knutwannheden
Copy link
Copy Markdown
Contributor

@timtebeek Did you try to cover all Hamcrest matchers or only a specific subset? It looks like the closeTo() matcher (see https://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/Matchers.html#closeTo(double,%20double)) isn't covered yet, which could be migrated to one of the isCloseTo() methods in AssertJ: http://joel-costigliola.github.io/assertj/core-8/api/org/assertj/core/api/AbstractDoubleAssert.html#isCloseTo-double-org.assertj.core.data.Offset-

I didn't compare the lists in detail.

@timtebeek
Copy link
Copy Markdown
Member Author

I'd mostly covered those for strings, objects and collections. There's indeed likely more matches that we could quite easily covered, and more complex recipes that require more work. We can track these in #357

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.

4 participants