Repository: mockito/mockito
Issue: Verification using After and ArgumentCaptor returns unexpected size of captured values list
Labels: ready

Hi, I found weird behavior of Mockito (ver. 1.10.19) when verifying method invocation with after() and ArgumentCaptor. The list of captured values returned by ArgumentCaptor has size much bigger than I expect. Moreover this behavior in connection with long timeout leads to exceeding memory limit. Example below. Is it expected behavior or bug?

``` java
package org.mockito.test;

import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.after;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

public class MockitoAfterFeatureTest {

    private RequestSender requestSender;
    private RequestProcessor requestProcessor;
    private ArgumentCaptor<String> stringArgumentCaptor;

    @Before
    public void doBeforeTest() {
        requestSender = mock(RequestSender.class);
        requestProcessor = new RequestProcessor(requestSender);
        stringArgumentCaptor = ArgumentCaptor.forClass(String.class);
    }

    @Test
    public void shouldVerifyUsingAfterAndArgumentCaptor() {
        // when
        requestProcessor.process("first_request");
        requestProcessor.process("second_request");

        // then
        verify(requestSender, after(2500).times(2)).send(stringArgumentCaptor.capture());
        assertEquals("first_request", stringArgumentCaptor.getAllValues().get(0));
        assertEquals("second_request", stringArgumentCaptor.getAllValues().get(1));

        assertEquals(2, stringArgumentCaptor.getAllValues().size());
    }

    public static class RequestProcessor {
        private final RequestSender requestSender;

        public RequestProcessor(RequestSender requestSender) {
            this.requestSender = requestSender;
        }

        public void process(String request) {
            requestSender.send(request);
        }
    }

    public interface RequestSender {
        void send(String request);
    }
}
```


Issue comments:
Issue #345 resolves this for atMost verification. What about other verification modes (i.e. times, atLeast)? Are you working on that, @tarnowskijan? Was the issue only for the atMost case? Because in the code from the description you used times instead of atMost.

@thesnowgoose, unfortunately you're right, the issue is still there when you use times() in combination with after(). I only fixed the problem with after() and atMost() and I don't know why I was focused only on this case. I'll try to fix the whole issue in the near future, but feel free to propose your own solution if you already have one.

Actually I'm working on it. There is also an issue when you use atLeast(), I'll try to fix it too.
Regards!!. @tarnowskijan 

Personally, I use extensively timeout and after in my tests, because their use is significantly different. Replace those by a single await can seem interesting at a first glance, but it is actually not.

@jmborer Could you elaborate ?

Well I have to test in a lot of asynch situations. Sometimes I need to wait for a interaction, continue quickly if it occurs before a given amount of time and fail otherwise. There I use timeout(). Then I have situations where I want to wait a given time and THEN only do the check. This is where I use after() instead of Thread.sleep()+verify() which I find less fluent to read...

@jmborer Thanks for the feedback I linked your comment in #472 

will be fixed with #936 aka `within(duration,timeunit)`
Will this be fixed? Mockito 2.19.0 still has this problem and the mentioned pull request which could resolve this (https://github.com/mockito/mockito/pull/936) was put on hold. Please either fix this or provide a way to express the after(x).atLeast(y) without having the argument captor returning a collection with millions [sic] of items via `ArgumentCaptor#getAllValues()` although the method was called only a couple of times.
@Trinova #936 needs to be reviewed by a core member, there is nothing I can do as contributor.  I don't know if it will ever happen since the core-team is busy and doesn't plan to hire new core member. Maybe the community should fork Mockito and release it with an other group and artefact-id to overcome the prolonged review process. 

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/verification/AtMost.java b/src/main/java/org/mockito/internal/verification/AtMost.java
index 54a5285c47..25a7fc59d1 100644
--- a/src/main/java/org/mockito/internal/verification/AtMost.java
+++ b/src/main/java/org/mockito/internal/verification/AtMost.java
@@ -5,6 +5,7 @@
 
 package org.mockito.internal.verification;
 
+import java.util.Iterator;
 import java.util.List;
 
 import org.mockito.exceptions.Reporter;
@@ -38,7 +39,8 @@ public void verify(VerificationData data) {
         if (foundSize > maxNumberOfInvocations) {
             new Reporter().wantedAtMostX(maxNumberOfInvocations, foundSize);
         }
-        
+
+        removeAlreadyVerified(found);
         invocationMarker.markVerified(found, wanted);
     }
 
@@ -46,4 +48,13 @@ public void verify(VerificationData data) {
     public VerificationMode description(String description) {
         return VerificationModeFactory.description(this, description);
     }
+
+    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 d20c39d8ba..e0d4f76c71 100644
--- a/src/test/java/org/mockitousage/verification/VerificationAfterDelayTest.java
+++ b/src/test/java/org/mockitousage/verification/VerificationAfterDelayTest.java
@@ -15,6 +15,8 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.exceptions.base.MockitoAssertionError;
 import org.mockitoutil.TestBase;
@@ -29,6 +31,9 @@ public class VerificationAfterDelayTest extends TestBase {
 
     private List<Exception> exceptions = new LinkedList<Exception>();
 
+    @Captor
+    private ArgumentCaptor<String> stringArgumentCaptor;
+
     @After
     public void teardown() {
         // making sure there are no threading related exceptions
@@ -106,6 +111,31 @@ public void shouldStopEarlyIfTestIsDefinitelyFailed() throws Exception {
         verify(mock, after(10000).never()).clear();
     }
 
+    /**
+     * Test for issue #345.
+     */
+    @Test
+    public void shouldReturnListOfArgumentsWithSameSizeAsGivenInAtMostVerification() {
+        // given
+        int n = 3;
+
+        // when
+        exerciseMockNTimes(n);
+
+        // then
+        verify(mock, after(200).atMost(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));
+        }
+    }
+
     private Thread waitAndExerciseMock(final int sleep) {
         Thread t = new Thread() {