-
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
Add JMockit's MockUp to Mockito migration #599
base: main
Are you sure you want to change the base?
Conversation
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
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.
Great addition @SiBorea ! I've pushed up a small change and left two initial comments; I'd need a little more time for a full review, but this is looking very promising.
Tagging @tinder-dthomson and @shivanisky for additional review (if possible), since they have more experience with JMockit migrations.
Awesome for working on this! |
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
@timtebeek @SiBorea hey folks! I am a bit unavailable at the moment as I'm currently caring for a one-week old newborn 😄 I'll try to take a look when I get some free time, but please don't depend on me for ongoing efforts for the time being! |
Congrats on new baby! |
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
Looking at the unit tests, we can't migrate Mockup directly to mock + when statements. Because Mockup is typically used to stub a subset of methods while the remaining methods behave as specified in the original class. https://javadoc.io/doc/org.jmockit/jmockit/latest/mockit/MockUp.html If we directly migrate to mock+when, then this will cause failures in all cases where a subset of methods are stubbed in the mockup. And this would be the most common outcome. I think best to use spy for non static. For static, private or final, may try PowerMockito - haven't tried this. But even this doesn't work in all cases and isn't a direct migration, but may cover most of the cases. I haven't seen enough MockUps to know. For direct migration Byte code injection/cglib may be needed. That would probably cover all the cases but it's a tough one. Maybe even worth going through jmockit source code and replicating what they are doing - probably byte code injection. I hope that the above makes sense. |
You could do it in the following way if you don't want to use byte code injection:
Will need to use doAnswer for multiline method bodies and anything that uses the method args ... as thenAnswer doesn't support void methods. Can use thenAnswer for non void, and doAnswer for void, or to simplify I think doAnswer supports both, may be better to just stick with that. Will need to handle the case for method params. This is very similar to migrating JMockit Delegate. To understand for the full migration, why byte code injection is needed JMockit allows this:
And can't be done without bytecode injection. Not sure if PowerMockito support this. |
There will be a possibility that multiple new object statements after MockUp. Using spy may have to insert code every occurrence. And I suppose most likely those stubbed methods will not be called (at lease in my project). MockConstruction is better under this circumstance. Powermock has been inactive for years, and has compatibility issues about JDK17 and high version of Mockito. Don't feel like a good solution. |
Do we have byte code injection example in OpenRewrite? Haven't seen it before |
I was referring to PowerMockito not PowerMock (hope I didn’t do a typo)! I
have no experience with this so I am unsure if it could be good.
MockUp is useful only for stubbing a subset of methods, otherwise no point
in using and better to use Jmockit Expectation for which we have already
have a lot of migration complete (not Delegate - your work to migrate the
methods to doAnswer can be reused in this case also!). Just one point which
may help, for doAnswer the syntax is a bit different than usual … its
doAnswer(…).when(obj).method() rather than when(obj.method()).thenAnswer
You could later on optimise it further where if the method has only return
one statement and doesn’t use method params can use a simple thenReturn.
I would be very surprised if I see code using MockUp when Expectation could
easily have been used instead. But I am not familiar with your codebase.
Probably best to support the most common cases first, and if it’s easy to
do the non common cases then sure, go ahead. But could support final
methods. Mockito 5 should be fine as to make your code compatible with
mockito 4 hopefully just need to add mockito-inline dependency as that got
merged into mockito 5.
Regarding private methods I’ll have to get back to you on that one.
I am unfamiliar with MockConstruction - will take a look and get back to
you.
…On Thu, 12 Sep 2024 at 10:45 PM, SiBorea ***@***.***> wrote:
There will be a possibility that multiple new object statements after
MockUp. Using spy may have to insert code every occurrence. And I suppose
most likely those stubbed methods will not be called (at lease in my
project). MockConstruction is better under this circumstance.
Nice suggestion to use doAnswer for both void and non void methods, will
try.
Powermock has been inactive for years, and has compatibility issues about
JDK17 and high version of Mockito. Don't feel like a good solution.
Furthermore, Mockito's claim about mocking private/final method sound to
me.
https://github.com/mockito/mockito/wiki/Mockito-And-Private-Methods
Do you think we should support these? It's a JMockit to Mockito 5 recipe.
—
Reply to this email directly, view it on GitHub
<#599 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BI27SFQN2JKVC4V6GVZ6EXTZWGEFXAVCNFSM6AAAAABN76V4QKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBWGE4DINRYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I think PowerMockito is the same as PowerMock, quote I guess people copied and pasted the MockUp code from somewhere in my case. I saw MockUp everywhere and no Expectation. |
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
…UpToMockitoTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…UpToMockitoTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…UpToMockitoTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
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.
Tests are looking very good. Just a few more comments - the main one being removal of the withSettings().defaultAnswer(CALLS_REAL...) as uneeded and to shorten some variable names and remove uneeded FQN, unless you think the FQN is needed. I'll take a look at anything remaining on my end.
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
I leave the fqn for those inner static classes from which other than the test class. But it's ok to remove, since such usage is very little. |
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Show resolved
Hide resolved
This CI build failed for PowerMockitoMockStaticToMockitoTest. Could you check? @timtebeek |
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Outdated
Show resolved
Hide resolved
import static org.openrewrite.java.tree.Flag.Private; | ||
import static org.openrewrite.java.tree.Flag.Static; | ||
|
||
public class JMockitMockUpToMockito extends Recipe { |
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.
Having a look at the overall design, I'm wondering if it would be much better to reuse the existing code in JMockitBlockToMockito, and add MockUp in the Preconditions, and then in JMockitBlockRewriter if the block type is a mockup, call much of the code of this class to rewrite the method by passing the the block, visitor and context. This however, would make it difficult to handle tearing down of any mock constructors if the teardown method is before the setup method so possibly a two cycle recipe would be needed, however it would probably reduce the number of lines of code due to the reuse.
class Bla {
@Before
void setup() {
// do some migration
}
@After
void teardown() {
// do some migration based on migration of setup method above - what if this teardown method is before the setup above
}
}
Also build needs to be green before submitting for review as the main build is green.
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.
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.
I cloned the latest code in main and built. Those cases are failed too. @timtebeek are they expected?
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
@Override | ||
public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { | ||
// Handle @Before/@BeforeEach mockUp | ||
Set<J.MethodDeclaration> mds = TreeVisitor.collect( |
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.
TreeVisitor.collect is marked @Incubating and is an experimental feature and may have breaking changes, is it possible to avoid this one?
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.
I think the final solution's code will very likely the same as TreeVisitor.collect()'s.
To me the functionality looks good, just have started to look at the main code:
It's quite a lot of code to review so adding back @timtebeek to the review process as the MockUp migration in unit tests looks right |
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockito.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/jmockit/JMockitMockUpToMockitoTest.java
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
What's changed?
Add JMockit MockUp to Mockito
What's your motivation?
Checklist