Repository: mockito/mockito
Issue: ArgumentCaptor no longer working for varargs
Labels: bug

I ran into the issue described here: http://stackoverflow.com/questions/27303562/why-does-upgrading-mockito-from-1-9-5-to-1-10-8-break-this-captor


Issue comments:
Hi sorry for the late reply.
I reproduced the issue, not sure when I will be able to fix though.

fixed by #211 


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/org/mockito/internal/invocation/CapturesArgumensFromInvocation.java b/src/org/mockito/internal/invocation/CapturesArgumentsFromInvocation.java
similarity index 83%
rename from src/org/mockito/internal/invocation/CapturesArgumensFromInvocation.java
rename to src/org/mockito/internal/invocation/CapturesArgumentsFromInvocation.java
index cc58b802db..ef7bde459f 100644
--- a/src/org/mockito/internal/invocation/CapturesArgumensFromInvocation.java
+++ b/src/org/mockito/internal/invocation/CapturesArgumentsFromInvocation.java
@@ -7,7 +7,7 @@
 
 import org.mockito.invocation.Invocation;
 
-public interface CapturesArgumensFromInvocation {
+public interface CapturesArgumentsFromInvocation {
     
     void captureArgumentsFrom(Invocation i);
     
diff --git a/src/org/mockito/internal/invocation/InvocationMarker.java b/src/org/mockito/internal/invocation/InvocationMarker.java
index 304fdb2ec5..d44cf9f0cd 100644
--- a/src/org/mockito/internal/invocation/InvocationMarker.java
+++ b/src/org/mockito/internal/invocation/InvocationMarker.java
@@ -11,18 +11,18 @@
 
 public class InvocationMarker {
 
-    public void markVerified(List<Invocation> invocations, CapturesArgumensFromInvocation wanted) {
+    public void markVerified(List<Invocation> invocations, CapturesArgumentsFromInvocation wanted) {
         for (Invocation invocation : invocations) {
             markVerified(invocation, wanted);
         }
     }
 
-    public void markVerified(Invocation invocation, CapturesArgumensFromInvocation wanted) {
-        invocation.markVerified();
-        wanted.captureArgumentsFrom(invocation);
-    }
+	public void markVerified(Invocation invocation, CapturesArgumentsFromInvocation wanted) {
+		invocation.markVerified();
+		wanted.captureArgumentsFrom(invocation);
+	}
 
-    public void markVerifiedInOrder(List<Invocation> chunk, CapturesArgumensFromInvocation wanted, InOrderContext context) {
+    public void markVerifiedInOrder(List<Invocation> chunk, CapturesArgumentsFromInvocation wanted, InOrderContext context) {
         markVerified(chunk, wanted);
         
         for (Invocation i : chunk) {
diff --git a/src/org/mockito/internal/invocation/InvocationMatcher.java b/src/org/mockito/internal/invocation/InvocationMatcher.java
index f1b7b9421f..0e562a0129 100644
--- a/src/org/mockito/internal/invocation/InvocationMatcher.java
+++ b/src/org/mockito/internal/invocation/InvocationMatcher.java
@@ -8,20 +8,18 @@
 import java.io.Serializable;
 import java.lang.reflect.Array;
 import java.lang.reflect.Method;
-import java.util.Collections;
-import java.util.LinkedList;
-import java.util.List;
+import java.util.*;
+
 import org.hamcrest.Matcher;
 import org.mockito.internal.matchers.CapturesArguments;
 import org.mockito.internal.matchers.MatcherDecorator;
-import org.mockito.internal.matchers.VarargMatcher;
 import org.mockito.internal.reporting.PrintSettings;
 import org.mockito.invocation.DescribedInvocation;
 import org.mockito.invocation.Invocation;
 import org.mockito.invocation.Location;
 
 @SuppressWarnings("unchecked")
-public class InvocationMatcher implements DescribedInvocation, CapturesArgumensFromInvocation, Serializable {
+public class InvocationMatcher implements DescribedInvocation, CapturesArgumentsFromInvocation, Serializable {
 
     private static final long serialVersionUID = -3047126096857467610L;
     private final Invocation invocation;
@@ -99,16 +97,16 @@ public boolean hasSameMethod(Invocation candidate) {
         Method m2 = candidate.getMethod();
         
         if (m1.getName() != null && m1.getName().equals(m2.getName())) {
-            /* Avoid unnecessary cloning */
-            Class[] params1 = m1.getParameterTypes();
-            Class[] params2 = m2.getParameterTypes();
-            if (params1.length == params2.length) {
-                for (int i = 0; i < params1.length; i++) {
-                if (params1[i] != params2[i])
-                    return false;
-                }
-                return true;
-            }
+        	/* Avoid unnecessary cloning */
+        	Class[] params1 = m1.getParameterTypes();
+        	Class[] params2 = m2.getParameterTypes();
+        	if (params1.length == params2.length) {
+        	    for (int i = 0; i < params1.length; i++) {
+        		if (params1[i] != params2[i])
+        		    return false;
+        	    }
+        	    return true;
+        	}
         }
         return false;
     }
@@ -118,46 +116,55 @@ public Location getLocation() {
     }
 
     public void captureArgumentsFrom(Invocation invocation) {
-        for (int position = 0; position < matchers.size(); position++) {
+        captureArguments(invocation);
+        if (invocation.getMethod().isVarArgs()) {
+            captureVarargsPart(invocation);
+        }
+    }
+
+    private void captureArguments(Invocation invocation) {
+        for (int position = 0; position < regularArgumentsSize(invocation); position++) {
             Matcher m = matchers.get(position);
-            if (m instanceof CapturesArguments && invocation.getRawArguments().length > position) {
-                //TODO SF - this whole lot can be moved captureFrom implementation
-                if(isVariableArgument(invocation, position) && isVarargMatcher(m)) {
-                    Object array = invocation.getRawArguments()[position];
-                    for (int i = 0; i < Array.getLength(array); i++) {
-                        ((CapturesArguments) m).captureFrom(Array.get(array, i));
-                    }
-                    //since we've captured all varargs already, it does not make sense to process other matchers.
-                    return;
-                } else {
-                    ((CapturesArguments) m).captureFrom(invocation.getRawArguments()[position]);
-                }
+            if (m instanceof CapturesArguments) {
+                ((CapturesArguments) m).captureFrom(invocation.getArgumentAt(position, Object.class));
             }
         }
     }
 
-    private boolean isVarargMatcher(Matcher matcher) {
-        Matcher actualMatcher = matcher;
-        if (actualMatcher instanceof MatcherDecorator) {
-            actualMatcher = ((MatcherDecorator) actualMatcher).getActualMatcher();
+    private void captureVarargsPart(Invocation invocation) {
+        int indexOfVararg = invocation.getRawArguments().length - 1;
+        for (Matcher m : uniqueMatcherSet(indexOfVararg)) {
+            if (m instanceof CapturesArguments) {
+                Object rawArgument = invocation.getRawArguments()[indexOfVararg];
+                for (int i = 0; i < Array.getLength(rawArgument); i++) {
+                    ((CapturesArguments) m).captureFrom(Array.get(rawArgument, i));
+                }
+            }
         }
-        return actualMatcher instanceof VarargMatcher;
     }
 
-    private boolean isVariableArgument(Invocation invocation, int position) {
-        return invocation.getRawArguments().length - 1 == position
-                && invocation.getRawArguments()[position] != null
-                && invocation.getRawArguments()[position].getClass().isArray()
-                && invocation.getMethod().isVarArgs();
+    private int regularArgumentsSize(Invocation invocation) {
+        return invocation.getMethod().isVarArgs() ? invocation.getRawArguments().length - 1 : matchers.size();
+    }
+
+    private Set<Matcher> uniqueMatcherSet(int indexOfVararg) {
+        HashSet<Matcher> set = new HashSet<Matcher>();
+        for (int position = indexOfVararg; position < matchers.size(); position++) {
+            Matcher matcher = matchers.get(position);
+            if(matcher instanceof MatcherDecorator) {
+                set.add(((MatcherDecorator) matcher).getActualMatcher());
+            } else {
+                set.add(matcher);
+            }
+        }
+        return set;
     }
 
     public static List<InvocationMatcher> createFrom(List<Invocation> invocations) {
         LinkedList<InvocationMatcher> out = new LinkedList<InvocationMatcher>();
-
         for (Invocation i : invocations) {
             out.add(new InvocationMatcher(i));
         }
-
         return out;
     }
 }


Test patch:
diff --git a/test/org/mockito/internal/invocation/InvocationMarkerTest.java b/test/org/mockito/internal/invocation/InvocationMarkerTest.java
index 085b9a808b..d85320c485 100644
--- a/test/org/mockito/internal/invocation/InvocationMarkerTest.java
+++ b/test/org/mockito/internal/invocation/InvocationMarkerTest.java
@@ -35,7 +35,7 @@ public void shouldCaptureArguments() {
         InvocationMarker marker = new InvocationMarker();
         Invocation i = new InvocationBuilder().toInvocation();
         final ObjectBox box = new ObjectBox();
-        CapturesArgumensFromInvocation c = new CapturesArgumensFromInvocation() {
+        CapturesArgumentsFromInvocation c = new CapturesArgumentsFromInvocation() {
             public void captureArgumentsFrom(Invocation i) {
                 box.put(i);
             }};
diff --git a/test/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsCheckerTest.java b/test/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsCheckerTest.java
index f067708f93..9dfb7ca1c7 100644
--- a/test/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsCheckerTest.java
+++ b/test/org/mockito/internal/verification/checkers/AtLeastXNumberOfInvocationsCheckerTest.java
@@ -28,6 +28,6 @@ public void shouldMarkActualInvocationsAsVerified() {
         c.check(asList(invocation, invocationTwo), new InvocationMatcher(invocation), 1);
 
         //then
-        Mockito.verify(c.invocationMarker).markVerified(eq(asList(invocation)), any(CapturesArgumensFromInvocation.class));
+        Mockito.verify(c.invocationMarker).markVerified(eq(asList(invocation)), any(CapturesArgumentsFromInvocation.class));
     }
 }
diff --git a/test/org/mockitousage/matchers/CapturingArgumentsTest.java b/test/org/mockitousage/matchers/CapturingArgumentsTest.java
index d833e681e2..553e913dd7 100644
--- a/test/org/mockitousage/matchers/CapturingArgumentsTest.java
+++ b/test/org/mockitousage/matchers/CapturingArgumentsTest.java
@@ -5,10 +5,10 @@
 
 package org.mockitousage.matchers;
 
+import org.junit.Assert;
 import org.fest.assertions.Assertions;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
-import org.mockito.Mock;
 import org.mockito.exceptions.base.MockitoException;
 import org.mockito.exceptions.verification.WantedButNotInvoked;
 import org.mockitousage.IMethods;
@@ -23,8 +23,6 @@
 
 public class CapturingArgumentsTest extends TestBase {
 
-    @Mock IMethods mock;
-    
     class Person {
 
         private final Integer age;
@@ -37,12 +35,12 @@ public int getAge() {
             return age;
         }
     }
-    
-    class Emailer {
-     
+
+    class BulkEmailService {
+
         private EmailService service;
-        
-        public Emailer(EmailService service) {
+
+        public BulkEmailService(EmailService service) {
             this.service = service;
         }
 
@@ -53,50 +51,53 @@ public void email(Integer ... personId) {
             }
         }
     }
-    
+
     interface EmailService {
         boolean sendEmailTo(Person person);
     }
 
     EmailService emailService = mock(EmailService.class);
-    Emailer emailer = new Emailer(emailService);
+    BulkEmailService bulkEmailService = new BulkEmailService(emailService);
+    IMethods mock = mock(IMethods.class);
 
     @SuppressWarnings("deprecation")
     @Test
     public void should_allow_assertions_on_captured_argument() {
+        //given
+        ArgumentCaptor<Person> argument = new ArgumentCaptor<Person>();
+
         //when
-        emailer.email(12);
-        
+        bulkEmailService.email(12);
+
         //then
-        ArgumentCaptor<Person> argument = new ArgumentCaptor<Person>();
         verify(emailService).sendEmailTo(argument.capture());
-        
         assertEquals(12, argument.getValue().getAge());
     }
     
     @Test
     public void should_allow_assertions_on_all_captured_arguments() {
+        //given
+        ArgumentCaptor<Person> argument = ArgumentCaptor.forClass(Person.class);
+
         //when
-        emailer.email(11, 12);
-        
+        bulkEmailService.email(11, 12);
+
         //then
-        ArgumentCaptor<Person> argument = ArgumentCaptor.forClass(Person.class);
-        verify(emailService, atLeastOnce()).sendEmailTo(argument.capture());
-        List<Person> allValues = argument.getAllValues();
-        
-        assertEquals(11, allValues.get(0).getAge());
-        assertEquals(12, allValues.get(1).getAge());
+        verify(emailService, times(2)).sendEmailTo(argument.capture());
+        assertEquals(11, argument.getAllValues().get(0).getAge());
+        assertEquals(12, argument.getAllValues().get(1).getAge());
     }
     
     @Test
     public void should_allow_assertions_on_last_argument() {
+        //given
+        ArgumentCaptor<Person> argument = ArgumentCaptor.forClass(Person.class);
+
         //when
-        emailer.email(11, 12, 13);
-        
+        bulkEmailService.email(11, 12, 13);
+
         //then
-        ArgumentCaptor<Person> argument = ArgumentCaptor.forClass(Person.class);
-        verify(emailService, atLeastOnce()).sendEmailTo(argument.capture());
-        
+        verify(emailService, times(3)).sendEmailTo(argument.capture());
         assertEquals(13, argument.getValue().getAge());
     }
     
@@ -117,11 +118,13 @@ public void should_print_captor_matcher() {
     
     @Test
     public void should_allow_assertions_on_captured_null() {
+        //given
+        ArgumentCaptor<Person> argument = ArgumentCaptor.forClass(Person.class);
+
         //when
         emailService.sendEmailTo(null);
-        
+
         //then
-        ArgumentCaptor<Person> argument = ArgumentCaptor.forClass(Person.class);
         verify(emailService).sendEmailTo(argument.capture());
         assertEquals(null, argument.getValue());
     }
@@ -130,12 +133,14 @@ public void should_allow_assertions_on_captured_null() {
     public void should_allow_construction_of_captor_for_parameterized_type_in_a_convenient_way()  {
         //the test passes if this expression compiles
         ArgumentCaptor<List<Person>> argument = ArgumentCaptor.forClass(List.class);
+        assertNotNull(argument);
     }
 
     @Test
     public void should_allow_construction_of_captor_for_a_more_specific_type()  {
         //the test passes if this expression compiles
         ArgumentCaptor<List> argument = ArgumentCaptor.forClass(ArrayList.class);
+        assertNotNull(argument);
     }
     
     @Test
@@ -171,20 +176,22 @@ public void should_say_something_smart_when_misused() {
         try {
             argument.getValue();
             fail();
-        } catch (MockitoException e) {}
+        } catch (MockitoException e) {
+            Assert.assertTrue(true);
+        }
     }
     
     @Test
     public void should_capture_when_full_arg_list_matches() throws Exception {
         //given
+        ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
+
+        //when
         mock.simpleMethod("foo", 1);
         mock.simpleMethod("bar", 2);
-        
-        //when
-        ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
-        verify(mock).simpleMethod(captor.capture(), eq(1));
-        
+
         //then
+        verify(mock).simpleMethod(captor.capture(), eq(1));
         assertEquals(1, captor.getAllValues().size());
         assertEquals("foo", captor.getValue());
     }
@@ -192,7 +199,6 @@ public void should_capture_when_full_arg_list_matches() throws Exception {
     @Test
     public void should_capture_int_by_creating_captor_with_primitive_wrapper() {
         //given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<Integer> argument = ArgumentCaptor.forClass(Integer.class);
 
         //when
@@ -206,7 +212,6 @@ public void should_capture_int_by_creating_captor_with_primitive_wrapper() {
     @Test
     public void should_capture_int_by_creating_captor_with_primitive() throws Exception {
         //given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<Integer> argument = ArgumentCaptor.forClass(int.class);
         
         //when
@@ -220,7 +225,6 @@ public void should_capture_int_by_creating_captor_with_primitive() throws Except
     @Test
     public void should_capture_byte_vararg_by_creating_captor_with_primitive() throws Exception {
         // given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<Byte> argumentCaptor = ArgumentCaptor.forClass(byte.class);
 
         // when
@@ -235,7 +239,6 @@ public void should_capture_byte_vararg_by_creating_captor_with_primitive() throw
     @Test
     public void should_capture_byte_vararg_by_creating_captor_with_primitive_wrapper() throws Exception {
         // given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<Byte> argumentCaptor = ArgumentCaptor.forClass(Byte.class);
 
         // when
@@ -250,7 +253,6 @@ public void should_capture_byte_vararg_by_creating_captor_with_primitive_wrapper
     @Test
     public void should_capture_vararg() throws Exception {
         // given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class);
 
         // when
@@ -264,7 +266,6 @@ public void should_capture_vararg() throws Exception {
     @Test
     public void should_capture_all_vararg() throws Exception {
         // given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class);
 
         // when
@@ -274,14 +275,12 @@ public void should_capture_all_vararg() throws Exception {
         // then
         verify(mock, times(2)).mixedVarargs(any(), argumentCaptor.capture());
 
-        List<String> allVarargsValues = argumentCaptor.getAllValues();
-        Assertions.assertThat(allVarargsValues).containsExactly("a", "b", "c", "again ?!");
+        Assertions.assertThat(argumentCaptor.getAllValues()).containsExactly("a", "b", "c", "again ?!");
     }
 
     @Test
     public void should_capture_one_arg_even_when_using_vararg_captor_on_nonvararg_method() throws Exception {
         // given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class);
 
         // when
@@ -293,9 +292,8 @@ public void should_capture_one_arg_even_when_using_vararg_captor_on_nonvararg_me
     }
 
     @Test
-    public void captures_correclty_when_captor_used_multiple_times() throws Exception {
+    public void captures_correctly_when_captor_used_multiple_times() throws Exception {
         // given
-        IMethods mock = mock(IMethods.class);
         ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class);
 
         // when
@@ -306,4 +304,17 @@ public void captures_correclty_when_captor_used_multiple_times() throws Exceptio
         verify(mock).mixedVarargs(any(), argumentCaptor.capture(), argumentCaptor.capture(), argumentCaptor.capture());
         Assertions.assertThat(argumentCaptor.getAllValues()).containsExactly("a", "b", "c");
     }
+
+    @Test
+    public void captures_correctly_when_captor_used_on_pure_vararg_method() throws Exception {
+        // given
+        ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class);
+
+        // when
+        mock.varargs(42, "capturedValue");
+
+        // then
+        verify(mock).varargs(eq(42), argumentCaptor.capture());
+        Assertions.assertThat(argumentCaptor.getValue()).contains("capturedValue");
+    }
 }
\ No newline at end of file