AdditionalMatchers.and() and or() swap matcher order #3331

DidierLoiseau · 2024-04-29T16:48:38Z

Consider the following code:

@ExtendWith(MockitoExtension.class)
public class MockitoTest {
  @Mock MyService myService;

  @Test
  public void test() {
    Mockito.when(
            myService.doSomething(
                and(any(MyPojo.class),
                    argThat(p -> p.id == 1))))
        .thenReturn(1);

    myService.doSomething("hello");
  }

  interface MyService {
    int doSomething(Object pojo);
  }

  static class MyPojo {
    int id = 1;
  }
}

This test throws the following exception:

java.lang.ClassCastException: class java.lang.String cannot be cast to class MockitoTest$MyPojo (java.lang.String is in module java.base of loader 'bootstrap'; MockitoTest$MyPojo is in unnamed module of loader 'app')
	at org.mockito.internal.matchers.And.matches(And.java:23)
	at org.mockito.internal.invocation.TypeSafeMatching.apply(TypeSafeMatching.java:35)
	at org.mockito.internal.invocation.MatcherApplicationStrategy.argsMatch(MatcherApplicationStrategy.java:85)
	at org.mockito.internal.invocation.MatcherApplicationStrategy.forEachMatcherAndArgument(MatcherApplicationStrategy.java:71)
	at org.mockito.internal.invocation.InvocationMatcher.argumentsMatch(InvocationMatcher.java:156)
	at org.mockito.internal.invocation.InvocationMatcher.matches(InvocationMatcher.java:82)
	at org.mockito.internal.stubbing.InvocationContainerImpl.findAnswerFor(InvocationContainerImpl.java:92)
	at org.mockito.internal.handler.MockHandlerImpl.handle(MockHandlerImpl.java:91)
	at org.mockito.internal.handler.NullResultGuardian.handle(NullResultGuardian.java:29)
	at org.mockito.internal.handler.InvocationNotifierHandler.handle(InvocationNotifierHandler.java:34)
	at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor.doIntercept(MockMethodInterceptor.java:82)
	at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor.doIntercept(MockMethodInterceptor.java:56)
	at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor$DispatcherDefaultingToRealMethod.interceptAbstract(MockMethodInterceptor.java:161)
	at MockitoTest$MyService$MockitoMock$v3DeXXsQ.doSomething(Unknown Source)
	at MockitoTest.test(MockitoTest.java:23)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)

Debugger inspection of And.matches() shows that m1 is the lambda and m2 is the InstanceOf, as opposed to what was passed to and(). I guess this comes from ArgumentMatcherStorageImpl#reportAnd() and reportOr() which pop m1 before m2, swapping the order.

This is with Mockito 5.11.0, Junit 5.10.2 and Java 11.

check that

The mockito message in the stacktrace have useful information, but it didn't help
No, it does not provide useful information (here the exception is similar to the one in argThat throws ClassCastException upon different subclass instance than for the ArgumentMatcher generic type #2268 but it could be something else like an NPE, depending on the use case)
The problematic code (if that's possible) is copied here;
Note that some configuration are impossible to mock via Mockito
Provide versions (mockito / jdk / os / any other relevant information)
Provide a Short, Self Contained, Correct (Compilable), Example of the issue
(same as any question on stackoverflow.com)
Read the contributing guide

The text was updated successfully, but these errors were encountered:

TimvdLippe · 2024-04-29T17:45:42Z

Yes this appears to be a legitimate bug, good catch! Feel free to submit a pull request with a fix and a regression test for it!

dev-jonghoonpark · 2024-05-01T02:13:16Z

can i try contribute?

TimvdLippe · 2024-05-01T17:17:26Z

@dev-jonghoonpark feel free to submit a pull request!

dev-jonghoonpark · 2024-05-02T12:51:07Z

hello. @TimvdLippe

I found a problem and fixed the code.

I think the problem was that we used stack inside ArgumentMatcherStorageImpl.java, so the order was reversed during the push/pop process.

so I changed code like this (I modified it to assign the result of the popMatcher to m2 first)

public void reportAnd() {
    assertStateFor("And(?)", TWO_SUB_MATCHERS);

    ArgumentMatcher<?> m2 = popMatcher();
    ArgumentMatcher<?> m1 = popMatcher();

    reportMatcher(new And(m1, m2));
}

@Override
public void reportOr() {
    assertStateFor("Or(?)", TWO_SUB_MATCHERS);

    ArgumentMatcher<?> m2 = popMatcher();
    ArgumentMatcher<?> m1 = popMatcher();

    reportMatcher(new Or(m1, m2));
}

Before the fix, m1 and m2 were reversed, as follows

After the fix, m1 and m2 were assigned as expected, as follows

I was happy to think I had fixed the bug.
But sadly, the error changed to the following
(I used the example code provided by the author)

org.mockito.exceptions.misusing.UnnecessaryStubbingException: 
Unnecessary stubbings detected.
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
  1. -> at com.example.demo.MockitoTest.test(MockitoTest.java:22)
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.
	at org.mockito.junit.jupiter.MockitoExtension.afterEach(MockitoExtension.java:192)
	...

I think it's correct that I'm getting an UnnecessaryStubbingException because I didn't enter the expected input. (Passed "hello", not any(MyPojo.class))
However, something feels like the result is not clear.

What are your thoughts?
I'm wondering if this is a good fix.

If this is the right fix, I'll raise a PR with the addition of test code for this.

(Note that the ./gradlew check passed without fail.)

Have a nice day

DidierLoiseau · 2024-05-02T18:47:43Z

I think it's correct that I'm getting an UnnecessaryStubbingException because I didn't enter the expected input. (Passed "hello", not any(MyPojo.class))
However, something feels like the result is not clear.

I think this is normal, I didn’t write the MRE such that the mock would actually be used, but you could simply add a second call with a parameter that matches the mock, e.g.

myService.doSomething(new MyPojo());

…rder

…ers.or() not to swap the order of matchers

…rder

Fixes #3331

TimvdLippe added the please contribute label Apr 29, 2024

dev-jonghoonpark added a commit to dev-jonghoonpark/mockito-forked that referenced this issue May 3, 2024

Fixes mockito#3331 : AdditionalMatchers.and() and or() swap matcher o…

af2a7e2

…rder

dev-jonghoonpark added a commit to dev-jonghoonpark/mockito-forked that referenced this issue May 3, 2024

Fixes mockito#3331 : Fix AdditionalMatchers.and() and AdditionalMatch…

e4a8b22

…ers.or() not to swap the order of matchers

dev-jonghoonpark mentioned this issue May 3, 2024

Fixes #3331 : Fix AdditionalMatchers.and() and AdditionalMatchers.or() not to swap the order of matchers #3335

Merged

8 tasks

dev-jonghoonpark added a commit to dev-jonghoonpark/mockito-forked that referenced this issue May 9, 2024

Fixes mockito#3331 : AdditionalMatchers.and() and or() swap matcher o…

c8397d5

…rder

dev-jonghoonpark added a commit to dev-jonghoonpark/mockito-forked that referenced this issue May 9, 2024

Fixes mockito#3331 : AdditionalMatchers.and() and or() swap matcher o…

757a415

…rder

dev-jonghoonpark added a commit to dev-jonghoonpark/mockito-forked that referenced this issue May 11, 2024

Fixes mockito#3331 : AdditionalMatchers.and() and or() swap matcher o…

65a76c0

…rder

TimvdLippe closed this as completed in #3335 May 11, 2024

TimvdLippe pushed a commit that referenced this issue May 11, 2024

AdditionalMatchers.and() and or() swap matcher order (#3335)

12cef84

Fixes #3331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AdditionalMatchers.and() and or() swap matcher order #3331

AdditionalMatchers.and() and or() swap matcher order #3331

DidierLoiseau commented Apr 29, 2024

TimvdLippe commented Apr 29, 2024

dev-jonghoonpark commented May 1, 2024

TimvdLippe commented May 1, 2024

dev-jonghoonpark commented May 2, 2024 •

edited

DidierLoiseau commented May 2, 2024

AdditionalMatchers.and() and or() swap matcher order #3331

AdditionalMatchers.and() and or() swap matcher order #3331

Comments

DidierLoiseau commented Apr 29, 2024

TimvdLippe commented Apr 29, 2024

dev-jonghoonpark commented May 1, 2024

TimvdLippe commented May 1, 2024

dev-jonghoonpark commented May 2, 2024 • edited

DidierLoiseau commented May 2, 2024

dev-jonghoonpark commented May 2, 2024 •

edited