-
Notifications
You must be signed in to change notification settings - Fork 69
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 JMockit block to Mockito which contains redundant "this" prefix for arg matcher and migrate array as parameter for any #610
Conversation
Many thanks for the continued improvements here! I'm traveling right now so it's a bit harder to do a detailed review. Perhaps @shivanisky can chime in? (No requirement of course!) |
Weekend is looking busy but I'll try to look if I get free. Glad to see this work is helping These code changes affect not just JMockit Verifications but Expectations and NonStrictExpectations migration as well, essentially the entire JMockit to Mockito migration. |
src/test/java/org/openrewrite/java/testing/jmockit/JMockitVerificationsToMockitoTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitVerificationsToMockitoTest.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/ArgumentMatchersRewriter.java
Outdated
Show resolved
Hide resolved
…nd without "this"
…ntMatchersRewrite.java affects Expectations too
@timtebeek Could you please kindly have a look at this pull-request? Thank you. |
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.
Thanks a lot for these fixes @yurii-yu ! Good to have those this.
cases covered as well, and thanks for the help reviewing @shivanisky 🙏🏻
What's changed?
Be able to cope with corner cases in refactoring JMockit Verifications.
anyString
in JMockit should be refactored toanyString()
in Mockito, so as toanyInt
oranyLong
. But when there are redundantthis
beforeanyString
, the code will not be processed because OpenWrite treatanyString
as anJ.Identifier
object where asthis.anyString
as aJ.FieldAccess
object. My patch will do a type cast for this exact case.(Object) any
in Mockito should be refactored toany(Object.class)
in Mockito. This works for simple Object. But whenObject
is an array such as(byte[]) any
, OpenRewrite will not change it toany(byte[].class)
because the class type ofbyte[]
is notJavaType.FullyQualified
. I added processing branch for this case.Corresponding unit tests were also added.
What's your motivation?
We encountered problem in dealing with several legacy projects.
Some codes can not be refactored automatically, while manually changing them seems tedious, since they are not complex at all. We located and solved this problem, which can save us quite a lot of time.
Benefited a lot from the OpenRewrite project previously, we believe that more people and more legacy projects should be able to take advantage of it.
Anything in particular you'd like reviewers to focus on?
My solution for the second question is obviously not the perfect one, because the element type of array could be another array so theoretically this could be a infinite loop. Solving such problem will take me much more time and energy.
But I guess that is a rare case which is almost impossible to encounter, so my code simply assume that the element type of the array is plain object.
Anyone you would like to review specifically?
@timtebeek Please have a look again on my code, amigo :)
Have you considered any alternatives or workarounds?
Yes, as I mentioned above, I think a perfect solution for the second question is unworthy.
Any additional context
Checklist