Repository: mockito/mockito
Issue: VerificationCollector doesn't work for invocations with non-matching args

I found the incubating `VerificationCollector` (#287), which is similar to AssertJ `SoftAssertion`s in that it collects multiple verifications and then asserts on the whole, rather than throwing an exception on the first one and skipping the rest of the verifications. Very useful for complicated testing.

However, I found that the verification collector doesn't work for verifications on method invocations with parameters. I had a quick check of the source code of the test, and I noticed that all of the tests in `VerificationCollectorImplTest` use no-arg invocations.

Hopefully this would be an easy fix, because this looks like an extremely useful feature for my application but I need to verify invocations with arguments. At the moment, my workaround is to wrap each verification call in an AssertJ soft assertion, but this makes writing the tests a bit more tedious and less readable.



Issue comments:
Update: I found that the lazy verification works properly when you use `VerificationCollector` directly. However, if you try and use it as a JUnit rule that's when this bug manifests.

I have updated `VerificationCollectorImplTest` to properly test for this situation and fail as appropriate. I will have a quick look and see if it is an easy fix; otherwise I may submit a PR with the test only and leave a more experienced hand to fix it.
Another update: I have gotten to the root of this problem.

>I found that the lazy verification works properly when you use VerificationCollector directly. However, if you try and use it as a JUnit rule that's when this bug manifests.

This wasn't quite correct. The distinguishing feature wasn't the JUnit rule, but if you do a verification for an invocation that tries to match on an argument while running under JUnit.

There is code in `org.mockito.internal.junit.ExceptionFactory` that checks to see if JUnit is on the classpath - if so, then it will throw `org.mockito.exceptions.verification.junit.ArgumentsAreDifferent` rather than `org.mockito.exceptions.verification.ArgumentsAreDifferent`. The JUnit version inherits from `junit.framework.ComparisonFailure` whereas the basic version inherits from `org.mockito.exceptions.base.MockitoAssertionError`.

It seems that the purpose of this is to help JUnit-aware GUIs to produce more helpful diagnostic output. This makes sense (although it would make more sense to derive from a more-up-to-date version of `ComparisonFailure` if it is available, like `org.junit.ComparisonFailure` - perhaps this could be the subject of a different issue/PR). However, because it is not a subclass of `MockitoAssertionError`, it skips out of the try/catch in `VerificationCollectorImpl` (see https://github.com/mockito/mockito/blob/b745b5d48e9c110fa419fd007b3159ff612b8643/src/main/java/org/mockito/internal/junit/VerificationCollectorImpl.java#L92).

There are two possible solutions:

1. Add another `catch()` to catch `ComparisonFailure` as well.
2. Broaden the catch for `MockitoAssertionError` to a common superclass (like `AssertionError`).

The second probably has more generic appeal and would future-proof the solution if other types of argument comparison were used (eg, a custom argument matcher). I will submit a PR for this shortly.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/junit/VerificationCollectorImpl.java b/src/main/java/org/mockito/internal/junit/VerificationCollectorImpl.java
index 7c19d35652..e54d47864e 100644
--- a/src/main/java/org/mockito/internal/junit/VerificationCollectorImpl.java
+++ b/src/main/java/org/mockito/internal/junit/VerificationCollectorImpl.java
@@ -75,7 +75,7 @@ private void append(String message) {
         this.numberOfFailures++;
         this.builder.append('\n')
                 .append(this.numberOfFailures).append(". ")
-                .append(message.substring(1, message.length()));
+                .append(message.trim()).append('\n');
     }
 
     private class VerificationWrapper implements VerificationMode {
@@ -89,7 +89,7 @@ private VerificationWrapper(VerificationMode delegate) {
         public void verify(VerificationData data) {
             try {
                 this.delegate.verify(data);
-            } catch (MockitoAssertionError error) {
+            } catch (AssertionError error) {
                 VerificationCollectorImpl.this.append(error.getMessage());
             }
         }


Test patch:
diff --git a/src/test/java/org/mockitousage/junitrule/VerificationCollectorImplTest.java b/src/test/java/org/mockitousage/junitrule/VerificationCollectorImplTest.java
index 8f4c420eca..9b4cc1ec1c 100644
--- a/src/test/java/org/mockitousage/junitrule/VerificationCollectorImplTest.java
+++ b/src/test/java/org/mockitousage/junitrule/VerificationCollectorImplTest.java
@@ -8,13 +8,14 @@
 import org.junit.Test;
 import org.junit.runner.JUnitCore;
 import org.junit.runner.Result;
+import org.mockito.ArgumentMatcher;
 import org.mockito.exceptions.base.MockitoAssertionError;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.VerificationCollector;
 import org.mockitousage.IMethods;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
 import static org.mockito.Mockito.*;
 
 public class VerificationCollectorImplTest {
@@ -46,14 +47,54 @@ public void should_collect_multiple_verification_failures() {
 
         IMethods methods = mock(IMethods.class);
 
+        methods.intArgumentMethod(6);
+
         verify(methods).simpleMethod();
         verify(methods).byteReturningMethod();
+        verify(methods).intArgumentMethod(8);
+        verify(methods).longArg(8L);
         try {
             collector.collectAndReport();
-            fail();
+            failBecauseExceptionWasNotThrown(MockitoAssertionError.class);
         } catch (MockitoAssertionError error) {
             assertThat(error).hasMessageContaining("1. Wanted but not invoked:");
             assertThat(error).hasMessageContaining("2. Wanted but not invoked:");
+            assertThat(error).hasMessageContaining("3. Argument(s) are different! Wanted:");
+            assertThat(error).hasMessageContaining("4. Wanted but not invoked:");
+        }
+    }
+
+    @Test
+    public void should_collect_matching_error_from_non_matching_arguments() {
+        VerificationCollector collector = MockitoJUnit.collector().assertLazily();
+
+        IMethods methods = mock(IMethods.class);
+
+        methods.intArgumentMethod(6);
+        methods.longArg(8L);
+        methods.forShort((short)6);
+
+        verify(methods).intArgumentMethod(8);
+        verify(methods).longArg(longThat(new ArgumentMatcher<Long>() {
+            @Override
+            public boolean matches(Long argument) {
+                throw new AssertionError("custom error message");
+            }
+        }));
+        verify(methods).forShort(shortThat(new ArgumentMatcher<Short>() {
+            @Override
+            public boolean matches(Short argument) {
+                return false;
+            }
+        }));
+
+        try {
+            collector.collectAndReport();
+            failBecauseExceptionWasNotThrown(MockitoAssertionError.class);
+        } catch (MockitoAssertionError error) {
+            assertThat(error).hasMessageContaining("1. Argument(s) are different! Wanted:");
+            assertThat(error).hasMessageContaining("2. custom error message");
+            assertThat(error).hasMessageContaining("3. Argument(s) are different! Wanted:");
         }
     }
 
@@ -66,7 +107,7 @@ public void should_only_collect_failures_ignore_successful_verifications() {
         verify(methods, never()).simpleMethod();
         verify(methods).byteReturningMethod();
 
-        this.assertAtLeastOneFailure(collector);
+        this.assertExactlyOneFailure(collector);
     }
 
     @Test
@@ -79,13 +120,13 @@ public void should_continue_collecting_after_failing_verification() {
         verify(methods).byteReturningMethod();
         verify(methods).simpleMethod();
 
-        this.assertAtLeastOneFailure(collector);
+        this.assertExactlyOneFailure(collector);
     }
 
-    private void assertAtLeastOneFailure(VerificationCollector collector) {
+    private void assertExactlyOneFailure(VerificationCollector collector) {
         try {
             collector.collectAndReport();
-            fail();
+            failBecauseExceptionWasNotThrown(MockitoAssertionError.class);
         } catch (MockitoAssertionError error) {
             assertThat(error).hasMessageContaining("1. Wanted but not invoked:");
             assertThat(error.getMessage()).doesNotContain("2.");
@@ -97,8 +138,11 @@ public void should_invoke_collector_rule_after_test() {
         JUnitCore runner = new JUnitCore();
         Result result = runner.run(VerificationCollectorRuleInner.class);
 
-        assertThat(result.getFailureCount()).isEqualTo(1);
-        assertThat(result.getFailures().get(0).getMessage()).contains("1. Wanted but not invoked:");
+        assertThat(result.getFailureCount()).as("failureCount").isEqualTo(2);
+        assertThat(result.getFailures().get(0).getMessage()).as("failure1").contains("1. Wanted but not invoked:");
+        assertThat(result.getFailures().get(1).getMessage()).as("failure2")
+            .contains("1. Argument(s) are different! Wanted:")
+            .contains("2. Wanted but not invoked:");
     }
 
     // This class is picked up when running a test suite using an IDE. It fails on purpose.
@@ -121,5 +165,14 @@ public void should_not_fail() {
 
             verify(methods).simpleMethod();
         }
+
+        @Test
+        public void should_fail_with_args() {
+            IMethods methods = mock(IMethods.class);
+            methods.intArgumentMethod(8);
+
+            verify(methods).intArgumentMethod(9);
+            verify(methods).byteReturningMethod();
+        }
     }
 }