JUnit runner reports argument stubbing mismatches #808

mockitoguy · 2016-12-11T00:38:57Z

Mockito v2 introduced a new feature for reporting unused stubs and reporting argument mismatches (when stubbing uses different arguments than invocation in code under test). We received feedback from our users that the behavior should be more consistent for JUnit rules and JUnit runner. This change introduces that consistency, making JUnit runner also report stubbing argument mismatches to the console. This way, the users should be able to debug tests faster and gain more productivity when writing and maintaining tests!

Also checkout new feature added in Mockito 2.3.x - opt-in strictness for Mockito JUnit rules, making the tests even cleaner and easier to debug!

codecov-io · 2016-12-11T00:46:53Z

Current coverage is 86.58% (diff: 94.11%)

Merging #808 into release/2.x will increase coverage by 0.01%

@@           release/2.x       #808   diff @@
=============================================
  Files              284        286     +2   
  Lines             5628       5657    +29   
  Methods              0          0          
  Messages             0          0          
  Branches           913        916     +3   
=============================================
+ Hits              4872       4898    +26   
- Misses             563        565     +2   
- Partials           193        194     +1

Powered by Codecov. Last update a75b5a1...ea1ec71

mockitoguy · 2016-12-12T22:24:20Z

Friendly ping

TimvdLippe

Did a quick overview, PR seems fine. Let's wait for the others to review before merging

mockitoguy · 2016-12-19T17:40:26Z

Friendly ping no. 2 ;) If no other feedback I'll merge the PR.

bric3 · 2016-12-20T13:47:18Z

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

@@ -2,7 +2,7 @@

 import org.mockito.mock.MockCreationSettings;

-class NoOpTestListener implements MockitoTestListener {
+public class NoOpTestListener implements MockitoTestListener {


Is it necessary to have this class public ?

yes. We still have 2 internal packages: internal.runners and internal.junit. We should merge them but perhaps after we stop maintaining 2.x?

That makes sense.

bric3 · 2016-12-20T13:53:21Z

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

 */
-public interface RunnerImpl extends Filterable{
+public interface RunnerImpl extends Filterable {


Since it is internal, I don't see a blocker to rename it now

It will make the diffs in GitHub unusable. Are you sure? :)

Nope git (and so is github) can track renames too

bric3 · 2016-12-20T13:54:37Z

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


+//TODO rename to DefaultInternalRunner and the parent to InternalRunner


I prefer the current name SilentJUnitRunner, it describes better the intention (even if it becomes the default runner)

Given that it has more features that just being "silent" (initializing mocks, validating framework) would still prefer the Silent in the name?

bric3 · 2016-12-20T15:00:22Z

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

+            protected Statement withBefores(FrameworkMethod method, Object target, Statement statement) {
+                // get new test listener and add add it to the framework
+                mockitoTestListener = listenerSupplier.get();
+                Mockito.framework().addListener(mockitoTestListener);


comment paraphrase ;)

:) Removing...

Actually, I kinda like it in the code :). Leaving it for now.

bric3 · 2016-12-20T15:18:05Z

src/main/java/org/mockito/internal/runners/util/RunnerProvider.java

        Constructor<?> constructor;
        try {
            Class<?> runnerClass = Class.forName(runnerClassName);
-            constructor = runnerClass.getConstructor(Class.class.getClass());
+            constructor = runnerClass.getConstructors()[0];


It's not safe to rely on order of constructor, I don't believe the JVM garantees it. If there's a single constructor that's ok, but any implementor can add another (e.g. for tests), etc..

Good point! For simplicity I'll just add a check that we expect exactly one constructor.

bric3 · 2016-12-20T15:20:44Z

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

        try {
-            return new RunnerProvider().newInstance("org.mockito.internal.runners.SilentJUnitRunner", klass);
+            return new RunnerProvider().newInstance("org.mockito.internal.runners.SilentJUnitRunner", klass, listenerSupplier);


Why not SilentJUnitRunner.class.getName() ?

To avoid static linking that leads to uncaught class errors. Good question!

Yes I supposed this was that, but since the class is in the same package it feels not needed. And may complicate a tad more renaming.

bric3 · 2016-12-20T16:01:00Z

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

+                return new MismatchReportingTestListener(new ConsoleMockitoLogger());
+            }
+        });
+    }


Why not using the same construction with strictness enum and a switch statement ?

Good point. It feels that the current implementation is simpler then when I add conditional logic.

- Makes the behavior of JUnit runner more consistent with JUnit rules. Lack of this consistency was reported by the users. - In Mockito 3.0 we will make the runner have exactly the same behavior as the rule and the implementation will be reused via MockitoTestListener implementations. Refactored JUnit runner implementation will make it easy. - The change should be safe. The most critical change is in SilentJUnitRunner - please review this class most carefully.

After some nice feedback from Brice!

Subjective decision. JUnit runner reporting stubbing mismatches is a nice feature!

mockitoguy · 2016-12-20T23:55:52Z

Merging today. Thanks guys for really nice feedback!!!

mockitoguy added the enhancement label Dec 11, 2016

TimvdLippe added the in progress label Dec 11, 2016

TimvdLippe approved these changes Dec 12, 2016

View reviewed changes

bric3 reviewed Dec 20, 2016

View reviewed changes

mockitoguy added 3 commits December 20, 2016 15:45

Added sanity check

d01b7a4

After some nice feedback from Brice!

Bumped minor version

ea1ec71

Subjective decision. JUnit runner reporting stubbing mismatches is a nice feature!

mockitoguy force-pushed the strict-runner branch from f45fafb to ea1ec71 Compare December 20, 2016 23:47

mockitoguy merged commit 064a3f2 into release/2.x Dec 21, 2016

TimvdLippe removed the in progress label Dec 21, 2016

mockitoguy deleted the strict-runner branch December 21, 2016 05:32


		//TODO rename to DefaultInternalRunner and the parent to InternalRunner

Uh oh!

JUnit runner reports argument stubbing mismatches #808

JUnit runner reports argument stubbing mismatches #808

Uh oh!

Conversation

mockitoguy commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 86.58% (diff: 94.11%)

Uh oh!

mockitoguy commented Dec 12, 2016

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

mockitoguy commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockitoguy commented Dec 20, 2016

Uh oh!

Uh oh!

mockitoguy commented Dec 11, 2016 •

edited

Loading

codecov-io commented Dec 11, 2016 •

edited

Loading