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 support for Junit 4 matchers on AssertionsArgumentOrder #637

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

adambir
Copy link
Contributor

@adambir adambir commented Nov 6, 2024

What's changed?

Added support for Junit 4 matchers in the AssertionsArgumentOrder recipe :

  • assertEquals
  • assertNotEquals
  • assertSame
  • assertNotSame

What's your motivation?

Junit 5 migration is sometimes complicated so supporting Junit 4 is a plus.

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

Did not add support for assertIterableEquals (does not exist in Junit 4), for assertArrayEquals and assertNull as the signature differs from Junit 5.

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

@adambir adambir changed the title Add support for Junit 4 matchers Add support for Junit 4 matchers on AssertionsArgumentOrder Nov 6, 2024
@timtebeek timtebeek self-requested a review November 6, 2024 22:38
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.

hi @adambir ; thanks for the initiative here! I'd originally left out JUnit 4 as the message argument handling there is different from JUnit Jupiter. I've placed a note on the new test to explain.

I think we could still support this provided we treat JUnit 4 and Jupiter separately, much like we also do for TestNG. Then we can separately check which indexed argument to use for the message, and which expected/actual arguments might need to be swapped. Perhaps good to update this PR to adjust the tests first, and only then look at separating those two.

@timtebeek timtebeek marked this pull request as draft November 11, 2024 21:07
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/test/java/org/openrewrite/java/testing/assertj/AssertJBestPracticesTest.java
    • lines 322-322

@adambir
Copy link
Contributor Author

adambir commented Nov 14, 2024

Hi @timtebeek , thanks for the review.

I reworked the PR to handle the Junit 4 assertion starting with a String message. The message should now stay in first place.
As suggested I separated the Junit5 and 4 matchers in the code. Support for assertArrayEquals and assertNull was added.

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.

Awesome! Thanks for the well tested minimal change here!

@timtebeek timtebeek marked this pull request as ready for review November 18, 2024 13:33
@timtebeek timtebeek merged commit b20da71 into openrewrite:main Nov 18, 2024
2 checks passed
@timtebeek timtebeek added the enhancement New feature or request label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

AssertionsArgumentOrder does not work with Junit 4 assertions
2 participants