Repository: mockito/mockito
Issue: Unused stubbings are not reported when filters are used

Unused stubbings are not reported when a filter is used during the test run. This is documented clearly in the [StrictRunner class](https://github.com/mockito/mockito/blob/f5ad9e9f9a338bcea6180fc474a562acc13f2c84/src/main/java/org/mockito/internal/runners/StrictRunner.java#L47-L52):

```java
// only report when:
// 1. if all tests from given test have ran (filter requested is false)
//   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)
```

Only reporting unused stubbings when all tests are run is reasonable. However, the logic for detecting this case is a little too coarse-grained, since even no-op filters end up disabling the reporting of unused stubbings. This can be troublesome when, for example, JUnit categories are used to distinguish between unit and integration tests.

It'd be nice if we could detect when filters actually prevent one or more tests from being run, and only skip reporting unused stubbings in those cases.


Created based off of https://github.com/gradle/gradle/issues/10694, specifically [this comment](https://github.com/gradle/gradle/issues/10694#issuecomment-1374911274).

This affects a project currently using Mockito version 4.11.0, but I've just verified that it also affects version 5.2.0.

Issue comments:
I've held off on providing a reproducible example as I hope the description is clear enough; can give it a shot if more detail is required, though.
Added a unit test case to #3078 that gives a decent picture of what this describes, but without some of the outer context (e.g., Gradle and JUnit exclusion categories) that helps demonstrate the impact of the current behavior.

Pull request review comments:
Why does this need to be `volatile`? We try to stay away of that as much as we can, as multi-threading is already a difficult story with Mockito. As I see it, there is no need for this to be shared across instances, so I think this is not necessary?
Ah thanks, good catch! I wasn't familiar with the threading model at play here and wanted to be conservative. I've removed the `volatile` modifier since it appears to be unnecessary.

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/runners/StrictRunner.java b/src/main/java/org/mockito/internal/runners/StrictRunner.java
index b3ea7749b0..b57b7c0c49 100644
--- a/src/main/java/org/mockito/internal/runners/StrictRunner.java
+++ b/src/main/java/org/mockito/internal/runners/StrictRunner.java
@@ -16,7 +16,7 @@ public class StrictRunner implements InternalRunner {
 
     private final Class<?> testClass;
     private final InternalRunner runner;
-    private boolean filterRequested;
+    private boolean testsSkipped;
 
     /**
      * @param runner - the runner to wrap around
@@ -25,6 +25,7 @@ public class StrictRunner implements InternalRunner {
     public StrictRunner(InternalRunner runner, Class<?> testClass) {
         this.runner = runner;
         this.testClass = testClass;
+        this.testsSkipped = false;
     }
 
     @Override
@@ -43,9 +44,9 @@ public void run(RunNotifier notifier) {
             Mockito.framework().removeListener(reporter);
         }
 
-        if (!filterRequested && listener.isSuccessful()) {
+        if (!testsSkipped && listener.isSuccessful()) {
             // only report when:
-            // 1. if all tests from given test have ran (filter requested is false)
+            // 1. if all tests from given test have ran (tests skipped is false)
             //   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
@@ -61,7 +62,40 @@ public Description getDescription() {
 
     @Override
     public void filter(Filter filter) throws NoTestsRemainException {
-        filterRequested = true;
-        runner.filter(filter);
+        Filter recordingFilter = new RecordingFilter(filter);
+        runner.filter(recordingFilter);
+    }
+
+    private class RecordingFilter extends Filter {
+
+        private final Filter delegate;
+
+        public RecordingFilter(Filter delegate) {
+            this.delegate = delegate;
+        }
+
+        @Override
+        public void apply(Object child) throws NoTestsRemainException {
+            delegate.apply(child);
+        }
+
+        @Override
+        public Filter intersect(Filter second) {
+            return delegate.intersect(second);
+        }
+
+        @Override
+        public boolean shouldRun(Description description) {
+            boolean result = delegate.shouldRun(description);
+            if (!result) {
+                testsSkipped = true;
+            }
+            return result;
+        }
+
+        @Override
+        public String describe() {
+            return delegate.describe();
+        }
     }
 }


Test patch:
diff --git a/src/test/java/org/mockitousage/junitrunner/StrictRunnerTest.java b/src/test/java/org/mockitousage/junitrunner/StrictRunnerTest.java
index 47ed6886e6..87700bd43a 100644
--- a/src/test/java/org/mockitousage/junitrunner/StrictRunnerTest.java
+++ b/src/test/java/org/mockitousage/junitrunner/StrictRunnerTest.java
@@ -11,9 +11,12 @@
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.runner.Description;
 import org.junit.runner.JUnitCore;
+import org.junit.runner.Request;
 import org.junit.runner.Result;
 import org.junit.runner.RunWith;
+import org.junit.runner.manipulation.Filter;
 import org.mockito.Mock;
 import org.mockito.exceptions.misusing.UnnecessaryStubbingException;
 import org.mockito.junit.MockitoJUnit;
@@ -84,6 +87,34 @@ public void runner_in_multi_threaded_tests() {
         JUnitResultAssert.assertThat(result).isSuccessful();
     }
 
+    @Test
+    public void does_not_report_unused_stubs_when_test_is_filtered() {
+        // This class has two test methods; run only the test method that does not use the stubbing
+        // set up in before
+        Request request = Request.method(StubbingInBeforeUsed.class, "dummy");
+
+        // when
+        Result result = runner.run(request);
+
+        // then
+        JUnitResultAssert.assertThat(result).isSuccessful();
+    }
+
+    @Test
+    public void fails_when_stubs_were_not_used_with_noop_filter() {
+        Class[] tests = {
+            StubbingInConstructorUnused.class,
+            StubbingInBeforeUnused.class,
+            StubbingInTestUnused.class
+        };
+
+        // when
+        Result result = runner.run(Request.classes(tests).filterWith(new NoOpFilter()));
+
+        // then
+        JUnitResultAssert.assertThat(result).fails(3, UnnecessaryStubbingException.class);
+    }
+
     @RunWith(MockitoJUnitRunner.class)
     public static class StubbingInConstructorUsed extends StubbingInConstructorUnused {
         @Test
@@ -201,4 +232,17 @@ public void run() {
             t.join();
         }
     }
+
+    private static class NoOpFilter extends Filter {
+
+        @Override
+        public boolean shouldRun(Description description) {
+            return true;
+        }
+
+        @Override
+        public String describe() {
+            return "No-op filter";
+        }
+    }
 }