Repository: mockito/mockito
Issue: Verification using After and ArgumentCaptor with Times or AtLeast methods returns unexpected size of captured values list.
Labels: enhancement

Hi!
I was looking issue #345 and found that the test "shouldReturnListOfArgumentsWithSameSizeAsGivenInAtMostVerification" (located in VerificationAfterDelayTest.java) fails if you use atLeast() or times() instead of atMost().
I created these tests:

![screenshot from 2016-03-24 16 50 29](https://cloud.githubusercontent.com/assets/17257307/14034443/8a3d8b98-f1e0-11e5-856a-65dc20a4fa6b.png)


Issue comments:
@thesnowgoose 
That a bug in VerificationOverTimeImpl#verify(VerificationData). The passed `VerificationData` contains all invocations, in your case "0","1" and "2". During the specified 200ms these invocations are verified in a loop without delay. That means if your CPU is able to verify the VerificationData 10,000 times during that 200ms the captor captures 30,000 arguments (10000 \* 3 arguments ). 

The implementation of `After` uses `VerificationOverTimeImpl` and that class performs the verify-loop until the time is elapsed.  This behaviour is fine for `Timeout` but violates the specification of  `Mockito.after(long)`, this is what the JavaDoc says:

> Allows verifying over a given period. It causes a verify to **wait for a specified period of time** for a desired interaction

In other words the `After` implementation must use some kind of `VerificationAfterDelayImpl`, that simply waits and than verify the invocations.

I have a solution and will provide a PR.

PR #380 relates to this issue.

Fixed by #380 

Why is this closed? I ran into the issue and lost about an hour before realizing it is a Mockito bug. What is the current status? It is not fixed in the 1.x branch. I am using 1.10.19 which seems the most recent version.

@jmborer can you test it with the latest 2.0.0-beta build, it should be fixed

OK. Thanx for the info, but I don't feel confortable to use a beta version to test operational sofware...

The beta is almost over, I hope to publish a release candidate in the
coming days. You should be able to upgrade without much problems, since the
breaking changes are kept at a minimum.

On Thu, 18 Aug 2016, 16:12 Jean-Marc Borer, notifications@github.com
wrote:

> OK. Thanx for the info, but I don't feel confortable to use a beta version
> to test operational sofware...
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> https://github.com/mockito/mockito/issues/379#issuecomment-240735180,
> or mute the thread
> https://github.com/notifications/unsubscribe-auth/AFrDb3NREzSakS3dFBKOKP-cyi5DMdb5ks5qhGhXgaJpZM4H4c7l
> .

@jmborer mockito 2 beta is fairly stable, the API should not move much. Yet there's some incompatible change with 1.x

> I don't feel confortable to use a beta version to test operational sofware...

Remember it's for tests not to run on your production servers/containers

Sure. I won't use in production. I'll wait a little until RC is out to do the migration. I already noticed it is no  longer compatible with PowerMock. I have to check if I need it anymore then my problem is solved. 

Was this actually merged into a release? I initially found the issue while running 2.0.41-beta, so I tried 2.4.0 and 2.8.9, but I'm still seeing the issue, specifically with after() and times(). 


Still encounter this issue in 3.6.0 (also tested latest 3.12.4) when using `after()`

Pull request review comments:
Since this method is duplicate from the method added in the class below, maybe we can extract an superclass `InvocationsChecker`. This class abstracts away all the logic regarding obtaining and reporting the failures, the constructor as well as this new method.


Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/verification/Times.java b/src/main/java/org/mockito/internal/verification/Times.java
index 62a7ed753f..f43ce4286c 100644
--- a/src/main/java/org/mockito/internal/verification/Times.java
+++ b/src/main/java/org/mockito/internal/verification/Times.java
@@ -31,12 +31,16 @@ public Times(int wantedNumberOfInvocations) {
     }
     
     public void verify(VerificationData data) {
+        List<Invocation> invocations = data.getAllInvocations();
+        InvocationMatcher wanted = data.getWanted();
+
         if (wantedCount > 0) {
             MissingInvocationChecker missingInvocation = new MissingInvocationChecker();
-            missingInvocation.check(data.getAllInvocations(), data.getWanted());
+            missingInvocation.check(invocations, wanted);
         }
+
         NumberOfInvocationsChecker numberOfInvocations = new NumberOfInvocationsChecker();
-        numberOfInvocations.check(data.getAllInvocations(), data.getWanted(), wantedCount);
+        numberOfInvocations.check(invocations, wanted, wantedCount);
     }
     
     public void verifyInOrder(VerificationDataInOrder data) {
diff --git a/src/main/java/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsChecker.java b/src/main/java/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsChecker.java
index cc29e0963c..f21489edf8 100644
--- a/src/main/java/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsChecker.java
+++ b/src/main/java/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsChecker.java
@@ -5,6 +5,7 @@
 
 package org.mockito.internal.verification.checkers;
 
+import java.util.Iterator;
 import java.util.List;
 
 import org.mockito.exceptions.Reporter;
@@ -28,7 +29,17 @@ public void check(List<Invocation> invocations, InvocationMatcher wanted, int wa
             Location lastLocation = finder.getLastLocation(actualInvocations);
             reporter.tooLittleActualInvocations(new AtLeastDiscrepancy(wantedCount, actualCount), wanted, lastLocation);        
         }
-        
+
+        removeAlreadyVerified(actualInvocations);
         invocationMarker.markVerified(actualInvocations, wanted);
     }
+
+    private void removeAlreadyVerified(List<Invocation> invocations) {
+        for (Iterator<Invocation> iterator = invocations.iterator(); iterator.hasNext(); ) {
+            Invocation i = iterator.next();
+            if (i.isVerified()) {
+                iterator.remove();
+            }
+        }
+    }
 }
\ No newline at end of file
diff --git a/src/main/java/org/mockito/internal/verification/checkers/NumberOfInvocationsChecker.java b/src/main/java/org/mockito/internal/verification/checkers/NumberOfInvocationsChecker.java
index 17dec9ff2e..2529095c46 100644
--- a/src/main/java/org/mockito/internal/verification/checkers/NumberOfInvocationsChecker.java
+++ b/src/main/java/org/mockito/internal/verification/checkers/NumberOfInvocationsChecker.java
@@ -5,6 +5,7 @@
 
 package org.mockito.internal.verification.checkers;
 
+import java.util.Iterator;
 import java.util.List;
 
 import org.mockito.exceptions.Reporter;
@@ -44,7 +45,17 @@ public void check(List<Invocation> invocations, InvocationMatcher wanted, int wa
             Location firstUndesired = actualInvocations.get(wantedCount).getLocation();
             reporter.tooManyActualInvocations(wantedCount, actualCount, wanted, firstUndesired);
         }
-        
+
+        removeAlreadyVerified(actualInvocations);
         invocationMarker.markVerified(actualInvocations, wanted);
     }
+
+    private void removeAlreadyVerified(List<Invocation> invocations) {
+        for (Iterator<Invocation> iterator = invocations.iterator(); iterator.hasNext(); ) {
+            Invocation i = iterator.next();
+            if (i.isVerified()) {
+                iterator.remove();
+            }
+        }
+    }
 }
\ No newline at end of file


Test patch:
diff --git a/src/test/java/org/mockitousage/verification/VerificationAfterDelayTest.java b/src/test/java/org/mockitousage/verification/VerificationAfterDelayTest.java
index e0d4f76c71..2462f28e45 100644
--- a/src/test/java/org/mockitousage/verification/VerificationAfterDelayTest.java
+++ b/src/test/java/org/mockitousage/verification/VerificationAfterDelayTest.java
@@ -130,6 +130,38 @@ public void shouldReturnListOfArgumentsWithSameSizeAsGivenInAtMostVerification()
         assertEquals("2", stringArgumentCaptor.getAllValues().get(2));
     }
 
+    @Test
+    public void shouldReturnListOfArgumentsWithSameSizeAsGivenInTimesVerification() {
+        // given
+        int n = 3;
+
+        // when
+        exerciseMockNTimes(n);
+
+        //Then
+        verify(mock, after(200).times(n)).add(stringArgumentCaptor.capture());
+        assertEquals(n, stringArgumentCaptor.getAllValues().size());
+        assertEquals("0", stringArgumentCaptor.getAllValues().get(0));
+        assertEquals("1", stringArgumentCaptor.getAllValues().get(1));
+        assertEquals("2", stringArgumentCaptor.getAllValues().get(2));
+    }
+
+    @Test
+    public void shouldReturnListOfArgumentsWithSameSizeAsGivenInAtLeastVerification() {
+        // given
+        int n = 3;
+
+        // when
+        exerciseMockNTimes(n);
+
+        //Then
+        verify(mock, after(200).atLeast(n)).add(stringArgumentCaptor.capture());
+        assertEquals(n, stringArgumentCaptor.getAllValues().size());
+        assertEquals("0", stringArgumentCaptor.getAllValues().get(0));
+        assertEquals("1", stringArgumentCaptor.getAllValues().get(1));
+        assertEquals("2", stringArgumentCaptor.getAllValues().get(2));
+    }
+
     private void exerciseMockNTimes(int n) {
         for (int i = 0; i < n; i++) {
             mock.add(String.valueOf(i));