-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
73bd320
to
eda775b
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
eda775b
to
d5b5a14
Compare
d5b5a14
to
ff6d1c8
Compare
There was a problem hiding this 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; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 ""; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.