Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strictness configurable per mock / stubbing #1272

Merged
merged 50 commits into from Jul 24, 2018

Conversation

Projects
None yet
5 participants
@mockitoguy
Copy link
Member

mockitoguy commented Dec 11, 2017

Fixes #792

Problem

Today we have strict stubs feature, a key enabler for cleaner tests and improved debuggability. Strict stubs don't work well with "common stubbing" pattern. They also don't like when we call stubbed methods with unexpected arguments - even though sometimes it is legit. For more, see #792

Solution

@Test public void demo() {
  //1. "lenient()" method on MockSettings interface:
  Foo mock = Mockito.mock(Foo.class, withSettings().lenient());

  //2. "lenient()" method on Mockito class:
  Mockito.lenient().when(mock.foo(1)).thenReturn(1);
  Mockito.lenient().doReturn(1).when(mock).foo(1);
}

For more, see #792

Public API changes

  • New methods:
    • Mockito#lenient()
    • MockSettings#lenient()
    • MockCreationSettings#isLenient()
    • Stubbing#getStrictness()
  • New interfaces:
    • BaseStubber - extracted out from existing "Stubber" interface, so that we can reuse API with "LenientStubber"
    • LenientStubber - instance returned by "Mockito.lenient()"
  • Other changes:
    • Made @org.mockito.NotExtensible annotation public so that we can use it in subpackages
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 11, 2017

Codecov Report

Merging #1272 into release/2.x will decrease coverage by 0.07%.
The diff coverage is 91.86%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/2.x    #1272      +/-   ##
=================================================
- Coverage          88.62%   88.54%   -0.08%     
- Complexity          2358     2391      +33     
=================================================
  Files                292      296       +4     
  Lines               5950     6005      +55     
  Branches             719      727       +8     
=================================================
+ Hits                5273     5317      +44     
- Misses               497      507      +10     
- Partials             180      181       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/mockito/quality/Strictness.java 100% <ø> (ø) 1 <0> (ø) ⬇️
src/main/java/org/mockito/junit/MockitoJUnit.java 66.66% <ø> (ø) 2 <0> (ø) ⬇️
.../exceptions/misusing/PotentialStubbingProblem.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...eptions/misusing/UnnecessaryStubbingException.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...mockito/internal/invocation/UnusedStubsFinder.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...mockito/internal/stubbing/OngoingStubbingImpl.java 91.66% <100%> (+1.66%) 6 <1> (+1) ⬆️
...rc/main/java/org/mockito/internal/MockitoCore.java 97.75% <100%> (+0.05%) 35 <2> (+2) ⬆️
...ckito/internal/stubbing/DoAnswerStyleStubbing.java 100% <100%> (ø) 6 <6> (?)
.../mockito/internal/junit/UnusedStubbingsFinder.java 100% <100%> (ø) 7 <0> (ø) ⬇️
.../org/mockito/internal/handler/MockHandlerImpl.java 100% <100%> (ø) 12 <2> (ø) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 940e9ac...0fcc2a3. Read the comment docs.

mockitoguy added a commit that referenced this pull request Jan 4, 2018

Added an extra test case
Stumbled upon it when working on #1272

mockitoguy added a commit that referenced this pull request Jan 4, 2018

Added an extra test case
Stumbled upon it when working on #1272

mockitoguy added a commit that referenced this pull request Jan 5, 2018

Updated documentation
Informed that 'silent' mode is effectively 'lenient' Strictness.

Stumbled on it when working on #1272

@mockitoguy mockitoguy force-pushed the fine-strictness branch from 411865d to 6d54766 Jan 8, 2018

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

I like the idea to have an opt-out of the strict stubbing, especially if you just have 1 method. However, the current implementation is very brittle and it seems to introduce quite some duplicate code as well.

For example, the Strictness.LENIENT is scattered throughout the code. This make it very hard to expand later on. Rather, I would add logic to strictness with methods that based on the type determine what they should do. This makes the code a lot more maintainable.

Moreover, I see null used a lot of times, which is unclear to me what the usefulness is.

TLDR: Yay for the feature, but the implementation needs quite some work still.

As a cross-point with JUnit5: I would actually say that we still want method level lenient stubbing, thus these two features can co-exist in my opinion.

@mockitoguy

This comment has been minimized.

Copy link
Member Author

mockitoguy commented Jan 28, 2018

As a cross-point with JUnit5: I would actually say that we still want method level lenient stubbing, thus these two features can co-exist in my opinion.

Can you provide a use case (test case) that demonstrates the value? You can also reply to the long email thread we had about it on Mockito mailing list :)

@mockitoguy

This comment has been minimized.

Copy link
Member Author

mockitoguy commented Jan 28, 2018

Moreover, I see null used a lot of times, which is unclear to me what the usefulness is.

Can you be more specific what nulls / are you not clear about? The code changes are light on the javadoc and comments, apologies! Thank you for review!!!

@mockitoguy

This comment has been minimized.

Copy link
Member Author

mockitoguy commented Jan 28, 2018

For example, the Strictness.LENIENT is scattered throughout the code. This make it very hard to expand later on. Rather, I would add logic to strictness with methods that based on the type determine what they should do. This makes the code a lot more maintainable.

Strictness is a public type and we want to be very judicious adding methods to it. I'm very curious about your idea, though! Perhaps we can put those methods on some new internal Strictness object? Let me try to work on this and get back. Great feedback!

@mockitoguy

This comment has been minimized.

Copy link
Member Author

mockitoguy commented Feb 5, 2018

Hey @mockito/developers, any feedback?

@TimvdLippe, I've addressed portion of your feedback. Can you clarify the other portion (see my questions to your questions :).

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

Mostly the usage of StubbedInvocationMatcher with null remains very brittle. I think we should handle all fallbacks of strictness in a type-safe manner. Preferably by defining a default strictness somewhere and update existing code to use this logic instead.

This is from a quick overview, I do not have the time to do a full code review sadly


private MatchableInvocation invocationForStubbing;

public InvocationContainerImpl(MockCreationSettings mockSettings) {
this.registeredInvocations = createRegisteredInvocations(mockSettings);
this.mockStrictness = mockSettings.isLenient() ? Strictness.LENIENT : null;

This comment has been minimized.

@TimvdLippe

TimvdLippe Feb 5, 2018

Contributor

We assign null here and use it throughout the code. I still think this is brittle, as it is really easy to miss. Moreover, in addConsecutiveAnswer null is passed in and in addAnswer again null is propagated. Rather, we should define a default somewhere and use this instead. That way, this field is never null and we always know what "not specifying a strictness for this method" actually means (e.g. fallback to mockStrictness)

This comment has been minimized.

@mockitoguy

mockitoguy Feb 5, 2018

Author Member

Thank you for feedback! I absolutely agree with you on the nulls.

When implementing this feature, I tried to have a default Strictness. This would require us to add new public value to the enum, for example: Strictness.UNSPECIFIED. I decided against it because it's adding a new awkward value to the public API.

Do you think we should add Strictness.UNSPECIFIED? What would that mean to the user? (e.g. how would we document it?).

This comment has been minimized.

@TimvdLippe

TimvdLippe Feb 5, 2018

Contributor

I would add a constant static attribute in Strictness like public static final Strictness DEFAULT = Strictness.LENIENT; (or whatever value we decide on).

This comment has been minimized.

@mockitoguy

mockitoguy Feb 5, 2018

Author Member

For mock objects and stubbings, the default Strictness is "unspecified", e.g. it will be inherited from MockitoSession or JUnit rule. The question is whether we want to add Strictness.UNSPECIFIED to the public API?

This comment has been minimized.

@TimvdLippe

TimvdLippe Feb 5, 2018

Contributor

I think we should extract this discussion into an issue, as it seems that this is a more fundamental issue with Strictness. Since we are using it more and more and it is configurable in multiple places, maybe we should extract all this logic into a separate class? In this way, you would make a chain of configurations, where we take the "most local" definition. Anyways, let's create an issue with the requirements so that we can tinker.

This comment has been minimized.

@mockitoguy

mockitoguy Feb 5, 2018

Author Member

There is already an issue: #792 - please write comment there with your idea! What do you mean about extracting it into a separate class?

This comment has been minimized.

@TimvdLippe

TimvdLippe Feb 6, 2018

Contributor

@mockitoguy Filed #1305 of what I meant and to hopefully address this issue.

This comment has been minimized.

@TimvdLippe

TimvdLippe Feb 17, 2018

Contributor

I have been trying to incorporate #1305 in this PR, but sadly Strictness is used in many places in the codebase. I feel like the Strictness feature as a whole has had more impact on our overall architecture than I would have liked. For now I am okay with these changes, but I think that we should revisit the whole Strictness story in a later stadium to see if we can reduce duplication and boilerplate code (such as the LenientStubber)

This comment has been minimized.

@mockitoguy

mockitoguy Feb 18, 2018

Author Member

I agree. Thank you for giving it a shot!

I'll fix the conflicts so that we're ready to merge.

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

I am okay with this change. Please address the left-over TODO's and add a new section in the Mockito.java javadoc. Then you can merge this PR 👍


Reporter.unncessaryStubbingException(invocations);
List<Invocation> invocations = new LinkedList<Invocation>();
for (Stubbing stubbing : unused) {

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 2, 2018

Contributor

Java 8 will make this so much nicer, but this is already an improvement 👍

import org.mockito.stubbing.OngoingStubbing;
import org.mockito.stubbing.Stubber;

public class DefaultLenientStubber implements LenientStubber {

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 2, 2018

Contributor

Unfortunate that we require all this boilerplate here. Would be great to have a future improvement to get rid of that. Not blocking though.

@@ -109,4 +109,7 @@
*/
@Incubating
Object getOuterClassInstance();

@Incubating//TODO x javadoc

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 2, 2018

Contributor

Please fix this TODO

@@ -50,4 +52,7 @@
* @since 2.2.3
*/
boolean wasUsed();

@Incubating//TODO x javadoc

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 2, 2018

Contributor

Please fix this TODO

@@ -46,7 +46,7 @@ public void setup() {
public void should_finish_stubbing_when_wrong_throwable_is_set() throws Exception {
state.stubbingStarted();
try {
invocationContainerImpl.addAnswer(new ThrowsException(new Exception()));
invocationContainerImpl.addAnswer(new ThrowsException(new Exception()), null);

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 2, 2018

Contributor

Would be great to overload addAnswer, to not having to modify all tests.

import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.withSettings;

//TODO 792 also move other Strictness tests to this package (unless they already have good package)

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 2, 2018

Contributor

I am not really sure what you mean with this TODO. Is this something we need to address before or after merging this PR?

@mockitoguy

This comment has been minimized.

Copy link
Member Author

mockitoguy commented Mar 5, 2018

Please address the left-over TODO's and add a new section in the Mockito.java javadoc. Then you can merge this PR 👍

Absolutely! Thank you for review!

@mpkempson

This comment has been minimized.

Copy link

mpkempson commented Jun 26, 2018

Hey guys, any ideas when this will be ready to merge in and released? Im just poking because we're having the exact issue this would resolve at our work.

Thank you in advance!

@mockitoguy

This comment has been minimized.

Copy link
Member Author

mockitoguy commented Jun 26, 2018

@mpkempson, thank you for upvoting - that's what I needed! I'll work on this shortly.

@mockitoguy mockitoguy self-assigned this Jul 4, 2018

@mockitoguy mockitoguy force-pushed the fine-strictness branch 2 times, most recently from 1c25d1a to dec1194 Jul 21, 2018

mockitoguy added some commits Dec 11, 2017

Progress on strictness per mock/stubbing
Implemented basic support for setting strictness per stubbing. Refactorings and more test coverage is pending. Some features not supported yet.

@mockitoguy mockitoguy force-pushed the fine-strictness branch from 79d3346 to 0fcc2a3 Jul 24, 2018

@mockitoguy mockitoguy merged commit d2145dd into release/2.x Jul 24, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
this.answers.addAll(answers);
}

boolean isSet() {

This comment has been minimized.

@marchpig

marchpig Jul 24, 2018

Contributor

Is there any special meaning of this method name?
I think isEmpty() is clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.