Add JMockit's MockUp to Mockito migration

[SiBorea]

What's changed?

Add JMockit MockUp to Mockito

What's your motivation?

• https://github.com/openrewrite/rewrite-testing-frameworks/issues/325

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

[shivanisky]

Awesome for working on this!

[tinder-dthomson]

@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!

[shivanisky]

Congrats on new baby!

[shivanisky]

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.

[shivanisky]

You could do it in the following way if you don't want to use byte code injection:

1. If in the MockUp, only static methods, then use MockStatic as you have already
2. If non static methods, use spy only and the standard Mockito when
3. If a mix, use both above - not sure if it will work, but one can hope.

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:

new MockUp<Bla> {
    void bla() {
    // custom
   }
}


// so where deep in the dev code this instance is created 
Bla bla = new Bla();
bla.bla(); // this will run custom above 

And can't be done without bytecode injection. Not sure if PowerMockito support this.

[SiBorea]

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.

[SiBorea]

Do we have byte code injection example in OpenRewrite? Haven't seen it before

[shivanisky]

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 https://github.com/openrewrite/rewrite-testing-frameworks/pull/599#issuecomment-2346184680, or unsubscribe https://github.com/notifications/unsubscribe-auth/BI27SFQN2JKVC4V6GVZ6EXTZWGEFXAVCNFSM6AAAAABN76V4QKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBWGE4DINRYGA . You are receiving this because you were mentioned.Message ID: @.***>

[SiBorea]

I think PowerMockito is the same as PowerMock, quote Basically, PowerMock provides a class called "PowerMockito" for creating mock/object/class and initiating verification, and expectations, everything else you can still use Mockito to setup and verify expectation (e.g. times(), anyInt()).

I guess people copied and pasted the MockUp code from somewhere in my case. I saw MockUp everywhere and no Expectation.

[SiBorea]

I consulted my colleague and he said MockUp is used when A calls B calls C, and only want to mock C, which spy might not suit this case.

I pushed a commit that using doAnswer and your idea that use thenCallRealMethod for all methods that are not mocked.

[shivanisky]

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.

Regarding private methods, mockito is a real pain when it comes to this. In past I've just changed the modifier away from private for testability.

Maybe with Reflection can solve this issue.

[shivanisky]

I consulted my colleague and he said MockUp is used when A calls B calls C, and only want to mock C, which spy might not suit this case.

I pushed a commit that using doAnswer and your idea that use thenCallRealMethod for all methods that are not mocked.

Ah yes - that's one way to go about it ... I deleted that comment yesterday because I wasn't sure if it's a good idea, but if it helps - great!

Mockconstructor looks great as we don't need to do byte code injection to get into tightly coupled code. Combined with reflection (hope it works!) looks like this would be fully migrated!

Id still choose spy if code is not tightly coupled, but hard to tell maybe.

[shivanisky]

Also, this looks interesting - maybe can avoid reflection this way? https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/AdditionalAnswers.html#delegatesTo(java.lang.Object)

Can use with mockConstruction

But won't be able to maintain state, maybe reflection is better.

[SiBorea]

Also, this looks interesting - maybe can avoid reflection this way? javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/AdditionalAnswers.html#delegatesTo(java.lang.Object)

Can use with mockConstruction

But won't be able to maintain state, maybe reflection is better.

I think delegateTo is a substitution of mockConstruction, not a supplement. And yes, state will be a issue. I suppose maintaining state is one of the reason that using MockUp.

[SiBorea]

Furthermore, Mockito's claim about mocking private/final method sound to me. Wiki: Mockito And Private Methods (mockito/mockito) Do you think we should support these? It's a JMockit to Mockito 5 recipe.

Regarding private methods, mockito is a real pain when it comes to this. In past I've just changed the modifier away from private for testability.

Maybe with Reflection can solve this issue.

I do the same too. You mean using Method.setAccessible() for it?

[SiBorea]

@shivanisky @timtebeek Do you think this PR is ready for review now?

[shivanisky]

@shivanisky @timtebeek Do you think this PR is ready for review now?

Do you want to add the case for when in the MockUp a stubbed method is using the method arguments?

Another case is where the MockUp class contains its own state ie member fields ... would need to create an Answer anonymous inner class. More tricky if two methods are manipulating the same member field.

For this case last case could exclude from migrating as it's tricky and maybe uncommon?

[shivanisky]

In the past I’ve changed the dev code that was private to nothing (package private) manually.

Even if you method.setAccessible() … how would you invoke the when? If it’s possible via reflection, can try.

Otherwise can migrate as normal, and expect someone to change the modifier of the dev code as I did manually at times. Will cause compilation issues.

Alternatively can skip migrating mockup with private methods.

On Sat, 14 Sep 2024 at 1:21 PM, SiBorea @.***> wrote:

Furthermore, Mockito's claim about mocking private/final method sound to me. Wiki: Mockito And Private Methods (mockito/mockito) 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.

Regarding private methods, mockito is a real pain when it comes to this. In past I've just changed the modifier away from private for testability.

Maybe with Reflection can solve this issue.

I do the same too. You mean using Method.setAccessible() for it?

— Reply to this email directly, view it on GitHub https://github.com/openrewrite/rewrite-testing-frameworks/pull/599#issuecomment-2350810037, or unsubscribe https://github.com/notifications/unsubscribe-auth/BI27SFRSU3PD5JMPXRPS6CLZWOTUDAVCNFSM6AAAAABN76V4QKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJQHAYTAMBTG4 . You are receiving this because you were mentioned.Message ID: @.***>

[SiBorea]

Indeed. I prefer leave it to user determine. Just one more question, should we paste some comment to tell that why those private methods are ignored?

[shivanisky]

Indeed. I prefer leave it to user determine.

Just one more question, should we paste some comment to tell that why those private methods are ignored?

You can put it in the description of the recipe.

[SiBorea]

@timtebeek can you run the workflow again?

[SiBorea]

Feel like it produces random order of methods during unit test and causes result comparison failed

[shivanisky]

For this PR I believe you need to also add test cases to migrate methods of the MockUp which have parameters and use them inside the method. Or, at least not migrate them. This would prevent incorrect migration

Also mockedConstructor will not work for this case?

Mockup<Foo> {

@Mock

int method() {

     return 5;

}

}

TestClass tc = new TestClass(); // this creates Foo and executes method()

Better to use mockedConstrructionWithAnswer(delagatesTo(mockedObject)?

Also, if the mockup is defined in setup method of test @.***/BeforeAll), then the close of the mockedConstructor may need to happen in teardown @AfterEach etc . This may be an uncommon scenario though. Mockup migration is very tricky and you’ve got a great start here.

On Sat, 21 Sep 2024 at 1:11 AM, SiBorea @.***> wrote:

@timtebeek https://github.com/timtebeek can you run the workflow again?

— Reply to this email directly, view it on GitHub https://github.com/openrewrite/rewrite-testing-frameworks/pull/599#issuecomment-2363958622, or unsubscribe https://github.com/notifications/unsubscribe-auth/BI27SFUZMV7JFAWP5GUZKTDZXQ3J7AVCNFSM6AAAAABN76V4QKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRTHE2TQNRSGI . You are receiving this because you were mentioned.Message ID: @.***>

[SiBorea]

For this PR I believe you need to also add test cases to migrate methods of the MockUp which have parameters and use them inside the method. Or, at least not migrate them. This would prevent incorrect migration Also mockedConstructor will not work for this case? Mockup<Foo> { @mock int method() { return 5; } } TestClass tc = new TestClass(); // this creates Foo and executes method() Better to use mockedConstrructionWithAnswer(delagatesTo(mockedObject)? Also, if the mockup is defined in setup method of test @./BeforeAll), then the close of the mockedConstructor may need to happen in teardown @AfterEach etc . This may be an uncommon scenario though. Mockup migration is very tricky and you’ve got a great start here. … On Sat, 21 Sep 2024 at 1:11 AM, SiBorea @.> wrote: @timtebeek https://github.com/timtebeek can you run the workflow again? — Reply to this email directly, view it on GitHub <#599 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/BI27SFUZMV7JFAWP5GUZKTDZXQ3J7AVCNFSM6AAAAABN76V4QKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRTHE2TQNRSGI . You are receiving this because you were mentioned.Message ID: @.***>

Yes to test cases and tearDown. What do you mean by "this creates Foo and executes method()"?

[shivanisky]

Ah - that would have been hard to read, was using gmail to reply. In the below case, mockConstruction will not work, need to use mockConstructionWithAnswer(delegatesTo(mockedObject)). Also useful when migrating PowerMock whenNew to Mockito.

   class Foo {
        int method() {
            return 0;
        }
    }

    class TightlyCoupled {
        final int num;

        TightlyCoupled() {
            this.num = new Foo().method();
        }
    }

    @Test
    void test() {
        new MockUp<Foo>() {
            @Mock
            int method() {
                return 5;
            }
        };
        TightlyCoupled tc = new TightlyCoupled();
        assertEquals(5, tc.num);
    }

[shivanisky]

Sorry for the more comments - found a way for you to avoid reflection and minimise the code changes. Using the example code from previous comment:

Foo f = mock(Foo.class, withSettings().defaultAnswer(CALLS_REAL_METHODS);
try(mockedConstructorWithAnswer(delegatesTo(f))) {
   when(f.method()).thenReturn(5);
   TightlyCoupled tc = new TightlyCoupled();
   assertEquals(5, tc.num);
}

I believe the above is a direct migration of MockUp and handles all the scenarios.

Actually some more googling found an alternative solution. Not sure which one is better.

try(MockedConstruction<PaymentService> mockPaymentService = Mockito.mockConstruction(PaymentService.class,(mock,context)-> {
        when(mock.processPayment()).thenReturn("Credit");
    })){
        PaymentProcessor paymentProcessor = new PaymentProcessor();
        Assertions.assertEquals(1,mockPaymentService.constructed().size());
        Assertions.assertEquals("Credit", paymentProcessor.processPayment());
    }
}

Just need to figure out how to call real methods in above.

https://www.baeldung.com/java-mockito-constructors-unit-testing

[SiBorea]

Thanks for the solutions, will give them a try. I am now struggling to handle the beforeEach and tearDown problem

[shivanisky]

Glad that helped! Regarding the setup method, could do it in a separate small PR, and for now just exit if it has that annotation. If you really want to do it now, I'd imagine the hardest part is adding a teardown method when one doesn't exist, which Tim would know about.

You would want to use try with resources or try finally as shown in previous comment of mine as if an exception is thrown then the close on the mockConstruction object will not be called. This will cause test failures for other tests running on the same thread that do not expect that mock object.

Following things are remaining:

1. Use try with resources or try-finally with delegatesTo and mocked object, and remove reflection
2. Support for method parameters https://www.baeldung.com/mockito-doanswer-thenreturn. This is somewhat easy to do. For each method parameter, in the beginning of the doAnswer or thenReturn, do ParameterType parameterName = invocation.getArgument(i). Then run this recipe in the doAfterVisit to remove unused local variables: https://docs.openrewrite.org/recipes/staticanalysis/removeunusedlocalvariables
3. Avoid migrating setup methods for now?

[SiBorea]

Glad that helped! Regarding the setup method, could do it in a separate small PR, and for now just exit if it has that annotation. If you really want to do it now, I'd imagine the hardest part is adding a teardown method when one doesn't exist, which Tim would know about.

You would want to use try with resources or try finally as shown in previous comment of mine as if an exception is thrown then the close on the mockConstruction object will not be called. This will cause test failures for other tests running on the same thread that do not expect that mock object.

Following things are remaining:

1. Use try with resources or try-finally with delegatesTo and mocked object, and remove reflection
2. Support for method parameters baeldung.com/mockito-doanswer-thenreturn. This is somewhat easy to do. For each method parameter, in the beginning of the doAnswer or thenReturn, do ParameterType parameterName = invocation.getArgument(i). Then run this recipe in the doAfterVisit to remove unused local variables: docs.openrewrite.org/recipes/staticanalysis/removeunusedlocalvariables
3. Avoid migrating setup methods for now?

1. I suppose we can have another PR for this one
2. Nice suggestion, will do today
3. It's almost done. Just need a little fix for unit test now

[SiBorea]

@shivanisky Pls have another look at it