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

AssertionError in MockUtil.getMockHandlerOrNull in our regression test suite #2823

Closed
TimvdLippe opened this issue Dec 14, 2022 · 8 comments · Fixed by #2829
Closed

AssertionError in MockUtil.getMockHandlerOrNull in our regression test suite #2823

TimvdLippe opened this issue Dec 14, 2022 · 8 comments · Fixed by #2829
Assignees

Comments

@TimvdLippe
Copy link
Contributor

We appear to be hitting the following assertion in our test suites:

assert getMockMaker(handler.getMockSettings().getMockMaker()) == mockMaker;

Example error from the logs:

org.mockito.StaticMockingExperimentTest > stubbing_new FAILED
    java.lang.AssertionError
        at org.mockito.internal.util.MockUtil.getMockHandlerOrNull(MockUtil.java:160)
        at org.mockito.internal.util.MockUtil.isMock(MockUtil.java:147)
        at org.mockito.internal.util.DefaultMockingDetails.isMock(DefaultMockingDetails.java:32)
        at org.mockito.internal.util.DefaultMockingDetails.assertGoodMock(DefaultMockingDetails.java:85)
        at org.mockito.internal.util.DefaultMockingDetails.mockHandler(DefaultMockingDetails.java:77)
        at org.mockito.internal.util.DefaultMockingDetails.getMockHandler(DefaultMockingDetails.java:68)
        at org.mockito.StaticMockingExperimentTest.<init>(StaticMockingExperimentTest.java:44)

The assertion was introduced in #2701

@reta
Copy link
Contributor

reta commented Dec 14, 2022

@TimvdLippe I think I do have time to look at it, feel free to assign it to me, thank you.

reta added a commit to reta/mockito that referenced this issue Dec 20, 2022
…t suite. Fixes mockito#2823

Signed-off-by: Andriy Redko <drreta@gmail.com>
@reta
Copy link
Contributor

reta commented Dec 20, 2022

@TimvdLippe It seems like issue was caused by one test case, ProgrammaticMockMakerTest, which was conflicting with default mock maker strategies (by programmatically manipulating mock makers). I have extracted this test into separate test module, programmatic-test, and the logs seem to be clear, #2829

@mr1chter
Copy link

mr1chter commented Jun 7, 2023

Hey! We hit this error as well, after upgrading to Mockito 5.3.1 from 3.x (using Java 17)

This happened unreproducibly as well because the failing tests are different from the ones using a non-standard mockmaker.

If this can't be fixed (because it's not reproducible), please improve the documentation and/or the assertion with a better error message like "Please do not use different MockMakers for the same target class(?)" or something. I would send a PR myself but I'm not sure myself what's going on there.

It's also very disconcerting that doing a thing in one test class affects other test classes in when using mockito. Can't this be avoided (I'm writing this knowing I have no clue about Mockito internals)?

@reta
Copy link
Contributor

reta commented Jun 7, 2023

Thanks @mr1chter

Hey! We hit this error as well, after upgrading to Mockito 5.3.1 from 3.x (using Java 17)

Could you share a bit more details - in you tests you also change the default mock maker from test class to test class? Could you share minimal examples how do you use different mock makers in your tests?

It's also very disconcerting that doing a thing in one test class affects other test classes in when using mockito. Can't this be avoided (I'm writing this knowing I have no clue about Mockito internals)?

This particular fix was specific to Mockito but we could certainly improve the documentation / error reporting, just need to understand the context a bit more.

@mr1chter
Copy link

mr1chter commented Jun 7, 2023

We don't change the MockMaker from the default in almost all cases. There was a single one where I had to change it to the subclass MockMaker because a specific interface could not be mocked[1]. When I did this, some tests left and right of it failed due to the AssertionError, but inconsistently. When running all failing and changed tests in isolation (using gradle test --test Mytest) nothing failed anymore. But running some of them at once leads to AssertionErrors, which was very strange(looks like a race condition or something).

I just dropped in with the debugger and it said that in this specific assertion it gets a InlineByteBuddyMockMaker on the left and a ByteBuddyMockMaker on the right, handler.getMockSettings().getMockMaker() is null. The stackstrace is roughly the follows (i slightly indented the places that are not mockito code and replaced the class and method names of those):

getMockHandlerOrNull:160, MockUtil (org.mockito.internal.util), MockUtil.java
getMockHandler:115, MockUtil (org.mockito.internal.util), MockUtil.java
isVoid:94, InvocationInfo (org.mockito.internal.stubbing.answers), InvocationInfo.java
validateFor:34, Returns (org.mockito.internal.stubbing.answers), Returns.java
addAnswer:64, InvocationContainerImpl (org.mockito.internal.stubbing), InvocationContainerImpl.java
addAnswer:55, InvocationContainerImpl (org.mockito.internal.stubbing), InvocationContainerImpl.java
thenAnswer:32, OngoingStubbingImpl (org.mockito.internal.stubbing), OngoingStubbingImpl.java
thenReturn:34, BaseStubbing (org.mockito.internal.stubbing), BaseStubbing.java
 lambda$createTestStubs$0:65, CompanyInternalTest (com.forflow.internalstuff), CompanyInternalTest.java
 answer:-1, CompanyInternalTest$$Lambda$511/0x0000000801331208 (com.forflow.control.tacticalplanning.exchange.forecast), Unknown Source
answer:42, StubbedInvocationMatcher (org.mockito.internal.stubbing), StubbedInvocationMatcher.java
handle:103, MockHandlerImpl (org.mockito.internal.handler), MockHandlerImpl.java
handle:29, NullResultGuardian (org.mockito.internal.handler), NullResultGuardian.java
handle:34, InvocationNotifierHandler (org.mockito.internal.handler), InvocationNotifierHandler.java
doIntercept:82, MockMethodInterceptor (org.mockito.internal.creation.bytebuddy), MockMethodInterceptor.java
handle:134, MockMethodAdvice (org.mockito.internal.creation.bytebuddy), MockMethodAdvice.java
 methodUsedBySUT:103, ClassUsedBySUT (com.forflow.internalstuff), ClassUsedBySUT.java

As you might see, this is happening in a when().doReturn() which is called as part of a stubbed Method response itself. But afaict this can also happen in simpler stubbings on Mocks which used the regular default inline MockMaker.

Regarding documentation, I might open another issue because there are still quite some places that say "subclass mockMaker is the default" or "in the future, inline will be the default" even though this changed.

[1] The interface that could not be mocked(lets call it A) is also weird because it is not special in any way. The mocking fails with a "classic" InternalError: class redefinition failed, invalid class. I followed until this error with the debugger and noticed that it tried to redefine 4 interfaces (three of which are superinterfaces of A) and managed to identify an even more simple interface which can't be mocked.

But somehow, A can be successfully mocked with the inline mockmaker in other test classes.
I'll investigate this further and why it works there.

@mr1chter
Copy link

mr1chter commented Jun 7, 2023

I got the inline MockMaker to work, sidestepping the problem (because now, no subclass mockmaker is used anywhere).
The workaround was to apply our internal junit extension which sets up a completely mocked environment. This is somewhat overkill for this test (and was not necessary before) but seems to do the trick. In fact, as long as the test mocking A is run after a separate test which had this setup extension applied to it, it works (also some kind of spooky side-effect that shouldn't be there).

So my takeaway is now to advise people to use one mockmaker or another, but not both, as long as those are used within the same Java process anywhere.

@mr1chter
Copy link

mr1chter commented Jun 7, 2023

Sorry for the wall of text monologue, next time I'll open a proper ticket.

@reta
Copy link
Contributor

reta commented Jun 7, 2023

Sorry for the wall of text monologue, next time I'll open a proper ticket.

Not a problem, thank you for sharing that

When running all failing and changed tests in isolation (using gradle test --test Mytest) nothing failed anymore. But running some of them at once leads to AssertionErrors, which was very strange(looks like a race condition or something).

That sounds about right, the order tests are scheduled on forked JVMs introduces randomness, and certainly, just running a test in isolation works all the time.

Regarding documentation, I might open another issue because there are still quite some places that say "subclass mockMaker is the default" or "in the future, inline will be the default" even though this changed.

Please do, thank you

But somehow, A can be successfully mocked with the inline mockmaker in other test classes.
I'll investigate this further and why it works there.

:+1

In fact, as long as the test mocking A is run after a separate test which had this setup extension applied to it, it works (also some kind of spooky side-effect that shouldn't be there).

Yes, similar approach we took for Mockito (to eliminate the possibility of race and isolate tests for differrent mock makers).

So my takeaway is now to advise people to use one mockmaker or another, but not both, as long as those are used within the same Java process anywhere.

Correct, or have some sort of test support for cleaning up the mock maker per test suite (the example that comes into mind is @DirtiesContext that Spring framework uses). I think we could explore this route.

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