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

Add a limited mock maker that is based only on the java.lang.reflect.Proxy utility #2397

Merged
merged 1 commit into from Aug 24, 2021

Conversation

raphw
Copy link
Member

@raphw raphw commented Aug 22, 2021

This could be an interesting addition for puristic projects that want to use Mockito's API but not rely on byte code generation which naturally requires an update of Byte Buddy for any new JVM version. The advantage of this mock maker is that it will work with any future release for this limited scope.

@raphw raphw requested a review from TimvdLippe August 22, 2021 22:21
@raphw raphw added the WIP Work in progress label Aug 22, 2021
@raphw raphw force-pushed the proxy-mock-maker branch 3 times, most recently from 73bd320 to eda775b Compare August 23, 2021 10:24
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2021

Codecov Report

Merging #2397 (ff6d1c8) into main (b04c703) will decrease coverage by 0.29%.
The diff coverage is 42.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2397      +/-   ##
============================================
- Coverage     84.69%   84.39%   -0.30%     
- Complexity     2797     2803       +6     
============================================
  Files           329      330       +1     
  Lines          8515     8576      +61     
  Branches       1032     1039       +7     
============================================
+ Hits           7212     7238      +26     
- Misses         1021     1053      +32     
- Partials        282      285       +3     
Impacted Files Coverage Δ
src/main/java/org/mockito/Mockito.java 91.25% <ø> (ø)
...ockito/internal/creation/proxy/ProxyMockMaker.java 41.66% <41.66%> (ø)
...l/configuration/plugins/DefaultMockitoPlugins.java 90.90% <100.00%> (+0.28%) ⬆️

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 b04c703...ff6d1c8. Read the comment docs.

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.

I like the idea, thanks for suggesting! I would like to see some tighter test coverage, but other than that this LGTM.

InvocationHandler.class.getMethod(
"invokeDefault", Object.class, Method.class, Object[].class);
} catch (NoSuchMethodException ignored) {
m = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe fail fast here and rethrow a nice Mockito exception with instructions that this mockmaker can't be used? (I know that this situation shouldn't ever occur, but I guess we can code defensively)

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be used. The method is only added in Java 16, so the functionality is offered only if it exists.

case "equals":
return proxy == args[0];
case "toString":
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to show a nicer Mock name here, like we do with our regular mocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right of course, darn, I overlooked your comments, I will add another PR.


import static org.assertj.core.api.Assertions.assertThat;

public class ProxyMockMakerTest
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 maybe also have some tests that verify that attempting to mock non-interfaces correctly fails with a descriptive error message?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on https://app.codecov.io/gh/mockito/mockito/compare/2397/diff#D2 I think it would also be good to add some tests that method invocations and all are working as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

We test for the mockable state. To avoid the overhead, those methods will never be invoked with non-mockable types and the error messages are rather undefined.

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.

None yet

3 participants