Repository: mockito/mockito
Issue: Broken unused stubbing reporting when matching previous stubbed call

When stubbing the same method call twice, and the second stub setup matches to first stubbed call, the first stub is not reported as unused, because Mockito detects the next stub setup as a usage of the previous stub setup. This is an example test:
```
public class UnnecessaryStubbingWithImplicitEqMatchers {
    @Mock IMethods mock;
    MockitoSession mockito = Mockito.mockitoSession().initMocks(this).strictness(Strictness.STRICT_STUBS).startMocking();

    @After public void after() {
        mockito.finishMocking();
    }

    @Test public void unnecessary_stubbing() {
        given(mock.simpleMethod("1")).willReturn("one");
        given(mock.simpleMethod("2")).willReturn("three");
        given(mock.simpleMethod("1")).willReturn("two");
        given(mock.simpleMethod("1")).willReturn("four");

        mock.simpleMethod("1");
        mock.simpleMethod("2");
    }
}
```

The test above should fail with `UnnecessaryStubbingException` containing two unused stubs, but it does not.

Mockito version: v3.2.4

Issue comments:
Nice catch! I'll review the PR!
Hey @mockitoguy @andreisilviudragnea, I think this PR has introduced a new bug (or a regression if you like).

The problem arises when using Deep Stubs, as every call in a deep stub counts as used stubbing, but if you end up stubbing that mock for real this change will change the state of the original stubbing and your test will fail with an `UnnecessaryStubbingException`

The following example is in Scala, I don't have a running Java project at hand, but I can provide a 100% Java example if this is not enough to explain the problem

```scala
  class Foo {
    def userClass: Bar = {...}
  }
  class Bar {
    def callMeMaybe: Baz = {...}
  }
  class Baz {
    def callMe: Option[String] = {...}
  }

val aFoo = mock[Foo](DefaultAnswers.ReturnsDeepStubs)
aFoo.userClass.callMeMaybe.callMe returns Some("my number")
aFoo.userClass.callMeMaybe.callMe.value shouldBe "my number"
```

That code will fail as there are 2 stubbings for `callMe`, one automatically created by the deep stub (see `ReturnsDeepStubs.class` line 67) and one for the actual stubbing I'm making to return `Some("my number")`.

This also breaks tests that needs to legitimately stub the same invocation multiple times. Particularly with invocations that have to arguments (since all invocations will match a previous stub). See https://github.com/mockito/mockito/commit/ce1632ddd6c55a7e85bec606536aa582a3b3aea1#diff-90b7e8e4bd136ee96421dc2adde3a918R71.

A simple common example is when stubbing a `Clock`:
```
@Mock
private Clock clock;

@Test 
void test() {
    Instant instant1 = Instant.ofEpochMillis(...);
    Instant instant2 = instant1.plus(...);

    when(clock.instant()).thenReturn(instant1);
    // do something at instant1

    when(clock.instant()).thenReturn(instant2);
    // do something at instant2
}
```
@pkoenig10 yes, although that may not be the best example as you could use a chain of `thenReturn()` and have a single stubbing.
That said, for a non-experienced user that may not be obvious at all, so we'd have to weight it.
It's not obvious to me how you would work around this if your code calls `clock.instant()` a large or variable number of times. I find the suggested work around to be pretty verbose and unergonomic:
```
when(clock.instant())
    .thenReturn(instant1)
    .thenReturn(instant1)
    // repeat however many times is necessary
    .thenReturn(instant2)
    .thenReturn(instant2)
    // repeat however many times is necessary
```

I think the best and simplest way to improve this behavior is to not unmark the previous stubbing if it has been used. If the stubbing has not previously been used, then I think the current behavior is appropriate. Unfortunately this does seem like it will be a simple fix since the a second stubbing causes a previous stubbing to be marked as used.
@pkoenig10 I fail to see how that is less verbose than multiple stubbings, but in any case, that's not the point.
I think this change should be reverted until it can be given a deeper thought. do you agree? @mockito/developers ?
Revert in progress: https://github.com/mockito/mockito/pull/1878

Pull request review comments:
Testing for arguments won't work in all cases, just use ```s.matches(invocation)```
There is no need to search for stubbed invocation this way. You can grab it from ```this.invocationForStubbing.getInvocation()```.
This is good work! We can pass null to "reset" the stubbing state. Do you mind updating the javadoc on those methods and write that we allow null parameter?
The name does not seem correct - I don't think this use case is related to implicit eq matchers, for example:

```
when(mock.foo(any()).then(...)
when(mock.foo(anyString()).then(...)
```

Perhaps it's worth adding test case for this, too.
Those are neat tests! In addition, I'd love to see coverage for the following use case:

```
when(mock.foo(anyString()).thenReturn(x);
when(mock.foo("foo")).thenReturn(x);
```

In above, we also should report 2 unused stubs. This way, we cover the case with arg matcher.
This is a good e2e test.

Can you add coverage for the same use case but with the "willReturn(x).when(...)" stubbing style? This should "just" work without new changes but adding extra coverage would be nice for our codebase :-)
Done.
Done.
I have updated the javadoc of the two methods.
Done.
If the test contains, for example, the `mock.foo("bar");` call, the test should report only one unused stubbing (the one with the `"foo"` argument). What did you mean by 2 unused stubs in this case?
Done.
I meant this:

```
when(mock.foo(anyString()).thenReturn(x);
when(mock.foo("foo")).thenReturn(x);
//2 unused stubbings
```

```
when(mock.foo(anyString()).thenReturn(x);
when(mock.foo("foo")).thenReturn(x);

mock.foo("foo")
//1 unused stubbing
```

Hope it makes sense!
Got it.

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/exceptions/Reporter.java b/src/main/java/org/mockito/internal/exceptions/Reporter.java
index f661b81e5c..ab94fd1551 100644
--- a/src/main/java/org/mockito/internal/exceptions/Reporter.java
+++ b/src/main/java/org/mockito/internal/exceptions/Reporter.java
@@ -5,6 +5,7 @@
 package org.mockito.internal.exceptions;
 
 import static org.mockito.internal.reporting.Pluralizer.pluralize;
+import static org.mockito.internal.reporting.Pluralizer.stubbings;
 import static org.mockito.internal.reporting.Pluralizer.were_exactly_x_interactions;
 import static org.mockito.internal.util.StringUtil.join;
 
@@ -882,9 +883,9 @@ public static MockitoException notAnException() {
 
     public static UnnecessaryStubbingException formatUnncessaryStubbingException(Class<?> testClass, Collection<Invocation> unnecessaryStubbings) {
         StringBuilder stubbings = new StringBuilder();
-        int count = 1;
+        int count = 0;
         for (Invocation u : unnecessaryStubbings) {
-            stubbings.append("\n  ").append(count++).append(". ").append(u.getLocation());
+            stubbings.append("\n  ").append(++count).append(". ").append(u.getLocation());
         }
         String heading = (testClass != null)?
                 "Unnecessary stubbings detected in test class: " + testClass.getSimpleName() :
@@ -893,7 +894,7 @@ public static UnnecessaryStubbingException formatUnncessaryStubbingException(Cla
         return new UnnecessaryStubbingException(join(
                 heading,
                 "Clean & maintainable test code requires zero unnecessary code.",
-                "Following stubbings are unnecessary (click to navigate to relevant line of code):" + stubbings,
+                "There are " + count + " unnecessary " + stubbings(count) + " (click to navigate to relevant line of code):" + stubbings,
                 "Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class."
         ));
     }
diff --git a/src/main/java/org/mockito/internal/reporting/Pluralizer.java b/src/main/java/org/mockito/internal/reporting/Pluralizer.java
index 1e8d0e3f4b..5a9d7425b0 100644
--- a/src/main/java/org/mockito/internal/reporting/Pluralizer.java
+++ b/src/main/java/org/mockito/internal/reporting/Pluralizer.java
@@ -17,4 +17,8 @@ public static String were_exactly_x_interactions(int x) {
             return "were exactly " + x + " interactions";
         }
     }
+
+    public static String stubbings(int number) {
+        return number == 1 ? "stubbing" : "stubbings";
+    }
 }
diff --git a/src/main/java/org/mockito/internal/stubbing/InvocationContainerImpl.java b/src/main/java/org/mockito/internal/stubbing/InvocationContainerImpl.java
index 07c5a5e41c..19922a7fa8 100644
--- a/src/main/java/org/mockito/internal/stubbing/InvocationContainerImpl.java
+++ b/src/main/java/org/mockito/internal/stubbing/InvocationContainerImpl.java
@@ -52,10 +52,31 @@ public void resetInvocationForPotentialStubbing(MatchableInvocation invocationMa
     }
 
     public void addAnswer(Answer answer, Strictness stubbingStrictness) {
+        unmarkLastUsedStubIfNecessary();
+
         registeredInvocations.removeLast();
         addAnswer(answer, false, stubbingStrictness);
     }
 
+    /**
+     *  A stubbed call is marked as used when the method call is stubbed again, because the second
+     *  stub looks like an actual usage of the first stubbed call. When the second stub is set up,
+     *  the previous one is marked as unused again.
+     */
+    private void unmarkLastUsedStubIfNecessary() {
+        synchronized (stubbed) {
+            Invocation invocation = invocationForStubbing.getInvocation();
+
+            for (StubbedInvocationMatcher s : stubbed) {
+                if (s.matches(invocation)) {
+                    s.markStubUsed(null);
+                    invocation.markStubbed(null);
+                    return;
+                }
+            }
+        }
+    }
+
     public void addConsecutiveAnswer(Answer answer) {
         addAnswer(answer, true, null);
     }
diff --git a/src/main/java/org/mockito/internal/stubbing/StubbedInvocationMatcher.java b/src/main/java/org/mockito/internal/stubbing/StubbedInvocationMatcher.java
index e88c85b2d1..0b1e4f4ba9 100644
--- a/src/main/java/org/mockito/internal/stubbing/StubbedInvocationMatcher.java
+++ b/src/main/java/org/mockito/internal/stubbing/StubbedInvocationMatcher.java
@@ -43,6 +43,10 @@ public void addAnswer(Answer answer) {
         answers.add(answer);
     }
 
+    /**
+     * Marks the stub as used by the passed invocation. Passing null marks the stub as unused.
+     * @param usedAt The invocation which uses this stub
+     */
     public void markStubUsed(DescribedInvocation usedAt) {
         this.usedAt = usedAt;
     }
diff --git a/src/main/java/org/mockito/invocation/Invocation.java b/src/main/java/org/mockito/invocation/Invocation.java
index 04d14907f8..ed25fd4c69 100644
--- a/src/main/java/org/mockito/invocation/Invocation.java
+++ b/src/main/java/org/mockito/invocation/Invocation.java
@@ -81,7 +81,8 @@ public interface Invocation extends InvocationOnMock, DescribedInvocation {
     StubInfo stubInfo();
 
     /**
-     * Marks this invocation as stubbed.
+     * Marks this invocation as stubbed. Passing null resets the invocation state back to not
+     * stubbed.
      *
      * @param stubInfo the information about stubbing.
      */


Test patch:
diff --git a/src/test/java/org/mockito/internal/reporting/PluralizerTest.java b/src/test/java/org/mockito/internal/reporting/PluralizerTest.java
index c1be4382c1..3e9a2fc28f 100644
--- a/src/test/java/org/mockito/internal/reporting/PluralizerTest.java
+++ b/src/test/java/org/mockito/internal/reporting/PluralizerTest.java
@@ -25,4 +25,11 @@ public void pluralizes_interactions() {
         assertEquals("was exactly 1 interaction", Pluralizer.were_exactly_x_interactions(1));
         assertEquals("were exactly 100 interactions", Pluralizer.were_exactly_x_interactions(100));
     }
+
+    @Test
+    public void pluralizes_stubbings() {
+        assertEquals("stubbings", Pluralizer.stubbings(0));
+        assertEquals("stubbing", Pluralizer.stubbings(1));
+        assertEquals("stubbings", Pluralizer.stubbings(100));
+    }
 }
diff --git a/src/test/java/org/mockitousage/junitrule/StrictJUnitRuleTest.java b/src/test/java/org/mockitousage/junitrule/StrictJUnitRuleTest.java
index 98b17915b6..6dbe3d83c2 100644
--- a/src/test/java/org/mockitousage/junitrule/StrictJUnitRuleTest.java
+++ b/src/test/java/org/mockitousage/junitrule/StrictJUnitRuleTest.java
@@ -140,7 +140,7 @@ public void doAssert(Throwable t) {
                 assertEquals(filterLineNo("\n" +
                         "Unnecessary stubbings detected.\n" +
                         "Clean & maintainable test code requires zero unnecessary code.\n" +
-                        "Following stubbings are unnecessary (click to navigate to relevant line of code):\n" +
+                        "There are 2 unnecessary stubbings (click to navigate to relevant line of code):\n" +
                         "  1. -> at org.mockitousage.junitrule.StrictJUnitRuleTest.unused_stubs_with_multiple_mocks(StrictJUnitRuleTest.java:0)\n" +
                         "  2. -> at org.mockitousage.junitrule.StrictJUnitRuleTest.unused_stubs_with_multiple_mocks(StrictJUnitRuleTest.java:0)\n" +
                         "Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class."), filterLineNo(t.getMessage()));
diff --git a/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java b/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java
index 09e1e70923..394d013a04 100644
--- a/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java
+++ b/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java
@@ -50,7 +50,7 @@ public void lists_all_unused_stubs_cleanly() {
         assertEquals("\n" +
                         "Unnecessary stubbings detected in test class: HasUnnecessaryStubs\n" +
                         "Clean & maintainable test code requires zero unnecessary code.\n" +
-                        "Following stubbings are unnecessary (click to navigate to relevant line of code):\n" +
+                        "There are 2 unnecessary stubbings (click to navigate to relevant line of code):\n" +
                         "  1. -> at org.mockitousage.junitrunner.UnusedStubsExceptionMessageTest$HasUnnecessaryStubs.<init>(UnusedStubsExceptionMessageTest.java:0)\n" +
                         "  2. -> at org.mockitousage.junitrunner.UnusedStubsExceptionMessageTest$HasUnnecessaryStubs.<init>(UnusedStubsExceptionMessageTest.java:0)\n" +
                         "Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.",
diff --git a/src/test/java/org/mockitousage/stubbing/StrictStubbingTest.java b/src/test/java/org/mockitousage/stubbing/StrictStubbingTest.java
index 96e6a9f346..37195cb29a 100644
--- a/src/test/java/org/mockitousage/stubbing/StrictStubbingTest.java
+++ b/src/test/java/org/mockitousage/stubbing/StrictStubbingTest.java
@@ -4,11 +4,16 @@
  */
 package org.mockitousage.stubbing;
 
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.BDDMockito.given;
+import static org.mockito.BDDMockito.willReturn;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockitoutil.ThrowableAssert.assertThat;
 
+import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert;
 import org.junit.After;
 import org.junit.Test;
 import org.mockito.Mock;
@@ -99,4 +104,68 @@ public void run() {
             }
         }).throwsException(UnnecessaryStubbingException.class);
     }
+
+    private static void assertUnusedStubbingsWhen(ThrowableAssert.ThrowingCallable shouldRaiseThrowable, int unusedStubbingsCount) {
+        Assertions.assertThatThrownBy(shouldRaiseThrowable)
+            .isInstanceOf(UnnecessaryStubbingException.class)
+            .hasMessageContaining(unusedStubbingsCount + " unnecessary stubbing");
+    }
+
+    @Test public void repeated_unnecessary_stubbing() {
+        given(mock.simpleMethod("1")).willReturn("one");
+        given(mock.simpleMethod("2")).willReturn("three");
+        given(mock.simpleMethod("1")).willReturn("two");  //repeat 1x
+        given(mock.simpleMethod("1")).willReturn("four"); //repeat 2x
+
+        mock.simpleMethod("1");
+        mock.simpleMethod("2");
+
+        assertUnusedStubbingsWhen(() -> mockito.finishMocking(), 2);
+    }
+
+    @Test public void repeated_unnecessary_stubbing_willReturn() {
+        willReturn("one").given(mock).simpleMethod("1");
+        willReturn("three").given(mock).simpleMethod("2");
+        willReturn("two").given(mock).simpleMethod("1");
+        willReturn("four").given(mock).simpleMethod("1");
+
+        mock.simpleMethod("1");
+        mock.simpleMethod("2");
+
+        assertUnusedStubbingsWhen(() -> mockito.finishMocking(), 2);
+    }
+
+    @Test public void unnecessary_stubbing_with_any_matchers() {
+        given(mock.simpleMethod((String) any())).willReturn("one");
+        given(mock.simpleMethod(anyString())).willReturn("three");
+
+        mock.simpleMethod("1");
+
+        assertUnusedStubbingsWhen(() -> mockito.finishMocking(), 1);
+    }
+
+    @Test public void unnecessary_stubbing_with_mixed_matchers() {
+        given(mock.simpleMethod(anyString())).willReturn("one");
+        given(mock.simpleMethod("foo")).willReturn("three");
+
+        mock.simpleMethod("1");
+
+        assertUnusedStubbingsWhen(() -> mockito.finishMocking(), 1);
+    }
+
+    @Test public void unnecessary_stubbing_with_stubs_only() {
+        given(mock.simpleMethod(anyString())).willReturn("one");
+        given(mock.simpleMethod("foo")).willReturn("three");
+
+        assertUnusedStubbingsWhen(() -> mockito.finishMocking(), 2);
+    }
+
+    @Test public void unnecessary_stubbing_with_mixed_matchers_willReturn() {
+        willReturn("one").given(mock).simpleMethod(anyString());
+        willReturn("three").given(mock).simpleMethod("foo");
+
+        mock.simpleMethod("1");
+
+        assertUnusedStubbingsWhen(() -> mockito.finishMocking(), 1);
+    }
 }