-
Notifications
You must be signed in to change notification settings - Fork 73
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
src/test/java/org/openrewrite/java/testing/cleanup/AssertionsArgumentOrderTest.java
Outdated
Show resolved
Hide resolved
@@ -37,8 +37,21 @@ public class AssertionsArgumentOrder extends Recipe { | |||
new MethodMatcher("org.junit.jupiter.api.Assertions assertEquals(..)"), | |||
new MethodMatcher("org.junit.jupiter.api.Assertions assertNotEquals(..)"), | |||
new MethodMatcher("org.junit.jupiter.api.Assertions assertSame(..)"), | |||
new MethodMatcher("org.junit.jupiter.api.Assertions assertNotSame(..)"), | |||
new MethodMatcher("org.junit.jupiter.api.Assertions assertArrayEquals(..)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant with L36
There was a problem hiding this 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
…pport for junit4 assertArrayEquals and assertNull.
62681cb
to
8c41ec2
Compare
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. |
What's changed?
Added support for Junit 4 matchers in the
AssertionsArgumentOrder
recipe :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