Add invoker API to allow for alternative invocation modes to better support the module system. #1996
Conversation
raphw
force-pushed
the
safe-accessibility
branch
from
a056d45
to
3cde1bf
Compare
Codecov Report
@@ Coverage Diff @@
## release/3.x #1996 +/- ##
=================================================
- Coverage 85.23% 85.01% -0.22%
- Complexity 2604 2633 +29
=================================================
Files 323 323
Lines 7520 7710 +190
Branches 899 912 +13
=================================================
+ Hits 6410 6555 +145
- Misses 873 915 +42
- Partials 237 240 +3
Continue to review full report at Codecov.
|
raphw
force-pushed
the
safe-accessibility
branch
5 times, most recently
from
0e365c6
to
755ce7c
Compare
| // initMocks called in TestBase Before method, so instances are not the same | ||
| MockitoAnnotations.openMocks(this); | ||
| // openMocks called in TestBase Before method, so instances are not the same | ||
| session = MockitoAnnotations.openMocks(this); |
There was a problem hiding this comment.
We should update openMocks to use @CheckReturnValue. Let's fix that in a separate PR.
| Class.class, | ||
| MethodHandles.Lookup.class)); | ||
| } catch (Throwable t) { | ||
| throw new MockitoInitializationException( |
There was a problem hiding this comment.
If this exception is thrown, should we automatically fall back to the reflection accessor? Or should we bail out at all times?
There was a problem hiding this comment.
This would mean that Java 9 does not contain a method that is speced to be there. I think an exception is appropriate. The use is guarded by ModuleMemberAccess.
| import static net.bytebuddy.matcher.ElementMatchers.named; | ||
| import static org.mockito.internal.util.StringUtil.join; | ||
|
|
||
| class InstrumentationMemberAccessor implements MemberAccessor { |
There was a problem hiding this comment.
There's quite a bit code there and I think the code coverage is not high enough, which is what CodeCov is catching. Is it feasible to increase the coverage here or is that not possible?
There was a problem hiding this comment.
I can add a few tests for the exception scenarios.
| Object module = getModule.bindTo(method.getDeclaringClass()).invokeWithArguments(); | ||
| String packageName = method.getDeclaringClass().getPackage().getName(); | ||
| assureOpen(module, packageName); | ||
| MethodHandle handle = | ||
| ((MethodHandles.Lookup) | ||
| privateLookupIn.invokeExact( | ||
| method.getDeclaringClass(), DISPATCHER.getLookup())) | ||
| .unreflect(method); | ||
| if (!Modifier.isStatic(method.getModifiers())) { | ||
| handle = handle.bindTo(target); | ||
| } | ||
| try { | ||
| return handle.invokeWithArguments(arguments); | ||
| } catch (Throwable t) { | ||
| throw new InvocationTargetException(t); | ||
| } |
There was a problem hiding this comment.
There is quite a bit of duplication across these methods. Does it maybe make sense to extract it out? The only difference I am spotting is the unreflect in here, where others use for example unreflectGetter.
There was a problem hiding this comment.
I didn't want to factor it out too much since I don't think it makes the code easier to read and it would be harded to extend if you wanted to add a special case for one of the dispatchers at some point that does not apply to the other.
|
Thanks. It should fix the warnings of illegal access together with #2002 - if using the inline mock maker. |
…upport the module system.
raphw
force-pushed
the
safe-accessibility
branch
from
755ce7c
to
24c0e45
Compare
Adds a
MemberAccessorabstraction for accessing fields, methods and constructors where the default implementationReflectionMemberAccessorimplements the current behavior of using reflection andsetAccessible.Also, this PR adds a new implementation
ModuleMemberAccessorwhere the instrumentation API is leveraged to open modules to Mockito before using method handles to access any such member. This way, module boundaries are no longer stopping Mockito from functioning on Java 9 and onwards. Since the instrumentation API is already used by the inline-mock-maker, it is enabled for this mock maker by default.