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

PowerMock and MockMaker: API change request. #1110

Closed
thekingn0thing opened this issue Jun 6, 2017 · 99 comments
Closed

PowerMock and MockMaker: API change request. #1110

thekingn0thing opened this issue Jun 6, 2017 · 99 comments

Comments

@thekingn0thing
Copy link

@thekingn0thing thekingn0thing commented Jun 6, 2017

Hi guys,

I've taken a time for reviewing PowerMock code to find place where it use internal Mockito API. I'd like to discuss what could be done to avoid.

First think that I'd like to talk about it's MockMaker. PowerMock uses it custom MockMaker, which it past was used only for two things:

  • cache a class create by CgLib
  • return fake InternalMockHandler for static mocks.

The first case is no more actual, because ByteBuddy uses current context ClassLoader, so MockClassLoader is used. But the second case still actual. But investigation shows that returning fake InternalMockHandler is required only for one case right now.

Method org.mockito.internal.exceptions.Reporter.noMoreInteractionsWanted(Invocation undesired, List<VerificationAwareInvocation> invocations) tries to safelyGetMockName(undesired.getMock()). Finally, call comes to org.mockito.internal.util.MockUtilisMock(Object mock). It tries to get MockHandler for mock, but it mock is static and it's not a PowerMockMaker, then null is returned.
I was surprised that having PowerMockMaker is required only for such thing. If speak, honestly, I don't have any idea, how it can be fixed on Mockito site. Maybe you have any thoughts?

The second point, it's a way how Mockito loads plugins. We had some discussion within #1006. Main point provided @podarsmarty (as for me main) that if there are two files org.mockito.plugins.MockMaker in class path, then order of loading plugins are unpredictable. Such undetermined behaviour could introduce some fluky bugs, when for example one plugin specified in project and other in dependency jar. And for example it works, but later author of dependency jar make refactoring and change package or something like this. As result other plugin is loaded by Mockito and tests start failed.

For PowerMock it is issue, because is has own MockMaker, which is required only if test runs with PowerMock. But, unfortunately, if PowerMock in class path it will be used always. If a developer wants to use mock-maker-inline to be able to mock final without PowerMock, then it was impossible until PowerMock 1.7.0, where I added ability to specify MockMaker to which PowerMockMaker delegates calls. As for me, it will be good to have ability to separate custom MockMaker and Mockito build-in MockMaker. So custom MockMaker could know which build-in MockMaker should be used to delegate call if its required.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 7, 2017

@thekingnothing, do you live in Krakow? I spent most of my adult life in Krakow ;)

Thank you for reaching out to us so that we can work together. That's how this should be done - we design the APIs between open source components so that our users get higher quality products (e.g. products that don't break on version upgrade :).

I'm also surprised that those are the only issues with using Mockito's private API. Something tells me there is more. Let's keep working on this an expose all integration points.

Method org.mockito.internal.exceptions.Reporter.noMoreInteractionsWanted(Invocation undesired, List invocations) tries to safelyGetMockName(undesired.getMock()). Finally, call comes to org.mockito.internal.util.MockUtilisMock(Object mock). It tries to get MockHandler for mock, but it mock is static and it's not a PowerMockMaker, then null is returned.
I was surprised that having PowerMockMaker is required only for such thing. If speak, honestly, I don't have any idea, how it can be fixed on Mockito site. Maybe you have any thoughts?

Multiple methods from Reporter class attempt to use 'safelyGetMockName'. I assume you use 'noMoreInteractionsWanted' as an example. Getting mock name is only used to make verification errors cleaner. It's not a critical feature, without it the errors are still decent.

To get started, I suggest to hack Mockito code and return empty String if getting mock name throws an exception. This way you can push forward and identify other contention points. Once we get full picture we can design public API that will solve all use cases cases for integration with PowerMock.

The second point, it's a way how Mockito loads plugins. We had some discussion within #1006. Main point provided @podarsmarty (as for me main) that if there are two files org.mockito.plugins.MockMaker in class path, then order of loading plugins are unpredictable. Such undetermined behaviour could introduce some fluky bugs, when for example one plugin specified in project and other in dependency jar. And for example it works, but later author of dependency jar make refactoring and change package or something like this. As result other plugin is loaded by Mockito and tests start failed.

I totally agree that random order of loading plugins is undesired. We don't have a clean way of solving this problem at the moment. It needs more brainstorming. Ideas are welcome :)

For PowerMock it is issue, because is has own MockMaker, which is required only if test runs with PowerMock. But, unfortunately, if PowerMock in class path it will be used always. If a developer wants to use mock-maker-inline to be able to mock final without PowerMock, then it was impossible until PowerMock 1.7.0, where I added ability to specify MockMaker to which PowerMockMaker delegates calls. As for me, it will be good to have ability to separate custom MockMaker and Mockito build-in MockMaker. So custom MockMaker could know which build-in MockMaker should be used to delegate call if its required.

This is an interesting use case. Let me understand it: an author of custom MockMaker would like to have access to default Mockito MockMaker so that she can delegate some invocations to it? In Mockito we can introduce public API to expose the default mock maker (or means to obtain the default mock maker).

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 8, 2017

@thekingnothing, do you live in Krakow? I spent most of my adult life in Krakow ;)

Yeah, I'm living in Krakow for one year. Nice city, except winter time. I mean issue with air in winter :)

I'm also surprised that those are the only issues with using Mockito's private API. Something tells me there is more.

It's just begin, I only started (demon laughing). There a lot of issue and thing todo. PowerMock has been being developed for 9 years in isolation and there are a lot of hacks (sometimes really dirty). I started with easiest issue. All other could take more time to explain and fix: directly creating mocks, registration invocation control, creating mock handler, verify static and so on.

Multiple methods from Reporter class attempt to use 'safelyGetMockName'. I assume you use 'noMoreInteractionsWanted' as an example.

No, noMoreInteractionsWanted and noMoreInteractionsWantedInOrder it's only one case right now. All other methods which use safelyGetMockName is not called in context of static mocking. PowerMock handle all these cases by itself. As result Mockito mockMaker always can returns handler and mock name could be obtained.

I totally agree that random order of loading plugins is undesired. We don't have a clean way of solving this problem at the moment. It needs more brainstorming. Ideas are welcome :)

First of all it will be nice to show warning like JVM shows on Mac ;) A developer will be aware about issue. I clear understand that most of developers are ignoring such messages. But at least some of them will know that they test are unstable.

objc[48004]: Class JavaLaunchHelper is implemented in both /Library/Java/JavaVirtualMachines/jdk1.8.0_111.jdk/Contents/Home/bin/java (0x103ae94c0) and /Library/Java/JavaVirtualMachines/jdk1.8.0_111.jdk/Contents/Home/jre/lib/libinstrument.dylib (0x104b544e0). One of the two will be used. Which one is undefined.

In Mockito we can introduce public API to expose the default mock maker (or means to obtain the default mock maker).

Yeah, it will be create to have ability to obtain default mock maker (or other plugin) instead writing something like private final MockMaker cglibMockMaker = new CglibMockMaker();

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 10, 2017

No, noMoreInteractionsWanted and noMoreInteractionsWantedInOrder it's only one case right now. All other methods which use safelyGetMockName is not called in context of static mocking.

Ok, thanks! Hack Mockito codebase and keep pushing forward. Safely getting mock name is a simple issue to fix so let's move on and identify hard problems :)

First of all it will be nice to show warning like JVM shows on Mac ;)

We can show a warning. I'd like us to brainstorm and understand what options we have here to fix this caveat. This problem should be fairly common when plugin system is based of metadata declared in the classpath.

Yeah, it will be create to have ability to obtain default mock maker (or other plugin) instead writing something like private final MockMaker cglibMockMaker = new CglibMockMaker();

Does PowerMock still use CglibMockMaker? This class no longer exists in Mockito 2.x codebase.

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 10, 2017

Hack Mockito codebase and keep pushing forward.

Just to make sure that we're on the same page. Does you mean to raise a pull request with the change on reporter?

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 10, 2017

Does PowerMock still use CglibMockMaker? This class no longer exists in Mockito 2.x codebase.

PowerMock have used repackaged CglibMockMaker until version 1.7.0. I've dropped all repackaged Cglib code a few hours ago.
I was going to release PowerMock 1.7.0 on next week, but I think I'll postpone it until issue with saflyGetMockName will be fixed and released.
It will give me ability to delete PowerMockMaker and make code simple.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 11, 2017

Answers to your questions below :) Thank you for pushing this forward!

Just to make sure that we're on the same page. Does you mean to raise a pull request with the change on reporter?

I was hoping you can hack the code, test locally if things work, and expose harder problems. I need to think a little bit about what's the best way to solve the Reporter.

Are you sure that there are no other import "org.mockito.internal" in Powermock codebase?

PowerMock have used repackaged CglibMockMaker until version 1.7.0. I've dropped all repackaged Cglib code a few hours ago.

Got it. Thank you!

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 12, 2017

I have investigated Powermockito codebase and found out that the problem is that MockHandler API in Mockito is not quite finished. Therefore Powermockito had to create custom implementation of MockHandler despite our Javadoc saying:

This api is work in progress. Do not provide your own implementations.

(I'm not complaining, I very much sympathize. Many times I needed to resort to using internal API because I had no choice. Thanks again for engaging with us on cleaning the APIs between both libraries!)

@thekingnothing, if we make the MockHandler API public, will this work for you? I've initiated the efforts here: #1121 My initial thought is that after #1121 is applied, we can just move the InternalMockHandler methods onto MockHandler and delete InternalMockHandler. In Powermockito you can then create implementation of MockHandler that works with static mocking. How does it sound?

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 12, 2017

@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Jun 13, 2017

@thekingnothing Just a question to understand the "saflyGetMockName" problem: What is missing here on the mockito side or what is not working as you expect? Do you get null as mock name?

@szczepiq As far as I can see InternalMockHandler needs to be part of the public API since MockUtil.getMockHandler(T mock) returns this type. An other option would be to move its's methods to the super class.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 13, 2017

@szczepiq As far as I can see InternalMockHandler needs to be part of the public API since MockUtil.getMockHandler(T mock) returns this type. An other option would be to move its's methods to the super class.

Yup, that's the plan :) I was thinking of just moving those methods onto MockHandler and killing InternalMockHandler. Thanks for joining the discussion!

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 13, 2017

I'm sorry, I didn't have a time to answer earlier. I was driving a car to Prague and back. I'm still limited in access and writing from mobile phone. Sorry for some inaccuracies.

No worries. Hopefully you had good time in Prague!

As we discuss earlier, if you resolve an issue with saflyGetMockName, then there will be no reason to implement MockMaker and MockHandler by PowerMock. I'll remove PowerMockMaker as result PowerMockInternalMockHandler will be removed.

Making "saflyGetMockName" lenient is only good for short term experimentation. In the future, other features of Mockito will be accessing MockHandler in Powermockito static scenarios and it can cause failures. Let's come up with a proper solution :) Plus, if we make MockHandler public, Powermockito use will be better: we will actually print decent static mock name in verification errors.

Please give public MockHandler a second thought and get back to us with any feedback. You would be able to keep PowerMockInternalMockHandler, just make it extend public type MockHandler.

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 13, 2017

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 13, 2017

  • to create a mock, PowerMock uses MockMaker. But to get an instance of MockMaker I have to call Plugins.getMockMaker() - internal API call

We should be able to address this by adding public API for default MockMaker instance. Easy to address. I haven't put enough thought to it but it seems easy so let's move on to next problem.

  • Then to create a mock I have to pass a MockHandler to createMock. I found only one way to do it use MockHandlerFactory - one more internal use

Why do you need to create instances of MockHandlers? Can you point some code? Keep in mind that we are making MockHandler public so you should be able to provide your own implementation if needed.

  • to create MockHandler the settings is required. Everything is pretty good with Mockito.withSettings, except that I need to set mock type.

Why setting mock type is problematic? Is it for static mocking scenario? Can you point some code?

  • the instance of MockHandler is used to create MockitoMethodInvocationControl. This class is full of internal calls and dirty hacks.

:) we will resolve them all :)

Nice progress!

I suggest we do this: I will push to my branch Mockito with cleaned up MockHandler API. You can build Mockito jar from source, from that branch and see how it fits Powermockito use case. Let's see how much internal API we can resolve by cleaning up MockHandler API. I will make changes on "sf" branch by the end of today.

Thanks again for working on this!

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 14, 2017

I have two concerns regarding keeping PowerMockInternalMockHandler

  1. It requires implementing PowerMock and leads to issue with mock-maker-inline.
  2. PowerMock never use PowerMockInternalMockHandler by itself. only for handling case "static mock name`

I'm entering on thin ice. If I choice to keep PowerMockInternalMockHandler and have more logic in it, I have to keep in mind that PowerMock works with EasyMock. As I see, I have two options: have better integration with Mockito, but duplicate logic for EasyMock and Mockito. Or implement some general things same for EasyMock and Mockito, but not use full power integration with Mockito.

Why do you need to create instances of MockHandlers? Can you point some code? Keep in mind that we are making MockHandler public so you should be able to provide your own implementation if needed.

The instance is used later to handle static/non-public method call by MockitoMethodInvocationControl. MockitoMethodInvocationControl implements MethodInvocationControl, MethodInvocationControl is a proxy for Mockito and EasyMock API.

When method is called, PowerMock looks for MethodInvocationControl for the method's signature and delegates handling call to MethodInvocationControl. In Mockito case, MockitoMethodInvocationControl uses MockHandler to handle method invocation.


Invocation invocation = new InvocationImpl(
            interceptionObject,
            new DelegatingMethod(method),
            arguments,
            SequenceNumber.next(),
            cleanTraceRealMethod,
            new LocationImpl()
        );
        
        try {
            return mockHandler.handle(invocation);
        } catch (NotAMockException e) {
            if (invocation.getMock()
                          .getClass()
                          .getName()
                          .startsWith("java.") && MockRepository.getInstanceMethodInvocationControl(invocation.getMock()) != null) {
                return invocation.callRealMethod();
            } else {
                throw e;
            }
        } catch (MockitoAssertionError e) {
            InvocationControlAssertionError.updateErrorMessageForMethodInvocation(e);
            throw e;
        }

source

Why setting mock type is problematic?

MockSettings does not have public method to set type.

Is it for static mocking scenario? Can you point some code?

I have to check when it is required.


       final MockSettingsImpl<T> settings;
       final MockMaker mockMaker = getMockMaker();
       
       if (mockSettings == null) {
           settings = (MockSettingsImpl) Mockito.withSettings();
       } else {
           settings = (MockSettingsImpl) mockSettings;
       }
       
       if (isSpy) {
           settings.defaultAnswer(Mockito.CALLS_REAL_METHODS);
       }
       
       settings.setMockName(new MockNameImpl(mockName));
       //settings.setTypeToMock(type);
       
       InternalMockHandler mockHandler = MockHandlerFactory.createMockHandler(settings);
       
       T mock = mockMaker.createMock(settings, mockHandler);

source

Please give public MockHandler a second thought and get back to us with any feedback. You would be able to keep PowerMockInternalMockHandler, just make it extend public type MockHandler.
I suggest we do this: I will push to my branch Mockito with cleaned up MockHandler API. You can build Mockito jar from source, from that branch and see how it fits Powermockito use case. Let's see how much internal API we can resolve by cleaning up MockHandler API. I will make changes on "sf" branch by the end of today.

Anyway, I'm going to focus on release in near few days. I have to clear documentation for the release. After release, I'll build PowerMock with your change and check.

Thank you for providing me help with it! :)

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 15, 2017

Thank you for all the information - it is super useful! I will work on the MockHandler API (this needs to be sorted out anyway). Later, I will reply with more details to your last comment.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 18, 2017

I'm making progress on exposing MockHandler API but it is taking more time than I have anticipated. Hah, I love engineering :D !!!

I'm entering on thin ice. If I choice to keep PowerMockInternalMockHandler and have more logic in it, I have to keep in mind that PowerMock works with EasyMock. As I see, I have two options: have better integration with Mockito, but duplicate logic for EasyMock and Mockito. Or implement some general things same for EasyMock and Mockito, but not use full power integration with Mockito.

I suspect that own implementation of MockHandler unavoidable. MockUtil.getMockHandler() is used in bunch of other places and fixing the Reporter case with mock name will be a very short term solution.

Let's push forward with making MockHandler public. I suspect that once this API is neatly public, you should be able to implement PowerMockMaker that supports static mocking scenarios. I'll keep you posted on the progress!

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 18, 2017

@thekingnothing, how can I try out Powermockito? When I clone the repo and import it to IDEA, tests don't work in IDEA. This makes it hard to debug. I love debugging to learn how stuff works. Can you tell me how do I import Powermockito code to IDEA so that I can run some static mocking testing with Powermockito? Thanks in advance :)

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 19, 2017

I suspect that own implementation of MockHandler unavoidable. MockUtil.getMockHandler() is used in bunch of other places and fixing the Reporter case with mock name will be a very short term solution.

Okay, I'm throwing in the towel :) I'll keep own implementation of MockHandler. in this case, it's hight important have ability use PluginLoader to prevent issues like powermock/powermock#811

@thekingnothing, how can I try out Powermockito? When I clone the repo and import it to IDEA, tests don't work in IDEA. This makes it hard to debug. I love debugging to learn how stuff works. Can you tell me how do I import Powermockito code to IDEA so that I can run some static mocking testing with Powermockito?

What issue did you have? Have you tried to build it with gradle? I've just cloned repository from master and imported to IntelliJ Idea as Gradle project and was able to run static tests.

Anyway, it was a tricky moment that modules depended on mockito had two test/source sets that pointed to same java source path. It was done to be able to test same code against mockito 1.x and mockito 2.x I've created a new branch mockito2 where I dropped supporting Mockito 1.x and removed all complicated logic from Gradle. Could you try with the branch?

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 20, 2017

I've created a new branch mockito2 where I dropped supporting Mockito 1.x and removed all complicated logic from Gradle. Could you try with the branch?

I will try it. Thank you!

Okay, I'm throwing in the towel :) I'll keep own implementation of MockHandler. in this case, it's hight important have ability use PluginLoader to prevent issues like powermock/powermock#811

Ok :) BTW. I made progress in my branch but it's late and will check in the morning what's the status. I think I managed to expose the API so that Powermockito can stop depending on the internal API wrt to the MockHandler.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 20, 2017

@thekingnothing, can you try out building Mockito from "sf" branch? "./gradlew build" produces binaries, "./gradlew publishToMavenLocal" builds and installs binaries to local maven repo. Things I changed:

  • MockHandler API is public and you can provide your own implementation :) Just don't provide your own implementation of mockHandler.getInvocationContainer()
  • you can create an instance of MockCreationSettings using a new public API: "Mockito.withSettings().name("foo").build()".

Let us know how it goes!

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 20, 2017

@szczepiq, thank you!

I built Mockito from branch and tried to run(after all compile issues were resolved) and one major issue appeared.
PowerMockitoStubberImpl has the method addAnswersForStubbing which use deleted MockHandler.setAnswersForStubbing.

    @SuppressWarnings("unchecked")
    private void addAnswersForStubbing(MockitoMethodInvocationControl invocationControl) {
        final MockHandler mockHandler = invocationControl.getMockHandler();
        final List list = Whitebox.getInternalState(this, List.class);
        try {
            Whitebox.invokeMethod(mockHandler, "setAnswersForStubbing", list);
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }

What do you think better to do in this case?

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 20, 2017

you can create an instance of MockCreationSettings using a new public API: "Mockito.withSettings().name("foo").build()".

Great! Now, PowerMockMaker has been cleared from internal imports

import org.mockito.Mockito;
import org.mockito.invocation.Invocation;
import org.mockito.invocation.InvocationContainer;
import org.mockito.invocation.MockHandler;
import org.mockito.mock.MockCreationSettings;
import org.mockito.plugins.MockMaker;
import org.powermock.configuration.GlobalConfiguration;

But I have to cast to MockSettingsImpl still in DefaultMockCreator, because if a mockSettings is specified I have two options:

  • implement logic of cloning mockSettings. But it also requires using internal Mockito's classes
  • set name and type to existing mockSettings

Is it possible to add method to builder creating mockSetting from existing settings?

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 21, 2017

But I have to cast to MockSettingsImpl still in DefaultMockCreator, because if a mockSettings is specified I have two options:

I need to understand the use case more. Can you point out to the code?

set name and type to existing mockSettings

This seems like a reasonable option. Would this work for you?

Happy we are making progress!

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 21, 2017

I need to understand the use case more. Can you point out to the code?

Example from the DefaultMockCreator. I didn't commit code after refactoring, but it's very close to old code.

    private static <T> MockSettingsImpl<T> getMockSettings(final String mockName, final Class<T> type, final boolean isSpy, final MockSettings mockSettings) {
        final MockSettingsImpl<T> settings;
        if (mockSettings == null) {
            settings = (MockSettingsImpl) Mockito.withSettings();
        } else {
            settings = (MockSettingsImpl) mockSettings;
        }
        
        if (isSpy) {
            settings.defaultAnswer(Mockito.CALLS_REAL_METHODS);
        }
        
        settings.setMockName(new MockNameImpl(mockName));
        settings.setTypeToMock(type);
        return settings;
    }
@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Jun 24, 2017

Is it possible to add method to builder creating mockSetting from existing settings?

There are 2 separate asks here:

  1. Create settings object conveniently, with the type
  2. Creating settings object from other settings object (e.g. copy)

I fully understand the use case behind 1) - in static mocking scenario you need custom mock handler with custom mock name. I don't understand why we need 2) and the code sample does not explain why. I hope you don't really need 2) given 1) is implemented ;)

For 1), you can already do it with the code that I pushed to the branch, new "build" public method: withSettings().build(Foo.class)

Let us know how things are and what your progress is!

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Jun 27, 2017

For 1), you can already do it with the code that I pushed to the branch, new "build" public method: withSettings().build(Foo.class)

I change code in branch and use a new API. It helps clear PowerMockMaker from internals calls.

Where are a least two reason why the 2) is asked:

  1. Set default answer if PowerMockito.spy(Class<T> type) or PowerMockito.spy(T object) (this part can be refactored, if take into account that public API does not provide ability to create spy with settings)
  2. NPE exception in SubclassByteBuddyMockMaker and InlineByteBuddyMockMaker if mock type is not set, but a user provides custom settings via mockStatic(Class<?> classToMock, MockSettings mockSettings) for example.
java.lang.NullPointerException
	at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.prettifyFailure(SubclassByteBuddyMockMaker.java:90)
	at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.createMockType(SubclassByteBuddyMockMaker.java:77)
	at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.createMock(SubclassByteBuddyMockMaker.java:42)
	at org.powermock.api.mockito.mockmaker.PowerMockMaker.createMock(PowerMockMaker.java:50)
	at org.powermock.api.mockito.internal.mockcreation.DefaultMockCreator.createMethodInvocationControl(DefaultMockCreator.java:116)
90:     if (mockFeatures.getTypeToMock().isArray()) {
            throw new MockitoException(join(
                    "Mockito cannot mock arrays: " + mockFeatures.getTypeToMock() + ".",
                    ""
                    ), generationFailed);
        }
@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 17, 2017

I've checked behaviour with inline-mock-maker and tests are passed.

I think, you may start finalising public API and cleaning code. I'll do required changes on PowerMock side and then also clean and a bit refactor code.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 17, 2017

I've checked behaviour with inline-mock-maker and tests are passed.

Great!

I think, you may start finalising public API and cleaning code.

Will do!

I'll do required changes on PowerMock side and then also clean and a bit refactor code.

Can you make sure that none of the Powermock code imports "org.mockito.internal." packages? Internal classes, e.g. classes that reside underneath "org.mockito.internal." can change without notice with any version of Mockito, those classes are not considered public API. In current "mockito2" branch in Powermock I still references to internal Mockito classes.

Do I need to provide you public API to get hold of the default implementation of MockMaker (inline or not)?

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 18, 2017

Can you make sure that none of the Powermock code imports "org.mockito.internal." packages? Internal classes, e.g. classes that reside underneath "org.mockito.internal." can change without notice with any version of Mockito, those classes are not considered public API. In current "mockito2" branch in Powermock I still references to internal Mockito classes.

I can find several places where org.mockito.internal is mentioned in code:

  1. Annotation processing - will be rewritten, not an issue
  2. Checking if method was called - as I'll change it as we discuss
  3. Several test - some of them will be refactored, for some of them it's not a big deal because string org.mockito.internal is used just as test data
  4. Configuration - using mock-maker-inline as alias org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker - it has to be changed maybe.

Do I need to provide you public API to get hold of the default implementation of MockMaker (inline or not)?

No, I don't need a public API for getting an instance of MockMaker. But it will be useful to get class of default MockMaker or class base on alias.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 18, 2017

Thank you for reviewing the code!

No, I don't need a public API for getting an instance of MockMaker. But it will be useful to get class of default MockMaker or class base on alias.

I see. Let me know if you need the instance. Exposing implementation class name is something I would like to avoid because the implementation class is internal and we want the flexibility of replacing it if needed. I can provide you the instance, but if you don't need the instance, I'd rather implement as little new public API as possible.

That said, I do believe that you need public API from Mockito to give you the instance of default MockMaker :) Please confirm

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 18, 2017

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 18, 2017

From our conversation and my understanding how Powermock works, it looks like you need access to default MockMakers ;) I'll provide API for that. I think it is a reasonable addition to Mockito public API.

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 18, 2017

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 18, 2017

Main idea, is that Android developers, especially Kotlin developers, want to use PowerMock for mocking static methods and Mockito for any other purpose like mocking final classes. But Mockito allows to declare only one MockMaker in class path. So if PowerMock put the file for in jar as it is done now, then a developer cannot declare and use inline MockMaker.

@podarsmarty
Copy link

@podarsmarty podarsmarty commented Aug 18, 2017

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 18, 2017

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 18, 2017

I've added new public api and changed existing API in my experimental branch 'sf'. Please try it out before I start polishing the code further. Here're the changes I made:

//for MockMakers:
Mockito.framework().getPlugins().getDefaultPlugin(MockMaker.class);
Mockito.framework().getPlugins().getInlineMockMaker();

//changed a little bit how Invocations are created:
Mockito.framework().getInvocationFactory().createInvocation(...);

Please confirm that this is all we need to get rid of all "org.mockiton.internal" from Powermock codebase.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 18, 2017

Exciting :) We're almost done!

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Aug 19, 2017

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 19, 2017

Thank you! I agree completely. I am also looking forward to the happy ending :)

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 20, 2017

Please confirm that this is all we need to get rid of all "org.mockiton.internal" from Powermock codebase.

I confirm. I've just push changes where "org.mockiton.internal" is used only in one place. It is annotation processing. As I mentioned earlier, I'm going create a additional task for it and completely rewrite this part, because EasyMock version also have such problem. I'm going to create a one annotation engine for Mockito and EasyMock.

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 20, 2017

I've started code clean up and one interesting idea came to my mind. If I'm able to create a custom implementation of MockitoSettings, then I'll implement all mock creating logic in PowerMockCreator instead using custom class that uses Mockito.mock.

In this case, PowerMockito could delegate all calls to Mockito with custom MockitoSettings. What do you think?

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 20, 2017

I confirm. I've just push changes where "org.mockiton.internal" is used only in one place.

Fantastic!!!

I've started code clean up and one interesting idea came to my mind. If I'm able to create a custom implementation of MockitoSettings, then I'll implement all mock creating logic in PowerMockCreator instead using custom class that uses Mockito.mock.

Do you mean something like that:

//instead:
PowerMockito.mockStatic(MyClass.class, Mockito.withSettings().name("foo"));
//you would do:
Mockito.mock(MyClass.class, PowerMockito.withSettings().staticMock().name("foo"));

Very interesting! Can you scope this and list down the pros & cons of both approaches?

Cons:

  • Mockito teams considers adding new methods to MockSettings as compatible changes because we don't recommend MockSettings to be subclassed. We need that ability, otherwise we would have to ship a major version of Mockito every time we add a new method to "Mockito" or "MockSettings" classes. If you provide custom interface that extends MockSettings you might be exposed to incompatible changes with minor versions of Mockito. We could potentially discuss this point and figure out a solution that would work for Powermockito.
  • New API would be different from what your users are accustomed to and might be more problematic to roll out, discourage from upgrading. Users still have to know about Powermockito static method entry point, regardless if it is PowerMockito.mock or PowerMockito.withSettings

Prons (I'm not sure, you can help out with this):

  • Are you looking to remove Powermockito class and have less code to maintain? Is it the goal that sparked this idea?
  • It would be cool to unify the API so that to use Powermockito the one uses normal Mockito API and drops some annotation to the class or method to signify to use Powermockito behind the hood. It does seem like a lot of work, though.

Hope that helps!

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 27, 2017

Are you looking to remove Powermockito class and have less code to maintain? Is it the goal that sparked this idea?

No, I'm not going to drop PowerMockito, because is has API for new and private code mocking. I thinking about putting code that create mock in one place. Right now the creating mock logic is spread between at least two classes: PowerMockMaker and DefaultMockCreator. First implements Mockito's interface and has logic with handler and so on, the second is use to create mock by PowerMock and validate mock (duplicates TypeMockability).

Do you mean something like that:

//instead:
PowerMockito.mockStatic(MyClass.class, Mockito.withSettings().name("foo"));
//you would do:
Mockito.mock(MyClass.class, PowerMockito.withSettings().staticMock().name("foo"));

It's one of the option. PowerMockito will still have mockStatic as shortcut for PowerMockito.withSettings().staticMock().

It would be cool to unify the API so that to use Powermockito the one uses normal Mockito API and drops some annotation to the class or method to signify to use Powermockito behind the hood. It does seem like a lot of work, though.

I agree, that it will be nice to have PowerMock as plugin to Mockito for mocking cases such as mock static, system and new. All these 3 cases could be covered by using Java Agent and class redefining with using ByteBuddy and without using custom class loader.

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 27, 2017

I think that we can stop for now. Make Mockito release with new API, first beta PowerMock 2.0 and continue discuss feature change. We have achieved our goal - removing using Mockito private API.

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Aug 27, 2017

I have raised a pull request powermock/powermock#837 to merge all change and prepare for release. I'm waiting only while Mockito code will be cleaned and released.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 27, 2017

Sounds good. We're on it!

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Aug 28, 2017

I made first pass on the changes and the public API. Looks good, we are not far from being ready to merge!

New public API

org.mockito.MockSettings#build
org.mockito.MockingDetails#getMockHandler
org.mockito.MockitoFramework#getPlugins
org.mockito.MockitoFramework#getInvocationFactory
Deprecated org.mockito.internal.InternalMockHandler
org.mockito.invocation.InvocationContainer
org.mockito.invocation.InvocationFactory
org.mockito.invocation.MockHandler#getMockSettings
org.mockito.invocation.MockHandler#getInvocationContainer
org.mockito.NotExtensible

TODO @szczepiq

  1. Verify Powermock needs it
  2. Add missing documentation / tests
  3. Add (@)Since tag
@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Sep 8, 2017

Mockito 2.10.0 has been published!!! Wooot!

Nice work everybody, it tooks us 3 months but we made it :) @thekingnothing let us know if you need something!

@mockitoguy mockitoguy closed this Sep 8, 2017
@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Sep 9, 2017

Yeah! It was a great job. @szczepiq thank you a lot for help! I'm preparing a first beta release with using Mockito public API.

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Sep 19, 2017

@thekingnothing, how are things? Were you able to push a stable release of Powermock with new version of Mockito? Do you need help? Is there a ticket in Powermock that we can track?

Cheers!

@thekingn0thing
Copy link
Author

@thekingn0thing thekingn0thing commented Sep 20, 2017

@szczepiq, I'm continue working on PowerMock 2.0.0 release, but it's too early to talk about a stable release. I focused on JDK 9.0 support in past two weeks which was required for Kafka and Maven Surefire (powermock/powermock#783).

I also dropped the first public beta with supporting new Mockito API and I'm collecting first issues, like powermock/powermock#853.

Yes, there is a ticket where all issues to fix/implement are listed
powermock/powermock#725

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.