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

Improve and develop APIs required for framework integrators #1121

Merged
merged 41 commits into from Sep 8, 2017
Merged

Conversation

@mockitoguy
Copy link
Member

mockitoguy commented Jun 12, 2017

Problem

Mockito does not do static mocking at the moment, Powermockito does. Powermockito uses a lot of internal Mockito APIs exposing the community to issues and wasted productivity on version upgrades. We want Mockito APIs to be easily extensible by framework integrators such us Powermockito without compromising API simplicity and consistency.

All proposed changes are backwards compatible in accordance to Mockito's compatibility contract (in short: limited changes to org.mockito.* types, freedom to change org.mockito.internal.*).

Public API changes

The full discussion on the use cases, implementation options and decisions is in #1110 ticket.

  1. New public method for creating mock creation settings object:
Mockito.withSettings().name(“my mock”).build(Foo.class);
  1. Framework integrators can now provide custom MockHandler via MockMaker#getMockHandler(). Previously, it we didn’t offer this capability because we didn’t have convincing use cases that it was needed.

Public API considerations

  1. Framework integrators may not provide custom implementation of InvocationContainer at this stage. This part of API is accessible via public MockMaker#getMockHandler().getInvocationContainer() method. There are no convincing use cases that exposing this new public API is needed.
@codecov-io
Copy link

codecov-io commented Jun 12, 2017

Codecov Report

Merging #1121 into release/2.x will increase coverage by 0.18%.
The diff coverage is 87.39%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/2.x    #1121      +/-   ##
=================================================
+ Coverage          86.93%   87.11%   +0.18%     
+ Complexity          2299     2287      -12     
=================================================
  Files                287      287              
  Lines               5825     5791      -34     
  Branches             686      685       -1     
=================================================
- Hits                5064     5045      -19     
+ Misses               564      554      -10     
+ Partials             197      192       -5
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/mockito/Mockito.java 96.55% <ø> (ø) 39 <0> (ø) ⬇️
...to/internal/stubbing/StubbedInvocationMatcher.java 100% <ø> (ø) 8 <0> (ø) ⬇️
...mockito/internal/invocation/UnusedStubsFinder.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/org/mockito/internal/stubbing/StubberImpl.java 90.9% <100%> (ø) 18 <0> (ø) ⬇️
...to/internal/verification/VerificationDataImpl.java 100% <100%> (ø) 7 <1> (ø) ⬇️
...g/mockito/internal/handler/NullResultGuardian.java 100% <100%> (ø) 6 <1> (-1) ⬇️
...to/internal/handler/InvocationNotifierHandler.java 100% <100%> (ø) 8 <1> (-1) ⬇️
.../org/mockito/internal/handler/MockHandlerImpl.java 100% <100%> (ø) 12 <6> (-1) ⬇️
...ito/internal/stubbing/InvocationContainerImpl.java 92.85% <100%> (ø) 29 <1> (ø) ⬇️
...to/internal/framework/DefaultMockitoFramework.java 100% <100%> (ø) 5 <2> (+2) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f03a92e...00b5539. Read the comment docs.

public List<StubbedInvocationMatcher> getStubbedInvocations() {
return stubbed;
public List<Stubbing> getStubbedInvocations() {
return (List) stubbed;

This comment has been minimized.

Copy link
@TimvdLippe

TimvdLippe Jun 12, 2017

Contributor

This seems unfortunate. Can we change the type of stubbed?

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Jun 12, 2017

Author Member

Nope :( Please suggest a different approach if you know it!

This comment has been minimized.

Copy link
@ChristianSchwarz

ChristianSchwarz Jun 13, 2017

Contributor

java generics are invariant, we need a wild card here: public List<? extends Stubbing> getStubbedInvocations()

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Jun 15, 2017

Author Member

We could certainly do that, thank you for the pointer! Problem is that it makes the public API a bit more complicated because the invariant wildcard leaks to the public interface.

@mockitoguy mockitoguy changed the title Experimented with MockHandler API Fully expose MockHandler public API Jun 18, 2017
} else {
throw new NotAMockException("Argument should be a mock, but is: " + mock.getClass());
}
}

public static InvocationContainerImpl getInvocationContainer(Object mock) {
return (InvocationContainerImpl) getMockHandler(mock).getInvocationContainer();

This comment has been minimized.

Copy link
@TimvdLippe

TimvdLippe Jun 20, 2017

Contributor

Why cast here?

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Jun 20, 2017

Author Member

I think that InvocationContainer cannot be practically made public. It's too coupled with Mockito impl. details. I wrote details in the commit message and high level information in the Javadoc.

Do you have suggestions how to make it better?

This comment has been minimized.

Copy link
@TimvdLippe

TimvdLippe Jun 22, 2017

Contributor

I just was wondering why we couldn't use the interface, but if the impl is too coupled then there is no other option.

public class VerificationDataImpl implements VerificationData {

private final InvocationMatcher wanted;
private final InvocationContainer invocations;
private final InvocationContainerImpl invocations;

This comment has been minimized.

Copy link
@TimvdLippe

TimvdLippe Jun 20, 2017

Contributor

I dont think this is necessary?

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Aug 28, 2017

Author Member

InvocationContainer is a marker interface at this stage to hide the internal implementation from leaking to the public API. It's impractical to expose it as public API. Hence, we use the 'impl' everywhere in the codebase. For more info, take a look at the javadoc for InvocationContainer interface.

Hope that helps!


verify(mockHandler).getInvocationContainer();
verify(mockHandler).getMockSettings();
verify(mockHandler).setAnswersForStubbing(ArgumentMatchers.<Answer<?>>anyList());

This comment has been minimized.

Copy link
@TimvdLippe

TimvdLippe Jun 20, 2017

Contributor

This is thus a breaking change? :(

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Jun 20, 2017

Author Member

Thank you for review! Absolutely let's avoid breaking changes!

Why do you consider this a breaking change? Those changes are all in "org.mockito.internal" components. Is it because VerificationData API already leaks some internal APIs? :)

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Jun 22, 2017

Author Member

This is thus a breaking change? :(

"setAnswersForStubbing" method was internal so this is not a breaking change. I removed this line from the test because we no longer need this method :)

Thank you for feedback!

@mockitoguy
Copy link
Member Author

mockitoguy commented Jun 20, 2017

Thank you @TimvdLippe for reviewing! The PR is still in progress and we definitely must avoid breaking changes.

@mockitoguy mockitoguy changed the title Fully expose MockHandler public API Improve APIs for framework integrators such as Powermockito Jun 24, 2017
mockitoguy added 20 commits Jun 12, 2017
In an effort to clean up the API to make it easier to cleanly integrate PowerMockito scenarios.

1. MockHandler API currently is a semi public API. Technically it lives in public package but the documentation clearly states that users should not provide own implementations. Internally, Mockito always casts MockHandler into InternalMockHandler. This is a dealbreaker for Powermockito (see https://github.com/powermock/powermock/blob/68d988b9cb6a20007d9387d01ded9cc6f7781e5c/powermock-api/powermock-api-mockito2/src/main/java/org/powermock/api/mockito/mockmaker/PowerMockMaker.java#L57).
2. InternalMockHandler is internal because it leaks internal StubbedInvocationMatcher API. This change makes InternalMockHandler ready to be converted into public API. We can start using Stubbing interface in place of StubbedInvocationMatcher. Stubbing interface was introduced this year, it was not available when we created MockMaker plugin.
3. This change makes Stubbing interface additionally extend Answer. This adds "answer()" method to Stubbing. Stubbings can give answers so it is a good change from the standpoint of the model
- Next step of exposing MockHandler API so that Mockito can be neatly integrated with.
MockHandler API was incomplete and this work attempts to tidy it up.
- Pushed the methods onto the public API. More work is pending to eliminate an internal API leaks.
This steps is required to complete exposing full public API of MockHandler

Work in progress, all tests pass :)
MockHandler will return InvocationContainer that will deal will invocations, stubbings and answers. This cleans up the code a bit, too.
- While working on exposing the MockHandler public API it became apparent that the API would extremely coupled with internal Mockito implementation. Just look at the current public methods on InvocationContainerIml - majority of those methods would have to be exposed to a public type. I don't see any use case where users would want to provide their own implementations of InvocationContainer - they would have to implement 10 methods extremely coupled with how Mockito works.
- To avoid exposing awkward API, I removed all methods from now public InvocationContainer. This way, we can help edge cases like static mocking in Powermockito, which needs to implement custom MockHandler but not necessarily custom InvocationContainer.
- If we have good use cases, we can expose InvocationContainer with all necessary methods (and trim some methods if possible)
This way, it is more conventional in how the builder pattern is used in the world
Framework integrators need to be able to create instances of MockCreationSettings objects without providing entire custom implementation of this interface. The latter approach is not viable because MockCreationSettings can grow with new methods when we add more configurability to mocks. When that happens, all clients who have custom implementation of MockCreationSettings will break. To avoid this risk, this change offers public API to create instances of MockCreationSettings.
- The purpose is to flesh out APIs required for other mocking frameworks to support static mocking.
- TODOs are pending for review of the public API and documentation
After we integrated with Bytebuddy, the InvocationImpl class was no longer needed. I removed this class along with surrounding types.
In some context (like not using proxy pattern for mocking but Java agent, static mocking), there is no such thing as SuperMethod. Hence, I renamed it to something more generic.
…ations.

Removed ugly workaround, now the client is responsible for providing the implementation of the the Callable.
This way, I can refer to this code from a blog post.
The usage would be: Mockito.framework().getMockitoPlugins().getDefaultPlugin(MockMaker.class);

This is experimentation only for now.
The API I came up with is best I could think of given that inline mock maker uses the mechanism of "plugin alias".
mockitoguy added 3 commits Aug 28, 2017
It is not needed as top level class, it is used in single narrow context.
Added new annotation to indicate that given public type is not intended for extension.
@mockitoguy
Copy link
Member Author

mockitoguy commented Aug 28, 2017

FYI: I'm not far from calling this complete. I don't expect much code changes, rather:

  • verify Powermock needs all the new public API added
  • add missing documentation / tests for public types
  • add Since tag

Despite above, I think the code is ready for being reviewed. If you have feedback, please let me know!

@mockitoguy
Copy link
Member Author

mockitoguy commented Aug 31, 2017

This is complete. @mockito/developers, friendly request for review :)

For high level summary of changes to the public API, see the new section added to our "user guide" in main Mockito class.

handler.handle(invocation);

//verify staticMethod on mock
verify(mock);

This comment has been minimized.

Copy link
@TimvdLippe

TimvdLippe Aug 31, 2017

Contributor

I think this should call the staticMethod? The same for the next verify

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Sep 1, 2017

Author Member

Thanks for feedback! I have added code comment to this test method explaining what and why it is implemented this way. Is it clearer now?

This comment has been minimized.

Copy link
@mockitoguy

mockitoguy Sep 1, 2017

Author Member

Here's the comment: 5be550b

Based on the feedback from Tim
@TimvdLippe
Copy link
Contributor

TimvdLippe commented Sep 1, 2017

@mockitoguy
Copy link
Member Author

mockitoguy commented Sep 4, 2017

Any feedback? I would love to merge it in a couple of days :)

@mockitoguy mockitoguy changed the title Improve APIs for framework integrators such as Powermockito Improve and develop APIs required for framework integrators Sep 4, 2017
@TimvdLippe
Copy link
Contributor

TimvdLippe commented Sep 5, 2017

Seems like this branch has conflicts and has a failing build. Other than this PR is approved!

@raphw
Copy link
Member

raphw commented Sep 5, 2017

A lot to read but it look really good to me. Props to the documentation and compatibility efforts!

@mockitoguy mockitoguy merged commit 8c2dfa6 into release/2.x Sep 8, 2017
4 checks passed
4 checks passed
codecov/patch 87.39% of diff hit (target 86.93%)
Details
codecov/project 87.11% (+0.18%) compared to f03a92e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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