Mockito #1013: Defines and implements API for static mocking. #1955
Conversation
|
I thought about it but I am pretty confident that this is what it's gonna be! I suggest we put the APIs as incubating as we have done with the inline mock maker. If we commit to the API, even in a separate artifact, I think the Android folks will still go for it and have a similar expectation on it's stability. You think a separate artifact would be cleaner? |
|
I mostly prefer a conservative route if we have that option. But keeping as Incubating is okay with me. |
|
I think I'd prefer the single artifact. It's fully opt-in and does not touch any existing code. I think this way it will be used more and we'll find out the adoption quicker. |
raphw
force-pushed
the
static-mock
branch
4 times, most recently
from
5f28b02
to
839da0d
Compare
Codecov Report
@@ Coverage Diff @@
## release/3.x #1955 +/- ##
=================================================
- Coverage 85.76% 85.43% -0.34%
- Complexity 2542 2637 +95
=================================================
Files 318 320 +2
Lines 7209 7540 +331
Branches 861 892 +31
=================================================
+ Hits 6183 6442 +259
- Misses 810 859 +49
- Partials 216 239 +23 Continue to review full report at Codecov.
|
|
I just saw this and cannot believe my eyes :-) Will try to review shortly!!! |
|
Awesome, just discovered a remaining recursion on |
|
And it works, too! |
There was a problem hiding this comment.
It's quite a lot of code. While I get the overall picture, the refactorings internally with the additional maps and how the static mocks are handled is complicated to follow. The changes probably make sense, and the tests show that it is WAI, but I have trouble keeping the overall picture. E.g. why do we need these refactorings.
It would be great if we can somehow reduce the scope of this PR by splitting it up, so that these refactorings make more sense to me. Let me know if that is feasible or why that is difficult to do.
Lastly, with regards to the return type change of MockitoAnnotations.initMocks, I think that is a breaking change. It would be good if we can create a separate method for initWithStaticMocks so that users can incrementally upgrade (and to make it more explicit that a particular test is expected to use static mocks). WDYT?
| * @Before public void initMocks() { | ||
| * MockitoAnnotations.initMocks(this); | ||
| * closeable = MockitoAnnotations.initMocks(this); |
There was a problem hiding this comment.
This is potentially a breaking change, if compilations downstream require the result of any AutoCloseable to be used. I would have to check for example internally if that is the case. Can we maybe separate this out into a separate method called MockitoAnnotations.initWithStaticMocks? That would make it clearer that a particular test is allowed to use static mocks. By using a separate method, users can write static analyses that verify that .close is called.
There was a problem hiding this comment.
I thought about this, too. It is source compatible but not binary compatible. However, I do not think that this is a very commonly used API by derivative tools and direct users do not have to apply any changes until using static mocks in their custom tooling what likely requires them to revisit this API. In a way, explicit life-cycles are a new concept in Mockito that get introduced by this change, it will require some upstream adoption and in such a case, I think that breakage is the most explicit way to communicate this to users; I always prefer this over silent or runtime failure. I suggest therefore to make this visible in the release notes and by the versioning scheme via releasing a new feature version when merging this.
| if (mock instanceof MockAccess) { | ||
| ((MockAccess) mock).setMockitoInterceptor(mockMethodInterceptor); | ||
| if (mock instanceof Class<?>) { | ||
| Map<Class<?>, MockMethodInterceptor> interceptors = mockedStatics.get(); |
There was a problem hiding this comment.
I am not really following this refactoring. Could you elaborate on why this is necessary? Would it maybe be possible to separate these changes into its own PR to ease reviewing?
There was a problem hiding this comment.
It's easier when looking at the raw file, the changefile cuts parts of the interesting segment. Basically, to avoid breaking the existing implementation, static mocks are represented by the Class value for the static mock. This is also done in the listerners etc. what is possible since it is currently impossible to mock a Class instance, even with the inline-mock maker.
We cannot store static mocks in the same concurrent map as the regular mocks since they are thread local. Therefore, we always need to make a distinction if a mock is an instance of Class or not to see if a static or a regular mock is requested. The advantage of this solution is that the current API can mainly remain untouched, keeping the changeset to a minimum.
|
I thought about splitting the I considered splitting up the PR but that way I could not implement static mocks as a running feature without putting all pieces together. I therefore suggest that we release it as one change, that way Mockito stays consistent in its offered features and we hopefully get frictionless adoption. I am afraid that this change has a certain inherent complexity that would not go away even when splitting up. |
I have seen numerous usages in Google. Before we make this change, I would like to verify that this is indeed safe to do. I am not sure when I have time to do so, but I think someday next week should be possible. |
|
I have not considered wide usage. In this case, I think it's better to deprecate I think such a life-cycle will be useful for other cases, too. For instance, I think it opens a solution for #1802 where I otherwise cannot see a solution. I updated the PR but still suggest that we do not split it up. We could otherwise introduce the life-cycle in a separate PR but without a single user of the life-cycle, it would be hard to capture the application of this feature in a meaningful way, not considering the additional work that splitting up the PR would entail. |
|
|
| * @since 3.4.0 | ||
| */ | ||
| @Incubating | ||
| <T> StaticMockControl<T> createStaticMock( |
There was a problem hiding this comment.
This is a breaking change and breaks Dexmaker. I recommend adding a default implementation with
throw new MockitoException("This mockmaker cannot create static mocks");There was a problem hiding this comment.
You are right, I added a default method which throws an explaining exception. I also had to move the static mock control into a non-internal package. Otherwise nobody else could implement the API.
| */ | ||
| void closeOnDemand(); | ||
|
|
||
| @FunctionalInterface |
There was a problem hiding this comment.
This is not available on Android builds and therefore will fail. I think we can remove this annotation and the rest will still work?
There was a problem hiding this comment.
You are right, I commented it out. It is mainly for documentation purposes and to fail compilation if the interfaces does not define a single abstract method.
| */ | ||
| void closeOnDemand(); | ||
|
|
||
| // @FunctionalInterface - not supported on Android |
There was a problem hiding this comment.
| // @FunctionalInterface - not supported on Android |
|
@raphw You also need to run |
Co-authored-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
| private static Class<?> inferStaticMock(Type type, String name) { | ||
| if (type instanceof ParameterizedType) { | ||
| ParameterizedType parameterizedType = (ParameterizedType) type; | ||
| return (Class<?>) parameterizedType.getRawType(); |
There was a problem hiding this comment.
Shouldn't this use getActualTypeArguments()[0] instead?
It's failing for me now since getRawType() returns MockedStatic
There was a problem hiding this comment.
Do you mind opening a PR with regression test + fix? Thanks!
There was a problem hiding this comment.
Hmm, seems this was easy to fix locally with my suggestion, but I now get another error, which I don't know why it happens:
org.mockito.exceptions.misusing.NotAMockException: Argument passed to Mockito.mockingDetails() should be a mock, but is an instance of class java.lang.Class!
at org.mockito.junit.jupiter.MockitoExtension.afterEach(MockitoExtension.java:186)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeAfterEachCallbacks$11(TestMethodTestDescriptor.java:255)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor$$Lambda$316/0000000000000000.invoke(Unknown Source)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeAllAfterMethodsOrCallbacks$12(TestMethodTestDescriptor.java:271)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor$$Lambda$1809/0000000000000000.execute(Unknown Source)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeAllAfterMethodsOrCallbacks$13(TestMethodTestDescriptor.java:271)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor$$Lambda$314/0000000000000000.accept(Unknown Source)
The class with the static method ends up there...
Interesting is that if I switch to JUnit 4, I get the same error but the test passes 😀
There was a problem hiding this comment.
I isolated the error. The problem is that the mocks for static methods are scoped. This means that they are released at the end of the try-with-resources block what implies that the mocks are no longer valid at the end of the method resulting in this error.
@TimvdLippe For now I think the easiest is to exclude static mocks from the post processing. The more correct approach would be to execute the "after each" logic upon releasing the mock rather then at the end of the test. I will look into it but need to understand the post-processing first.
There was a problem hiding this comment.
Btw, my setup was with and without a try-with-resources block, since I understood from the documentation of @Mock that it should work with that as well, which would simplify things (and since I already had other fields that were annotated with @Mock)
There was a problem hiding this comment.
@ExtendWith(MockitoExtension.class)
public class TenantScopeTest {
@Mock
private UserPrincipal principal;
@Mock
private MockedStatic<UserPrincipalUtil> userPrincipalUtil;
@Test
public void opsShouldBeCachedInstancesByNameAndTenant() {
String tenant = "x";
when(principal.getMainTenant()).thenReturn(tenant);
userPrincipalUtil.when(UserPrincipalUtil::requireUserPrincipal).thenReturn(principal);
//....
}There was a problem hiding this comment.
@andrei-ivanov I have not had the time to test it yet (we should have unit tests for the runners using both mock makers) but maybe the 'runner-fix' branch already fixes the problem.
There was a problem hiding this comment.
I still get the NotAMockException with the runner-fix branch
There was a problem hiding this comment.
Let me know when you want to do run some more tests, I'm eager to get rid of PowerMock 😁
|
I also have this problem, when the mockMaker is InlineByteBuddyMockMaker, I use SpyBean to spy a spring bean, and then verify the bean, it throws NotAMockException. I debug and find the class type is not contains mockito, but it contains when I use the ByteBuddyMockMaker. and it seems not a spring bug. when I use the verify((AppGroupClusterAction) AopTestUtils.getTargetObject(appGroupClusterAction)) to verify. it works. |
Fixes #1013 - allows for static method mocking in Mockito.