Started designing the code changes for issue #384

I'm planning to rework the current behavior of JUnit rules, so that the rule by default:
 - on test failure print the stubbing mismatch
 - when test passes show unused stubs

It will use the new StubbingListener interface. WarningsCollector class and friends will be deleted.

src/main/java/org/mockito/internal/debugging/WarningsCollector.java

@@ -4,16 +4,18 @@
  */
 package org.mockito.internal.debugging;
 
-import static org.mockito.internal.progress.ThreadSafeMockingProgress.mockingProgress;
-
-import java.util.LinkedList;
-import java.util.List;
 import org.mockito.internal.invocation.InvocationMatcher;
 import org.mockito.internal.invocation.UnusedStubsFinder;
 import org.mockito.internal.invocation.finder.AllInvocationsFinder;
 import org.mockito.internal.listeners.CollectCreatedMocks;
 import org.mockito.invocation.Invocation;
 
+import java.util.LinkedList;
+import java.util.List;
+
+import static org.mockito.internal.progress.ThreadSafeMockingProgress.mockingProgress;
+
+//TODO SF delete among with all code it uses
 public class WarningsCollector {
 
     private final List<Object> createdMocks;

src/main/java/org/mockito/internal/junit/JUnitRule.java

@@ -4,7 +4,7 @@
 import org.junit.runners.model.Statement;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
-import org.mockito.internal.debugging.WarningsCollector;
+import org.mockito.internal.runners.UnnecessaryStubbingsReporter;
 import org.mockito.internal.util.MockitoLogger;
 import org.mockito.junit.MockitoRule;
 
@@ -23,14 +23,24 @@ public Statement apply(final Statement base, FrameworkMethod method, final Objec
 		return new Statement() {
             @Override
             public void evaluate() throws Throwable {
-                WarningsCollector c = new WarningsCollector();
+                UnnecessaryStubbingsReporter reporter = new UnnecessaryStubbingsReporter();
+                Mockito.framework().setStubbingListener(reporter);
+                try {
+                    performEvaluation(reporter);
+                } finally {
+                    Mockito.framework().setStubbingListener(null);
+                }
+            }
+
+            private void performEvaluation(UnnecessaryStubbingsReporter reporter) throws Throwable {
                 MockitoAnnotations.initMocks(target);
                 try {
                     base.evaluate();
                 } catch(Throwable t) {
-                    logger.log(c.getWarnings());
+                    logger.log(reporter.printStubbingMismatches());
                     throw t;
                 }
+                logger.log(reporter.printUnusedStubbings());
                 Mockito.validateMockitoUsage();
             }
         };

src/main/java/org/mockito/internal/runners/StrictRunner.java

@@ -45,7 +45,7 @@ public void run(RunNotifier notifier) {
             //   Otherwise we would report unnecessary stubs even if the user runs just single test from the class
             //2. tests are successful (we don't want to add an extra failure on top of any existing failure, to avoid confusion)
             //TODO JUnit runner should have a specific message explaining to the user how it works.
-            reporter.report(testClass, notifier);
+            reporter.validateUnusedStubs(testClass, notifier);
         }
     }
 

src/main/java/org/mockito/internal/runners/UnnecessaryStubbingsReporter.java

@@ -15,7 +15,8 @@
 /**
  * Created by sfaber on 5/6/16.
  */
-class UnnecessaryStubbingsReporter implements StubbingListener {
+//TODO package rework, merge internal.junit with internal.runners. Rename this class (it's doing more than reporting unnecessary stubs)
+public class UnnecessaryStubbingsReporter implements StubbingListener {
 
     private final Map<String, Invocation> stubbings = new HashMap<String, Invocation>();
     private final Set<String> used = new HashSet<String>();
@@ -36,7 +37,7 @@ public void usedStubbing(Invocation stubbing, Invocation actual) {
         stubbings.remove(location);
     }
 
-    public void report(Class<?> testClass, RunNotifier notifier) {
+    public void validateUnusedStubs(Class<?> testClass, RunNotifier notifier) {
         if (stubbings.isEmpty()) {
             //perf tweak, bailing out early to avoid extra computation
             return;
@@ -57,4 +58,14 @@ public void report(Class<?> testClass, RunNotifier notifier) {
         notifier.fireTestFailure(new Failure(unnecessaryStubbings,
                 Reporter.formatUnncessaryStubbingException(testClass, stubbings.values())));
     }
+
+    //TODO SF I'm not completely happy with putting this functionality in this listener.
+    //I'd rather have separate listener which is cleaner and more SRPy
+    public String printStubbingMismatches() {
+        return "";
+    }
+
+    public String printUnusedStubbings() {
+        return "";
+    }
 }

src/test/java/org/mockito/internal/junit/JUnitRuleTest.java

@@ -1,5 +1,7 @@
 package org.mockito.internal.junit;
 
+import org.junit.After;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runners.model.Statement;
 import org.mockito.InjectMocks;
@@ -12,23 +14,29 @@
 import org.mockitoutil.TestBase;
 
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.mock;
 
 public class JUnitRuleTest extends TestBase {
 
     private SimpleMockitoLogger logger = new SimpleMockitoLogger();
     private JUnitRule jUnitRule = new JUnitRule(logger);
     private InjectTestCase injectTestCase = new InjectTestCase();
 
+    @After public void after() {
+        //so that the validate framework usage exceptions do not collide with the tests here
+        resetState();
+    }
+
     @Test
-    public void shouldInjectIntoTestCase() throws Throwable {
+    public void injects_into_test_case() throws Throwable {
         jUnitRule.apply(new DummyStatement(),null, injectTestCase).evaluate();
         assertNotNull("@Mock mock object created", injectTestCase.getInjected());
         assertNotNull("@InjectMocks object created", injectTestCase.getInjectInto());
         assertNotNull("Mock injected into the object", injectTestCase.getInjectInto().getInjected());
     }
 
     @Test
-    public void shouldRethrowException() throws Throwable {
+    public void rethrows_exception() throws Throwable {
         try {
             jUnitRule.apply(new ExceptionStatement(), null,injectTestCase).evaluate();
             fail("Should throw exception");
@@ -38,7 +46,7 @@ public void shouldRethrowException() throws Throwable {
     }
 
     @Test
-    public void shouldDetectUnfinishedStubbing() throws Throwable {
+    public void detects_invalid_mockito_usage_on_success() throws Throwable {
         try {
             jUnitRule.apply(new UnfinishedStubbingStatement(),null, injectTestCase).evaluate();
             fail("Should detect invalid Mockito usage");
@@ -47,11 +55,31 @@ public void shouldDetectUnfinishedStubbing() throws Throwable {
     }
 
     @Test
-    public void shouldWarnAboutUnusedStubsWhenFailed() throws Throwable {
+    public void does_not_check_invalid_mockito_usage_on_failure() throws Throwable {
+        //I am not sure that this is the intended behavior
+        //However, it was like that since the beginning of JUnit rule support
+        //Users never questioned this behavior. Hence, let's stick to it unless we have more data
         try {
             jUnitRule.apply(new Statement() {
                 public void evaluate() throws Throwable {
-                    IMethods mock = Mockito.mock(IMethods.class);
+                    IMethods mock = mock(IMethods.class);
+                    Mockito.when(mock.simpleMethod()); // <--- unfinished stubbing
+                    throw new RuntimeException("Boo!"); // <--- some failure
+                }
+            }, null, injectTestCase).evaluate();
+            fail();
+        } catch (RuntimeException e) {
+            assertEquals("Boo!", e.getMessage());
+        }
+    }
+
+    @Test
+    @Ignore //work in progress
+    public void should_warn_about_stubbing_arg_mismatch_on_failure() throws Throwable {
+        try {
+            jUnitRule.apply(new Statement() {
+                public void evaluate() throws Throwable {
+                    IMethods mock = mock(IMethods.class);
                     declareUnusedStub(mock);
                     throw new AssertionError("x");
                 }
@@ -60,7 +88,7 @@ public void evaluate() throws Throwable {
         } catch (AssertionError e) {
             assertEquals("x", e.getMessage());
             assertEquals(filterLineNo(logger.getLoggedInfo()),
-                "[Mockito] Additional stubbing information (see javadoc for StubbingInfo class):\n" +
+                "<TODO>" +
                 "[Mockito]\n" +
                 "[Mockito] Unused stubbing (perhaps can be removed from the test?):\n" +
                 "[Mockito]\n" +
@@ -70,22 +98,16 @@ public void evaluate() throws Throwable {
     }
 
     @Test
-    public void can_remove_line_numbers() throws Throwable {
-        assertEquals(
-                "[Mockito] 1. -> at org.mockito.internal.junit.JUnitRuleTest.declareUnusedStub(JUnitRuleTest.java:0)",
-                filterLineNo("[Mockito] 1. -> at org.mockito.internal.junit.JUnitRuleTest.declareUnusedStub(JUnitRuleTest.java:82)"));
-    }
-
-    @Test
-    public void shouldNotWarnAboutUnusedStubsWhenPassed() throws Throwable {
+    @Ignore //work in progress
+    public void warns_about_unused_stubs_when_passed() throws Throwable {
         jUnitRule.apply(new Statement() {
             public void evaluate() throws Throwable {
-                IMethods mock = Mockito.mock(IMethods.class);
+                IMethods mock = mock(IMethods.class);
                 declareUnusedStub(mock);
             }
         },null, injectTestCase).evaluate();
 
-        assertEquals("", logger.getLoggedInfo());
+        assertEquals("<TODO>", logger.getLoggedInfo());
     }
 
     private static void declareUnusedStub(IMethods mock) {