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

Mock resolver plugin #2042

Merged
merged 3 commits into from Oct 19, 2020
Merged

Mock resolver plugin #2042

merged 3 commits into from Oct 19, 2020

Conversation

raphw
Copy link
Member

@raphw raphw commented Sep 14, 2020

Adds a plugin to allow for adding one or multiple mock resolvers. This way, instances that are provided to Mockito's DSL do no longer require to be the actual mocks but can also be proxies of mocks what is something already being applied by frameworks such as Spring.

This works today with the subclass mock maker by accident as we read the mock state from the mock instance via a method which gets proxied. If we ever find a more private appraoch this would however break Spring's Mockito use and it is already broken with the inline mock maker.

By this SPI, Spring could add an unproxy resolver to its Mockito build-up and make it's proxied mocks compatible with Mockito. Other frameworks could use the same approach if desired as this SPI is generic.

Closes #1980

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation and solution seems fine. I do want to wait for the Spring folks to try this out before committing to this API.

Comment on lines 72 to 74
for (MockResolver mockResolver : Plugins.getMockResolvers()) {
mock = mockResolver.resolve(mock);
}

MockHandler handler = mockMaker.getHandler(mock);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extract this out in a private method and use that below as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree, I hope for some feedback first, too.


@MockitoSettings(strictness = Strictness.WARN)
@ExtendWith(MockitoExtension.class)
class MockResolverTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test that uses multiple resolvers? I can think of the following scenarios:

  1. Multiple resolvers, no match
  2. Multiple resolvers, one match
  3. Multiple resolvers, multiple match

For scenario 3, should we throw maybe? What if it is order-dependent? Should we continue iterating over the resolvers until none match?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think multiple resolvers should be allowed. You can technically add several in your specified order and decompose multiple levels of nesting. It's of course an obscure scenario, but it makes sense to not forbid it since it would be extra work. I can add the tests you suggest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, registering multiple converters in a test is not quite that easy since it would require multiple modules. I don't think its worth the noise to add this functionality just for testing such a simple corner case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. How would that work when a user wants to integrate with multiple frameworks that all expose a resolver?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They would all expose their resolver and they would be invoked in their registration order. For instance, let's say Hibernate offered a Mockito-based mocking API but returns Hibernate proxies on mocks. Those proxies are by definition never Spring Beans. But if one passed a Spring bean or a Hibernate proxy, both would traverse the resolver chain and be unpacked accordingly.

I assume that normally, only one resolver would apply to any wrapped mock but since Mockito should remain agnostic to this, that would be the best approach in my eyes.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2020

Codecov Report

Merging #2042 into release/3.x will increase coverage by 0.03%.
The diff coverage is 69.84%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/3.x    #2042      +/-   ##
=================================================
+ Coverage          84.89%   84.92%   +0.03%     
- Complexity          2704     2724      +20     
=================================================
  Files                325      325              
  Lines               8204     8320     +116     
  Branches             979     1009      +30     
=================================================
+ Hits                6965     7066     +101     
- Misses               968      983      +15     
  Partials             271      271              
Impacted Files Coverage Δ Complexity Δ
...g/mockito/internal/util/DefaultMockingDetails.java 100.00% <ø> (ø) 14.00 <0.00> (ø)
src/main/java/org/mockito/plugins/MockMaker.java 100.00% <ø> (ø) 3.00 <0.00> (ø)
...o/internal/configuration/plugins/PluginLoader.java 79.31% <14.28%> (-20.69%) 9.00 <1.00> (+1.00) ⬇️
...ernal/configuration/plugins/PluginInitializer.java 67.44% <60.00%> (-6.48%) 6.00 <2.00> (+2.00) ⬇️
...o/internal/configuration/plugins/PluginFinder.java 85.18% <69.23%> (-14.82%) 7.00 <2.00> (+2.00) ⬇️
.../internal/creation/bytebuddy/MockMethodAdvice.java 77.90% <75.00%> (+0.06%) 23.00 <0.00> (ø)
...internal/configuration/plugins/PluginRegistry.java 100.00% <100.00%> (ø) 9.00 <1.00> (+1.00)
...ockito/internal/configuration/plugins/Plugins.java 90.00% <100.00%> (+1.11%) 9.00 <1.00> (+1.00)
.../main/java/org/mockito/internal/util/MockUtil.java 98.03% <100.00%> (+0.60%) 26.00 <6.00> (+3.00)
...al/creation/bytebuddy/InlineBytecodeGenerator.java 95.47% <0.00%> (+2.75%) 48.00% <0.00%> (+10.00%)

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 b6ae6cf...c81b028. Read the comment docs.

@@ -55,21 +56,25 @@ public static TypeMockability typeMockabilityOf(Class<?> type) {
return mock;
}

public static <T> void resetMock(T mock) {
public static void resetMock(Object mock) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were the generic parameters removed from this and the other methods? I understand that they are not used, but I wonder if there are unintended side-effects of this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a cleanup on the side since I am touching the code already. The generic type is erased to Object and has no other function, that's why I removed it. No impact on source or binary compatibility.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with 1 nit

* provided to the {@link org.mockito.Mockito}-DSL. This mechanism can be used by
* frameworks that provide mocks that are implemented by Mockito but which are wrapped
* by other instances to enhance the proxy further.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be marked as @Incubating

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arghs, I missed this and just hit merge. Sorry, we can still add this, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes let's still add it.

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

Successfully merging this pull request may close these issues.

mockito3.4.0 InlineByteBuddyMockMaker can't work with spring cglib proxy, but ByteBuddyMockMaker works
3 participants