From c0a67d75e6dac3ebfe86b5662819ecb0b5769eff Mon Sep 17 00:00:00 2001 From: Szczepan Faber Date: Fri, 19 Aug 2016 14:36:17 -0700 Subject: [PATCH] Made the JUnit runner thread safe - unused stubbing detection logic was not thread safe in Mockito JUnit runner - stopped using the StubbingListener - by design it couldn't support thread safe scenarios unfortunately :( Fixes #384 --- .../junit/UnnecessaryStubbingsReporter.java | 38 ++++++ .../internal/junit/UnusedStubbings.java | 4 +- .../internal/junit/UnusedStubbingsFinder.java | 38 +++++- .../internal/runners/StrictRunner.java | 5 +- .../runners/UnnecessaryStubbingsReporter.java | 69 ---------- .../junit/UnusedStubbingsFinderTest.java | 79 ----------- .../junit/UnusedStubbingsFinderTest.java | 127 ++++++++++++++++++ .../UnusedStubsExceptionMessageTest.java | 3 - 8 files changed, 203 insertions(+), 160 deletions(-) create mode 100644 src/main/java/org/mockito/internal/junit/UnnecessaryStubbingsReporter.java delete mode 100644 src/main/java/org/mockito/internal/runners/UnnecessaryStubbingsReporter.java delete mode 100644 src/test/java/org/mockito/internal/junit/UnusedStubbingsFinderTest.java create mode 100644 src/test/java/org/mockitousage/internal/junit/UnusedStubbingsFinderTest.java diff --git a/src/main/java/org/mockito/internal/junit/UnnecessaryStubbingsReporter.java b/src/main/java/org/mockito/internal/junit/UnnecessaryStubbingsReporter.java new file mode 100644 index 0000000000..a43e35f875 --- /dev/null +++ b/src/main/java/org/mockito/internal/junit/UnnecessaryStubbingsReporter.java @@ -0,0 +1,38 @@ +package org.mockito.internal.junit; + +import org.junit.runner.Description; +import org.junit.runner.notification.Failure; +import org.junit.runner.notification.RunNotifier; +import org.mockito.internal.exceptions.Reporter; +import org.mockito.invocation.Invocation; +import org.mockito.listeners.MockCreationListener; +import org.mockito.mock.MockCreationSettings; + +import java.util.Collection; +import java.util.LinkedList; +import java.util.List; + +/** + * Reports unnecessary stubbings + */ +public class UnnecessaryStubbingsReporter implements MockCreationListener { + + private List mocks = new LinkedList(); + + public void validateUnusedStubs(Class testClass, RunNotifier notifier) { + Collection unused = new UnusedStubbingsFinder().getUnusedStubbingsByLocation(mocks); + if (unused.size() == 0) { + return; //whoa!!! All stubbings were used! + } + + //Oups, there are unused stubbings + Description unnecessaryStubbings = Description.createTestDescription(testClass, "unnecessary Mockito stubbings"); + notifier.fireTestFailure(new Failure(unnecessaryStubbings, + Reporter.formatUnncessaryStubbingException(testClass, unused))); + } + + @Override + public void onMockCreated(Object mock, MockCreationSettings settings) { + mocks.add(mock); + } +} diff --git a/src/main/java/org/mockito/internal/junit/UnusedStubbings.java b/src/main/java/org/mockito/internal/junit/UnusedStubbings.java index 1f415ca776..eae58c30b0 100644 --- a/src/main/java/org/mockito/internal/junit/UnusedStubbings.java +++ b/src/main/java/org/mockito/internal/junit/UnusedStubbings.java @@ -8,7 +8,7 @@ /** * Contains unused stubbings, knows how to format them */ -class UnusedStubbings { +public class UnusedStubbings { private final Collection unused; @@ -32,7 +32,7 @@ void format(String testName, MockitoLogger logger) { logger.log(hint.toString()); } - int size() { + public int size() { return unused.size(); } diff --git a/src/main/java/org/mockito/internal/junit/UnusedStubbingsFinder.java b/src/main/java/org/mockito/internal/junit/UnusedStubbingsFinder.java index 0cc70abeb0..f519f57c7c 100644 --- a/src/main/java/org/mockito/internal/junit/UnusedStubbingsFinder.java +++ b/src/main/java/org/mockito/internal/junit/UnusedStubbingsFinder.java @@ -1,17 +1,17 @@ package org.mockito.internal.junit; +import org.mockito.internal.invocation.Stubbing; import org.mockito.internal.invocation.finder.AllInvocationsFinder; import org.mockito.internal.util.collections.ListUtil; -import org.mockito.internal.invocation.Stubbing; +import org.mockito.invocation.Invocation; -import java.util.Collection; -import java.util.LinkedList; +import java.util.*; /** * Finds unused stubbings */ -class UnusedStubbingsFinder { - UnusedStubbings getUnusedStubbings(Iterable mocks) { +public class UnusedStubbingsFinder { + public UnusedStubbings getUnusedStubbings(Iterable mocks) { Collection stubbings = (Collection) AllInvocationsFinder.findStubbings(mocks); LinkedList unused = ListUtil.filter(stubbings, new ListUtil.Filter() { @@ -22,4 +22,32 @@ public boolean isOut(Stubbing s) { return new UnusedStubbings(unused); } + + public Collection getUnusedStubbingsByLocation(Iterable mocks) { + Collection stubbings = (Collection) AllInvocationsFinder.findStubbings(mocks); + + //1st pass, collect all the locations of the stubbings that were used + //note that those are _not_ locations where the stubbings was used + Set locationsOfUsedStubbings = new HashSet(); + for (Stubbing s : stubbings) { + if (s.wasUsed()) { + String location = s.getInvocation().getLocation().toString(); + locationsOfUsedStubbings.add(location); + } + } + + //2nd pass, collect unused stubbings by location + //If the location matches we assume the stubbing was used in at least one test method + //Also, using map to deduplicate reported unused stubbings + // if unused stubbing appear in the setup method / constructor we don't want to report it per each test case + Map out = new LinkedHashMap(); + for (Stubbing s : stubbings) { + String location = s.getInvocation().getLocation().toString(); + if (!locationsOfUsedStubbings.contains(location)) { + out.put(location, s.getInvocation()); + } + } + + return out.values(); + } } diff --git a/src/main/java/org/mockito/internal/runners/StrictRunner.java b/src/main/java/org/mockito/internal/runners/StrictRunner.java index e7ac448909..3bf5bd3fc2 100644 --- a/src/main/java/org/mockito/internal/runners/StrictRunner.java +++ b/src/main/java/org/mockito/internal/runners/StrictRunner.java @@ -5,6 +5,7 @@ import org.junit.runner.manipulation.NoTestsRemainException; import org.junit.runner.notification.RunNotifier; import org.mockito.Mockito; +import org.mockito.internal.junit.UnnecessaryStubbingsReporter; import org.mockito.internal.runners.util.FailureDetecter; public class StrictRunner implements RunnerImpl { @@ -27,13 +28,13 @@ public void run(RunNotifier notifier) { UnnecessaryStubbingsReporter reporter = new UnnecessaryStubbingsReporter(); FailureDetecter listener = new FailureDetecter(); - Mockito.framework().setStubbingListener(reporter); + Mockito.framework().addListener(reporter); try { // add listener that detects test failures notifier.addListener(listener); runner.run(notifier); } finally { - Mockito.framework().setStubbingListener(null); + Mockito.framework().removeListener(reporter); } if (!filterRequested && listener.isSussessful()) { diff --git a/src/main/java/org/mockito/internal/runners/UnnecessaryStubbingsReporter.java b/src/main/java/org/mockito/internal/runners/UnnecessaryStubbingsReporter.java deleted file mode 100644 index f5eca5a1e6..0000000000 --- a/src/main/java/org/mockito/internal/runners/UnnecessaryStubbingsReporter.java +++ /dev/null @@ -1,69 +0,0 @@ -package org.mockito.internal.runners; - -import org.junit.runner.Description; -import org.junit.runner.notification.Failure; -import org.junit.runner.notification.RunNotifier; -import org.mockito.internal.exceptions.Reporter; -import org.mockito.invocation.Invocation; -import org.mockito.listeners.StubbingListener; - -import java.util.*; - -/** - * Reports unnecessary stubbings - */ -class UnnecessaryStubbingsReporter implements StubbingListener { - - private final Map stubbings = new LinkedHashMap(); - private final Set used = new LinkedHashSet(); - - public void newStubbing(Invocation stubbing) { - //We compare stubbings by the location of stubbing - // so that a stubbing in @Before is considered used when at least one test method uses it - // but not necessarily all test methods need to trigger 'using' it. - //If we compared stubbings by 'invocation' we would not be able to satisfy this scenario - // because stubbing declared in the same place in code is a different 'invocation' for every test. - //The downside of this approach is that it will not work when there are multiple stubbings / invocations - // per line of code. This should be pretty rare in Java, though. - stubbings.put(stubbing.getLocation().toString(), stubbing); - } - - public void usedStubbing(Invocation stubbing, Invocation actual) { - String location = stubbing.getLocation().toString(); - used.add(location); - - //perf tweak, let's get rid of used stubbing now, less calculation later - stubbings.remove(location); - } - - public void stubbingNotFound(Invocation actual) {} - - void validateUnusedStubs(Class testClass, RunNotifier notifier) { - if (stubbings.isEmpty()) { - //perf tweak, bailing out early to avoid extra computation - return; - } - - //removing all used stubbings accounting for possible constructor / @Before stubbings - // that were used only in specific test methods (e.g. not all test methods) - for (String u : used) { - //it's awkward to manipulate the state of this object here, - // we cannot safely rerun validateUnusedStubs() method. - // However, we get good performance and simpler code. - stubbings.remove(u); - } - - if (stubbings.isEmpty()) { - return; - } - - if (stubbings.isEmpty()) { - return; //whoa!!! All stubbings were used! - } - - //Oups, there are unused stubbings - Description unnecessaryStubbings = Description.createTestDescription(testClass, "unnecessary Mockito stubbings"); - notifier.fireTestFailure(new Failure(unnecessaryStubbings, - Reporter.formatUnncessaryStubbingException(testClass, stubbings.values()))); - } -} diff --git a/src/test/java/org/mockito/internal/junit/UnusedStubbingsFinderTest.java b/src/test/java/org/mockito/internal/junit/UnusedStubbingsFinderTest.java deleted file mode 100644 index dfddf1128b..0000000000 --- a/src/test/java/org/mockito/internal/junit/UnusedStubbingsFinderTest.java +++ /dev/null @@ -1,79 +0,0 @@ -package org.mockito.internal.junit; - -import org.junit.Test; -import org.mockito.Mock; -import org.mockitousage.IMethods; -import org.mockitoutil.TestBase; - -import java.util.List; - -import static java.util.Arrays.asList; -import static org.junit.Assert.*; -import static org.mockito.Mockito.when; - -public class UnusedStubbingsFinderTest extends TestBase { - - UnusedStubbingsFinder finder = new UnusedStubbingsFinder(); - @Mock IMethods mock1; - @Mock IMethods mock2; - - @Test - public void no_interactions() throws Exception { - //when - UnusedStubbings stubbings = finder.getUnusedStubbings((List) asList(mock1, mock2)); - - //then - assertEquals(0, stubbings.size()); - } - - @Test - public void no_stubbings() throws Exception { - mock1.simpleMethod(); - - //when - UnusedStubbings stubbings = finder.getUnusedStubbings((List) asList(mock1, mock2)); - - //then - assertEquals(0, stubbings.size()); - } - - @Test - public void no_unused_stubbings() throws Exception { - when(mock1.simpleMethod()).thenReturn("1"); - mock1.simpleMethod(); - - //when - UnusedStubbings stubbings = finder.getUnusedStubbings((List) asList(mock1, mock2)); - - //then - assertEquals(0, stubbings.size()); - } - - @Test - public void unused_stubbings() throws Exception { - when(mock1.simpleMethod()).thenReturn("1"); - - //when - UnusedStubbings stubbings = finder.getUnusedStubbings((List) asList(mock1, mock2)); - - //then - assertEquals(1, stubbings.size()); - } - - @Test - public void some_unused_stubbings() throws Exception { - when(mock1.simpleMethod(1)).thenReturn("1"); - when(mock2.simpleMethod(2)).thenReturn("2"); - when(mock2.simpleMethod(3)).thenReturn("3"); - - mock2.simpleMethod(2); - - //when - UnusedStubbings stubbings = finder.getUnusedStubbings((List) asList(mock1, mock2)); - - //then - assertEquals(2, stubbings.size()); - assertEquals("[mock1.simpleMethod(1); stubbed with: [Returns: 1], mock2.simpleMethod(3); stubbed with: [Returns: 3]]", - stubbings.toString()); - } -} \ No newline at end of file diff --git a/src/test/java/org/mockitousage/internal/junit/UnusedStubbingsFinderTest.java b/src/test/java/org/mockitousage/internal/junit/UnusedStubbingsFinderTest.java new file mode 100644 index 0000000000..22870b6f40 --- /dev/null +++ b/src/test/java/org/mockitousage/internal/junit/UnusedStubbingsFinderTest.java @@ -0,0 +1,127 @@ +package org.mockitousage.internal.junit; + +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.internal.junit.UnusedStubbings; +import org.mockito.internal.junit.UnusedStubbingsFinder; +import org.mockitousage.IMethods; +import org.mockitoutil.TestBase; + +import java.util.Collection; +import java.util.List; + +import static java.util.Arrays.asList; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.when; + +/** + * This unit test lives in 'org.mockitousage' package for a reason. + * It makes it easy to write tests that depend on the stack trace filtering logic. + * Long term, it would be nice to have more configurability in TestBase + * to make it easy to write such unit tests in 'org.mockito.*' packages. + */ +public class UnusedStubbingsFinderTest extends TestBase { + + UnusedStubbingsFinder finder = new UnusedStubbingsFinder(); + @Mock IMethods mock1; + @Mock IMethods mock2; + + @Test + public void no_interactions() throws Exception { + //expect + assertEquals(0, finder.getUnusedStubbings((List) asList(mock1, mock2)).size()); + assertEquals(0, finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)).size()); + } + + @Test + public void no_stubbings() throws Exception { + //when + mock1.simpleMethod(); + + //then + assertEquals(0, finder.getUnusedStubbings((List) asList(mock1, mock2)).size()); + assertEquals(0, finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)).size()); + } + + @Test + public void no_unused_stubbings() throws Exception { + //when + when(mock1.simpleMethod()).thenReturn("1"); + mock1.simpleMethod(); + + //then + assertEquals(0, finder.getUnusedStubbings((List) asList(mock1, mock2)).size()); + assertEquals(0, finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)).size()); + } + + @Test + public void unused_stubbings() throws Exception { + //when + when(mock1.simpleMethod()).thenReturn("1"); + + //then + assertEquals(1, finder.getUnusedStubbings((List) asList(mock1, mock2)).size()); + assertEquals(1, finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)).size()); + } + + @Test + public void some_unused_stubbings() throws Exception { + when(mock1.simpleMethod(1)).thenReturn("1"); + when(mock2.simpleMethod(2)).thenReturn("2"); + when(mock2.simpleMethod(3)).thenReturn("3"); + + mock2.simpleMethod(2); + + //when + UnusedStubbings stubbings = finder.getUnusedStubbings((List) asList(mock1, mock2)); + + //then + assertEquals(2, stubbings.size()); + assertEquals("[mock1.simpleMethod(1); stubbed with: [Returns: 1], mock2.simpleMethod(3); stubbed with: [Returns: 3]]", + stubbings.toString()); + } + + @Test + public void some_unused_stubbings_by_location() throws Exception { + when(mock1.simpleMethod(1)).thenReturn("1"); + when(mock2.simpleMethod(2)).thenReturn("2"); + when(mock2.simpleMethod(3)).thenReturn("3"); + + mock2.simpleMethod(2); + + //when + Collection stubbings = finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)); + + //then + assertEquals(2, stubbings.size()); + assertEquals("[mock1.simpleMethod(1);, mock2.simpleMethod(3);]", stubbings.toString()); + } + + @Test + public void stubbing_used_by_location() throws Exception { + //when + //Emulating stubbing in the same location by putting stubbing in the same line: + when(mock1.simpleMethod(1)).thenReturn("1"); when(mock2.simpleMethod(1)).thenReturn("1"); + //End of emulation + mock1.simpleMethod(1); + + //then technically unused stubbings exist + assertEquals(1, finder.getUnusedStubbings((List) asList(mock1, mock2)).size()); + //however if we consider stubbings in the same location as the same stubbing, all is used: + assertEquals(0, finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)).size()); + } + + @Test + public void deduplicates_stubbings_by_location() throws Exception { + //when + //Emulating stubbing in the same location by putting stubbing in the same line: + when(mock1.simpleMethod(1)).thenReturn("1"); when(mock2.simpleMethod(1)).thenReturn("1"); + //End of emulation + + //when + Collection stubbings = finder.getUnusedStubbingsByLocation((List) asList(mock1, mock2)); + + //then + assertEquals(1, stubbings.size()); + } +} \ No newline at end of file diff --git a/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java b/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java index e722bfc70e..3760e64467 100644 --- a/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java +++ b/src/test/java/org/mockitousage/junitrunner/UnusedStubsExceptionMessageTest.java @@ -13,9 +13,6 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -/** - * Created by sfaber on 4/22/16. - */ public class UnusedStubsExceptionMessageTest extends TestBase { //Moving the code around this class is tricky and may cause the test to fail