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

New strict stubbing API - MockitoSession #865

Merged
merged 27 commits into from Jan 29, 2017

Conversation

Projects
None yet
4 participants
@mockitoguy
Member

mockitoguy commented Jan 3, 2017

See proposed design at #857

The code is reviewable, especially new public API. Please give feedback! More work is pending.

Current status:

  • address @bric3 feedback
    • getter
    • javadoc
  • review documentation for consistency
    • Mockito.session()
    • MockitoSessionBuilder
    • Strictness
    • MockitoHint
    • PotentialStubbingProblem
    • UnnecessaryStubbingException
    • MockitoJUnitRunner and subclasses
    • MockitoSession
    • MockitoRule
  • Add / update mentions in the main Mockito class
  • add validate mockito usage (should be very easy)
  • Try making rules/runner use the session. They already reuse the code, they just don't use the API directly. Created #898 to track this work.
  • Address @TimvdLippe feedback
  • Create concurrent test (multiple sessions active in different threads)
  • (non-code change, not blocking merge) GitHub tickets linked from source code should be better documented #769, #384. Alternatively, create ticket for tracking.
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jan 3, 2017

Current coverage is 86.59% (diff: 90.00%)

No coverage report found for release/2.x at 23c75bd.

Powered by Codecov. Last update 23c75bd...d61f877

codecov-io commented Jan 3, 2017

Current coverage is 86.59% (diff: 90.00%)

No coverage report found for release/2.x at 23c75bd.

Powered by Codecov. Last update 23c75bd...d61f877

@bric3

This comment has been minimized.

Show comment
Hide comment
@bric3

bric3 Jan 4, 2017

Contributor

Bit busy in this couple of weeks I'l give my review asap, probably next week.

Contributor

bric3 commented Jan 4, 2017

Bit busy in this couple of weeks I'l give my review asap, probably next week.

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jan 4, 2017

Contributor

I think this API makes more sense if it is actually used in the JUnit rule and runner. E.g. this API exposes the methods and implementation and the rule/runner call this API accordingly

Contributor

TimvdLippe commented Jan 4, 2017

I think this API makes more sense if it is actually used in the JUnit rule and runner. E.g. this API exposes the methods and implementation and the rule/runner call this API accordingly

@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 7, 2017

Member

Thanks guys! Waiting for feedback.

@TimvdLippe, I completely agree this API should be used by the Rule / Runner. What are your thoughts about exposing this API to the users?

Member

mockitoguy commented Jan 7, 2017

Thanks guys! Waiting for feedback.

@TimvdLippe, I completely agree this API should be used by the Rule / Runner. What are your thoughts about exposing this API to the users?

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jan 7, 2017

Contributor

@szczepiq Not sure what the benefit is for users. Therefore I would like this PR to implement it as internal API used in the rule and runner. Later we can see if we want to expose it or not.

Contributor

TimvdLippe commented Jan 7, 2017

@szczepiq Not sure what the benefit is for users. Therefore I would like this PR to implement it as internal API used in the rule and runner. Later we can see if we want to expose it or not.

@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 7, 2017

Member

@TimvdLippe thanks for feedback! You are not sure about benefits of a) strict stubbing or b) strict stubbing availibility/need outside of runner/rule?

Member

mockitoguy commented Jan 7, 2017

@TimvdLippe thanks for feedback! You are not sure about benefits of a) strict stubbing or b) strict stubbing availibility/need outside of runner/rule?

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jan 7, 2017

Contributor

@szczepiq b. Strict stubbing is very valuable, but I am not sure what a user could do with the API outside of a runner/rule. I think keeping it internal is just fine for now.

Contributor

TimvdLippe commented Jan 7, 2017

@szczepiq b. Strict stubbing is very valuable, but I am not sure what a user could do with the API outside of a runner/rule. I think keeping it internal is just fine for now.

@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 7, 2017

Member

Ok. What about users that cannot use our JUnit Rule/runner?

Member

mockitoguy commented Jan 7, 2017

Ok. What about users that cannot use our JUnit Rule/runner?

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jan 7, 2017

Contributor

At this moment I am not aware of users requesting this. Still, we can initially keep it internal and expose it later if requested?

Contributor

TimvdLippe commented Jan 7, 2017

At this moment I am not aware of users requesting this. Still, we can initially keep it internal and expose it later if requested?

@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 8, 2017

Member

I'm with you on being very cautious about introducing features without user feedback or request. I think that this exactly the approach the core team should have to build a great library.

Thing is that in this situation there are users needing it. For example, I'm frustrated that myself and other engineers at LinkedIn cannot use strict stubbing (most of the tests use TestNG, there are also usages of custom runners).

Another use case are existing integrations like Jukito, Springockito, etc. Users of those frameworks won't be able to take advantage of strict stubbing.

In general, if the team chooses to implement a feature such us strict stubbing, then we should make the usage of the feature easy and flexible. We should not lock in users with JUnit. For example, Mockito has "@mock" annotation and offers flexible way of using that annotation: with JUnit Rule, Runner and without them (initMocks() method).

Also keep in mind that the new API is "@Incubating" so we could potentially take it away if we don't find it valuable.

Thoughts?

Member

mockitoguy commented Jan 8, 2017

I'm with you on being very cautious about introducing features without user feedback or request. I think that this exactly the approach the core team should have to build a great library.

Thing is that in this situation there are users needing it. For example, I'm frustrated that myself and other engineers at LinkedIn cannot use strict stubbing (most of the tests use TestNG, there are also usages of custom runners).

Another use case are existing integrations like Jukito, Springockito, etc. Users of those frameworks won't be able to take advantage of strict stubbing.

In general, if the team chooses to implement a feature such us strict stubbing, then we should make the usage of the feature easy and flexible. We should not lock in users with JUnit. For example, Mockito has "@mock" annotation and offers flexible way of using that annotation: with JUnit Rule, Runner and without them (initMocks() method).

Also keep in mind that the new API is "@Incubating" so we could potentially take it away if we don't find it valuable.

Thoughts?

@TimvdLippe

This comment has been minimized.

Show comment
Hide comment
@TimvdLippe

TimvdLippe Jan 8, 2017

Contributor

myself and other engineers at LinkedIn cannot use strict stubbing

I was not aware of this usecase, as no issue was created. Given that this is requested by user, making it public is okay with me!

Contributor

TimvdLippe commented Jan 8, 2017

myself and other engineers at LinkedIn cannot use strict stubbing

I was not aware of this usecase, as no issue was created. Given that this is requested by user, making it public is okay with me!

@bric3

Requesting change, as I definately would prefer a builder style API. But I'm quite ok already.

Show outdated Hide outdated src/main/java/org/mockito/Mockito.java
@Incubating
public static MockitoMocking startMocking(Object testClassInstance, Strictness strictness) {
return new DefaultMockitoMocking(testClassInstance, strictness, new ConsoleMockitoLogger());
}

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor
  • While public API is ok for me, I wonder if this API should be exposed in the Mockito class at first. Why not a class like MockitoMocking.startMocking()..., and rename the current MockitoMocking interface to OngoingMockitoMocking.
  • This method maybe more interesting if started some kind of builder instead of taking arguments. One could want to modify the logger. And future options could be passed much more easily without breaking binary compatibility.
@bric3

bric3 Jan 11, 2017

Contributor
  • While public API is ok for me, I wonder if this API should be exposed in the Mockito class at first. Why not a class like MockitoMocking.startMocking()..., and rename the current MockitoMocking interface to OngoingMockitoMocking.
  • This method maybe more interesting if started some kind of builder instead of taking arguments. One could want to modify the logger. And future options could be passed much more easily without breaking binary compatibility.

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor

Could become something like :

MockitoMocking.mocking().testInstance(this)
                        .logger(devnull)
                        .strictness(...)
                        // possible configuration options to add at later time
                        .initAnnotations(...)
                        .overridePlugin(...)
                        .validate(...)
                        // ...
                        .start(); // returns OngoingMockitoMocking
@bric3

bric3 Jan 11, 2017

Contributor

Could become something like :

MockitoMocking.mocking().testInstance(this)
                        .logger(devnull)
                        .strictness(...)
                        // possible configuration options to add at later time
                        .initAnnotations(...)
                        .overridePlugin(...)
                        .validate(...)
                        // ...
                        .start(); // returns OngoingMockitoMocking

This comment has been minimized.

@mockitoguy

mockitoguy Jan 14, 2017

Member

I like a lot the idea of a builder. "MockitoMocking" name is somewhat meaningless. How about we call it "MockitoSession" or "MockitoRun" to indicate a lifecycle of some kind with clear starting and ending point:

MockitoSession mockito = Mockito.mockitoSession().initMocks(this).strictness(Strictness.STRICT_STUBS).startMocking();
mockito.finishMocking();

I'm leaning towards new method on Mockito class instead for new static class endpoint. This way, the API is easier to discover / easier to use.

@mockitoguy

mockitoguy Jan 14, 2017

Member

I like a lot the idea of a builder. "MockitoMocking" name is somewhat meaningless. How about we call it "MockitoSession" or "MockitoRun" to indicate a lifecycle of some kind with clear starting and ending point:

MockitoSession mockito = Mockito.mockitoSession().initMocks(this).strictness(Strictness.STRICT_STUBS).startMocking();
mockito.finishMocking();

I'm leaning towards new method on Mockito class instead for new static class endpoint. This way, the API is easier to discover / easier to use.

This comment has been minimized.

@bric3

bric3 Jan 15, 2017

Contributor

OK for either MockitoRun or MockitoSession.

@bric3

bric3 Jan 15, 2017

Contributor

OK for either MockitoRun or MockitoSession.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 15, 2017

Member

Mockito.mockitoSession() VS MockitoSession.mockitoSession() ?

@mockitoguy

mockitoguy Jan 15, 2017

Member

Mockito.mockitoSession() VS MockitoSession.mockitoSession() ?

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 15, 2017

Contributor

Let's do Mockito.mockitoSession() for now, but we should take another look at the method creep in this class. It is getting out of hand slowly but steadily :P

@TimvdLippe

TimvdLippe Jan 15, 2017

Contributor

Let's do Mockito.mockitoSession() for now, but we should take another look at the method creep in this class. It is getting out of hand slowly but steadily :P

This comment has been minimized.

@mockitoguy

mockitoguy Jan 16, 2017

Member

Absolutely we need to be cautious about adding more public method to Mockito class because it is so big. On the other hand, our users expect to find new API on Mockito class, it is a main entry to our entire API. MockitoAnnotations is an odd exception that we can live with because everybody is so used to it ;)

An alternative approach is to new create classes like MockitoSession, and place new public static methods there. However, this would fragment the API and would make it harder to use. Having lots of methods on Mockito is not convenient. However, having several static classes with API entry points is even more inconvenient. It makes the API less discoverable.

Given decent use case and no other sensible host, I'm ok with new method on Mockito class.

Given @TimvdLippe vote on this direction, I'm going ahead with Mockito.mockitoSession() in this PR.

Thanks everybody for great feedback!

@mockitoguy

mockitoguy Jan 16, 2017

Member

Absolutely we need to be cautious about adding more public method to Mockito class because it is so big. On the other hand, our users expect to find new API on Mockito class, it is a main entry to our entire API. MockitoAnnotations is an odd exception that we can live with because everybody is so used to it ;)

An alternative approach is to new create classes like MockitoSession, and place new public static methods there. However, this would fragment the API and would make it harder to use. Having lots of methods on Mockito is not convenient. However, having several static classes with API entry points is even more inconvenient. It makes the API less discoverable.

Given decent use case and no other sensible host, I'm ok with new method on Mockito class.

Given @TimvdLippe vote on this direction, I'm going ahead with Mockito.mockitoSession() in this PR.

Thanks everybody for great feedback!

Show outdated Hide outdated src/main/java/org/mockito/internal/framework/DefaultMockitoMocking.java
//TODO
// 1. validate mockito usage
// 2. can we have JUnit rule and runner use MockitoMocking API directly?

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor
// 3. also JUnit 5 / Jupiter extensions
// 4. There's also the unreleased TestNG code.
@bric3

bric3 Jan 11, 2017

Contributor
// 3. also JUnit 5 / Jupiter extensions
// 4. There's also the unreleased TestNG code.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 13, 2017

Member

Nice ideas!

@mockitoguy

mockitoguy Jan 13, 2017

Member

Nice ideas!

Show outdated Hide outdated src/main/java/org/mockito/internal/junit/DefaultStubbingLookupListener.java
@@ -20,6 +20,7 @@
class DefaultStubbingLookupListener implements StubbingLookupListener {
Strictness currentStrictness;
boolean mismatchesReported;

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor

Since it not final but muted once, maybe introduce a getter ?

@bric3

bric3 Jan 11, 2017

Contributor

Since it not final but muted once, maybe introduce a getter ?

This comment has been minimized.

@mockitoguy

mockitoguy Jan 13, 2017

Member

I'd rather keep it simple, without a getter. Is it really important for you?

@mockitoguy

mockitoguy Jan 13, 2017

Member

I'd rather keep it simple, without a getter. Is it really important for you?

This comment has been minimized.

@bric3

bric3 Jan 13, 2017

Contributor

Actually having the field public (package visible) made me wonder if anyone could change the state, if the field is private and there's a getter I know instantly this field is supposed to be read elsewhere.

@bric3

bric3 Jan 13, 2017

Contributor

Actually having the field public (package visible) made me wonder if anyone could change the state, if the field is private and there's a getter I know instantly this field is supposed to be read elsewhere.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 16, 2017

Member

Actually having the field public (package visible) made me wonder if anyone could change the state

This is the intent of this code. This state is mutable. The alternative is to provide getter and setter, which is more code for a simple collaboration between classes in the same package. If you feel that getter and setter yield code that is easier to understand, I'm happy to change it. LMK!

@mockitoguy

mockitoguy Jan 16, 2017

Member

Actually having the field public (package visible) made me wonder if anyone could change the state

This is the intent of this code. This state is mutable. The alternative is to provide getter and setter, which is more code for a simple collaboration between classes in the same package. If you feel that getter and setter yield code that is easier to understand, I'm happy to change it. LMK!

This comment has been minimized.

@bric3

bric3 Jan 17, 2017

Contributor

I'm an anti setter getter person. But on the other hand I don't like leaking mutable variables. It may be nit picky I agree. But it feels future proof safe.

@bric3

bric3 Jan 17, 2017

Contributor

I'm an anti setter getter person. But on the other hand I don't like leaking mutable variables. It may be nit picky I agree. But it feels future proof safe.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 20, 2017

Member

Fixed it. Indeed it looks better now because after all I needed only 2 out of 4 setter/getter methods. The public/package local footprint of the class is smaller now (will push soon).

Thank you for useful code review feedback. My code is easy for me to understand (most of the time :), but I want it to be clear for other engs. Keep it coming.

@mockitoguy

mockitoguy Jan 20, 2017

Member

Fixed it. Indeed it looks better now because after all I needed only 2 out of 4 setter/getter methods. The public/package local footprint of the class is smaller now (will push soon).

Thank you for useful code review feedback. My code is easy for me to understand (most of the time :), but I want it to be clear for other engs. Keep it coming.

Show outdated Hide outdated src/main/java/org/mockito/internal/junit/UniversalTestListener.java
if (event.getFailure() == null) {
private void reportUnusedStubs(TestFinishedEvent event, Collection<Object> mocks) {
if (event.getFailure() == null && !stubbingLookupListener.mismatchesReported) {
//If there is some other failure (or mismatches were detected) don't report another exception to avoid confusion

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor

This comment line should be above (Today I understand the code because I did follow on this code and did the review, but in 2 years form now if someone stumble on this code)

@bric3

bric3 Jan 11, 2017

Contributor

This comment line should be above (Today I understand the code because I did follow on this code and did the review, but in 2 years form now if someone stumble on this code)

This comment has been minimized.

@mockitoguy

mockitoguy Jan 13, 2017

Member

Good idea. Thanks!

@mockitoguy

mockitoguy Jan 13, 2017

Member

Good idea. Thanks!

@@ -34,6 +36,7 @@ public void testFinished(TestFinishedEvent event) {
Collection<Object> createdMocks = mocks.keySet();
//At this point, we don't need the mocks any more and we can mark all collected mocks for gc
//TODO make it better, it's easy to forget to clean up mocks and we still create new instance of list that nobody will read, it's also duplicated
//TODO clean up all other state, null out stubbingLookupListener

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor

Should we promote the test util class StateMaster to track mockito state ?

@bric3

bric3 Jan 11, 2017

Contributor

Should we promote the test util class StateMaster to track mockito state ?

This comment has been minimized.

@mockitoguy

mockitoguy Jan 13, 2017

Member

Interesting idea. StateMaster manages thread local state. In this case, it's the state of test listener that we want to keep clean. Hence, StateMaster does not really apply here.

@mockitoguy

mockitoguy Jan 13, 2017

Member

Interesting idea. StateMaster manages thread local state. In this case, it's the state of test listener that we want to keep clean. Hence, StateMaster does not really apply here.

This comment has been minimized.

@bric3

bric3 Jan 13, 2017

Contributor

I meant a tool like StateMaster, or refactor it to work for Mockito in a more general way.

@bric3

bric3 Jan 13, 2017

Contributor

I meant a tool like StateMaster, or refactor it to work for Mockito in a more general way.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 16, 2017

Member

Your suggestion is not super clear to me. Given that this refactoring is not critical for the feature nor code quality, let's move on for now. We can get back to this if needed!

@mockitoguy

mockitoguy Jan 16, 2017

Member

Your suggestion is not super clear to me. Given that this refactoring is not critical for the feature nor code quality, let's move on for now. We can get back to this if needed!

This comment has been minimized.

@bric3

bric3 Jan 17, 2017

Contributor

Yes it's not directly related to this PR. Just an idea to discuss. ;)

@bric3

bric3 Jan 17, 2017

Contributor

Yes it's not directly related to this PR. Just an idea to discuss. ;)

Show outdated Hide outdated src/test/java/org/mockitousage/stubbing/StrictStubbingE2ETest.java
import static org.mockito.BDDMockito.given;
public class StrictStubbingE2ETest {

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor

nit-picky I'd rename E2E to EndToEnd

@bric3

bric3 Jan 11, 2017

Contributor

nit-picky I'd rename E2E to EndToEnd

This comment has been minimized.

@mockitoguy

mockitoguy Jan 13, 2017

Member

ok :)

@mockitoguy
Show outdated Hide outdated src/test/java/org/mockitoutil/JUnitResultAssert.java
*/
public JUnitResultAssert fails(Class expectedException, String exceptionMessage) {
fails(1, expectedException);
Failure f = result.getFailures().iterator().next();

This comment has been minimized.

@bric3

bric3 Jan 11, 2017

Contributor

I remember that at some point some Iterator implementations failed if hasNext was not invoked. I'd be careful on that one. (Better make a utility method in org.mockito.internal.util.collections.Iterables)

@bric3

bric3 Jan 11, 2017

Contributor

I remember that at some point some Iterator implementations failed if hasNext was not invoked. I'd be careful on that one. (Better make a utility method in org.mockito.internal.util.collections.Iterables)

This comment has been minimized.

@mockitoguy

mockitoguy Jan 13, 2017

Member

Interesting. iterator().next() is a fairly common idiom to get first element, for example from a set. It's a good idea to put it in Iterables. There is no harm to call hasNext so I'll do it. Thanks for the suggestion!

@mockitoguy

mockitoguy Jan 13, 2017

Member

Interesting. iterator().next() is a fairly common idiom to get first element, for example from a set. It's a good idea to put it in Iterables. There is no harm to call hasNext so I'll do it. Thanks for the suggestion!

This comment has been minimized.

@bric3

bric3 Jan 13, 2017

Contributor

Also this will be easier to replace by Stream with Mockito 3

@bric3

bric3 Jan 13, 2017

Contributor

Also this will be easier to replace by Stream with Mockito 3

@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 16, 2017

Member

Pushed changes, new API is ready. Some TODOs, javadoc updates in code are pending.

Member

mockitoguy commented Jan 16, 2017

Pushed changes, new API is ready. Some TODOs, javadoc updates in code are pending.

@bric3

This comment has been minimized.

Show comment
Hide comment
@bric3

bric3 Jan 17, 2017

Contributor

I will try to review the pushed change tonight. But I'm a tad overwhelmed at this moment.

Contributor

bric3 commented Jan 17, 2017

I will try to review the pushed change tonight. But I'm a tad overwhelmed at this moment.

@bric3

bric3 approved these changes Jan 18, 2017

I like it ! Great javadoc, I'm approving but I believe my new javadoc comments can be addressed before merging.

Show outdated Hide outdated src/main/java/org/mockito/MockitoSession.java
* {@code MockitoSession} helps driving cleaner tests but eliminating boilerplate code and adding extra validation.
* <p>
* {@code MockitoSession} is a session of mocking, during which the user creates and uses Mockito mocks.
* Typically the session is an execution of a single test method.

This comment has been minimized.

@bric3

bric3 Jan 18, 2017

Contributor

Is it compatible with with the TestNG tests lifecycle ?
JUnit supposedly creates one instance for each test method, while TestNG creates a single instance of the class.

@bric3

bric3 Jan 18, 2017

Contributor

Is it compatible with with the TestNG tests lifecycle ?
JUnit supposedly creates one instance for each test method, while TestNG creates a single instance of the class.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 18, 2017

Member

Thank you! I'll address your feedback.

@mockitoguy

mockitoguy Jan 18, 2017

Member

Thank you! I'll address your feedback.

Show outdated Hide outdated src/main/java/org/mockito/MockitoSession.java
* <p>
* {@code MockitoSession} is useful when you cannot use {@link MockitoJUnitRunner} or {@link MockitoRule}.
* Example use cases include TestNG users or tests that cannot use Mockito's JUnit support
* because they already use different JUnit support (Jukito, Springockito).

This comment has been minimized.

@bric3

bric3 Jan 18, 2017

Contributor

unclear.

@bric3

bric3 Jan 18, 2017

Contributor

unclear.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 18, 2017

Member

Nice, thanks for reading the new docs!

@mockitoguy

mockitoguy Jan 18, 2017

Member

Nice, thanks for reading the new docs!

@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 18, 2017

Member

I added game plan to the PR description. Thanks so much for review! The code will not be changed much (individual commits can be reviewed anyway).

Member

mockitoguy commented Jan 18, 2017

I added game plan to the PR description. Thanks so much for review! The code will not be changed much (individual commits can be reviewed anyway).

mockitoguy added a commit that referenced this pull request Jan 18, 2017

Continued to update the documentation
Updated the docs for consistency across multiple files. See game plan at #865.
private Strictness strictness;
@Override
public MockitoSessionBuilder initMocks(Object testClassInstance) {

This comment has been minimized.

@bric3

bric3 Jan 19, 2017

Contributor

Something just poped in my head, before merging, I thought that initMocks should be renamed to initAnnotations, initMocks is the old name when there was only @Mock annotation, but since a long time Mockito handles several types of annotations and has even a plugable annotation engine.

@bric3

bric3 Jan 19, 2017

Contributor

Something just poped in my head, before merging, I thought that initMocks should be renamed to initAnnotations, initMocks is the old name when there was only @Mock annotation, but since a long time Mockito handles several types of annotations and has even a plugable annotation engine.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 20, 2017

Member

Yes, initMocks is not fully accurate.

OTOH, users are used to "initMocks" method from MockitoAnnotations, so the API is consistent. Also, technically we are not "initializing annotations", we are initializing annotated fields, and most of the time we annotate mocks ;)

I'd say we keep it as "initMocks".

@mockitoguy

mockitoguy Jan 20, 2017

Member

Yes, initMocks is not fully accurate.

OTOH, users are used to "initMocks" method from MockitoAnnotations, so the API is consistent. Also, technically we are not "initializing annotations", we are initializing annotated fields, and most of the time we annotate mocks ;)

I'd say we keep it as "initMocks".

This comment has been minimized.

@bric3

bric3 Jan 20, 2017

Contributor

Yes technically it is indeed fields ;)
But initMocks is still inacurate. Ideas : initAnnotated (thanks to @raphw), processAnnotations, initFields. What I'd like is to give these users the hint that it's not only about mocks only, but about mockito annotations in general. And then in the javadoc one can mention the AnnotationEngine.

@bric3

bric3 Jan 20, 2017

Contributor

Yes technically it is indeed fields ;)
But initMocks is still inacurate. Ideas : initAnnotated (thanks to @raphw), processAnnotations, initFields. What I'd like is to give these users the hint that it's not only about mocks only, but about mockito annotations in general. And then in the javadoc one can mention the AnnotationEngine.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 20, 2017

Member

What about introduced inconsistency. Do you want to deprecate and rename MockitoAnnotations method, too?

@mockitoguy

mockitoguy Jan 20, 2017

Member

What about introduced inconsistency. Do you want to deprecate and rename MockitoAnnotations method, too?

This comment has been minimized.

@bric3

bric3 Jan 20, 2017

Contributor

I think the new name will speak more than the old name.
On MockitoAnnotations, this initMocks tickled me for quite some time, but since I use moregularly the JUnit runner or the JUnit rule, I didn't acted on it.

I think we could deprecate the MockitoAnnotations.initMocks and add the alternative method, it could in another PR though.

@bric3

bric3 Jan 20, 2017

Contributor

I think the new name will speak more than the old name.
On MockitoAnnotations, this initMocks tickled me for quite some time, but since I use moregularly the JUnit runner or the JUnit rule, I didn't acted on it.

I think we could deprecate the MockitoAnnotations.initMocks and add the alternative method, it could in another PR though.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 21, 2017

Member

Not sure about "initAnnotated(this)". Looks kind of awkward, half-finished and not speaking to me on what is actually going on. Verb + noun would be my preference. Other options: "initFields" or "initAnnotatedFields". The more I think about it, the more I like "initMocks" :) I acknowledge that it is not 100% accurate but it is slick and simple. At the end of the day, we're working with mocks!

There is a separate question about ROI. Does changing this method name actually make any tangible difference in the quality of the API? There is some churn caused by deprecation of 9-year old method in a very mature library that has lots of users (all code, sample and documentation all over the world).

@bric3, I'd like to move on with your original idea, e.g. session().initMocks(this). If we want to change initMocks in both, session and MockitoAnnotations, let's move it to a separate ticket.

@mockitoguy

mockitoguy Jan 21, 2017

Member

Not sure about "initAnnotated(this)". Looks kind of awkward, half-finished and not speaking to me on what is actually going on. Verb + noun would be my preference. Other options: "initFields" or "initAnnotatedFields". The more I think about it, the more I like "initMocks" :) I acknowledge that it is not 100% accurate but it is slick and simple. At the end of the day, we're working with mocks!

There is a separate question about ROI. Does changing this method name actually make any tangible difference in the quality of the API? There is some churn caused by deprecation of 9-year old method in a very mature library that has lots of users (all code, sample and documentation all over the world).

@bric3, I'd like to move on with your original idea, e.g. session().initMocks(this). If we want to change initMocks in both, session and MockitoAnnotations, let's move it to a separate ticket.

This comment has been minimized.

@bric3

bric3 Jan 22, 2017

Contributor

OK for initAnnotated, but what about processAnnotations.

@bric3, I'd like to move on with your original idea, e.g. session().initMocks(this). If we want to change initMocks in both, session and MockitoAnnotations, let's move it to a separate ticket.

That's an option bu in the meantime we introduce an API that could be deprecated. And in the next PR, it's probably gonna be discarded because the API has just been introduced in mockitosession and we won't deprecate something that just entered the scene. We rarely change incubating APIs. At least we could mention it in the javadoc.

@bric3

bric3 Jan 22, 2017

Contributor

OK for initAnnotated, but what about processAnnotations.

@bric3, I'd like to move on with your original idea, e.g. session().initMocks(this). If we want to change initMocks in both, session and MockitoAnnotations, let's move it to a separate ticket.

That's an option bu in the meantime we introduce an API that could be deprecated. And in the next PR, it's probably gonna be discarded because the API has just been introduced in mockitosession and we won't deprecate something that just entered the scene. We rarely change incubating APIs. At least we could mention it in the javadoc.

This comment has been minimized.

@bric3

bric3 Jan 22, 2017

Contributor

About ROI : mockitoSession will probably be more especially useful to framework developers. initMocks is for them the way to initialize mockito annotations. Or other annotations if they plug another AnnotationEngine. If we were to introduce other annotations that are not about mocks, it will feel awkward.

If we ever implement a way to inject non mock in the @InjectMocks field then initMocks will be even more awkward. If we add other annotations that non mock related, but for Mockito lifecycle it'll be even more off.

@bric3

bric3 Jan 22, 2017

Contributor

About ROI : mockitoSession will probably be more especially useful to framework developers. initMocks is for them the way to initialize mockito annotations. Or other annotations if they plug another AnnotationEngine. If we were to introduce other annotations that are not about mocks, it will feel awkward.

If we ever implement a way to inject non mock in the @InjectMocks field then initMocks will be even more awkward. If we add other annotations that non mock related, but for Mockito lifecycle it'll be even more off.

Show outdated Hide outdated src/main/java/org/mockito/internal/junit/UniversalTestListener.java
@@ -49,7 +49,7 @@ public void testFinished(TestFinishedEvent event) {
private void reportUnusedStubs(TestFinishedEvent event, Collection<Object> mocks) {
//If there is some other failure (or mismatches were detected) don't report another exception to avoid confusion
if (event.getFailure() == null && !stubbingLookupListener.mismatchesReported) {
if (event.getFailure() == null && !stubbingLookupListener.isMismatchesReported()) {

This comment has been minimized.

@bric3

bric3 Jan 20, 2017

Contributor

+1000 ;)

@bric3

bric3 Jan 20, 2017

Contributor

+1000 ;)

mockitoguy added a commit that referenced this pull request Jan 21, 2017

Continued to update the documentation
Updated the docs for consistency across multiple files. See game plan at #865.
@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 21, 2017

Member

I just rebased and bumped minor version to 2.7.0. This code is ready for final review and merge. I don't plan to work on the code unless there is more feedback.

The remaining unfinished action item in the description is a non-code change and does not block the review / merge.

Member

mockitoguy commented Jan 21, 2017

I just rebased and bumped minor version to 2.7.0. This code is ready for final review and merge. I don't plan to work on the code unless there is more feedback.

The remaining unfinished action item in the description is a non-code change and does not block the review / merge.

Show outdated Hide outdated src/main/java/org/mockito/internal/framework/DefaultMockitoSession.java
this.testClassInstance = testClassInstance;
listener = new UniversalTestListener(strictness, logger);
try {
Mockito.framework().addListener(listener);

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Why add a listener here? It seems that the code is trying to detect invalid usage of mocking, but in finishMocking this listener is automatically removed and manually invoked with testFinished. This seems like a misusage of Mockito.framework()

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Why add a listener here? It seems that the code is trying to detect invalid usage of mocking, but in finishMocking this listener is automatically removed and manually invoked with testFinished. This seems like a misusage of Mockito.framework()

This comment has been minimized.

@mockitoguy

mockitoguy Jan 22, 2017

Member

Why add a listener here?

When the session is started, the listener is added so that it can track creation of mocks and stubbing misuse.

In finish mocking we need to clean up state hence we remove the listener and report and findings.

Does it explain the workflow sufficiently? Do you suggest changes to the code / adding code comments?

@mockitoguy

mockitoguy Jan 22, 2017

Member

Why add a listener here?

When the session is started, the listener is added so that it can track creation of mocks and stubbing misuse.

In finish mocking we need to clean up state hence we remove the listener and report and findings.

Does it explain the workflow sufficiently? Do you suggest changes to the code / adding code comments?

This comment has been minimized.

@bric3

bric3 Jan 22, 2017

Contributor

Eventually wrap this try block in a private method that self explain itself.

@bric3

bric3 Jan 22, 2017

Contributor

Eventually wrap this try block in a private method that self explain itself.

@Override
public MockitoSession startMocking() {
//Configure default values
Object effectiveTest = this.testClassInstance == null ? new Object() : this.testClassInstance;

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

I would rather move this argument to the constructor, in my eyes it does not make sense to not track the testclassInstance when using this builder. Moreover, it does not notify framework authors when they are using the listener.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

I would rather move this argument to the constructor, in my eyes it does not make sense to not track the testclassInstance when using this builder. Moreover, it does not notify framework authors when they are using the listener.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 22, 2017

Member

I would rather move this argument to the constructor

To the constructor of DefaultMockitoSessionBuilder? Are you advocating for API modification:

//current
Mockito.session().initMocks(this).strictness(Strictness.WARN).startMocking();

//suggested by Tim
Mockito.session().strictness(Strictness.WARN).startMocking(this);
//or
Mockito.session(this).strictness(Strictness.WARN).startMocking();

Moreover, it does not notify framework authors when they are using the listener

This feedback is unclear to me.

@mockitoguy

mockitoguy Jan 22, 2017

Member

I would rather move this argument to the constructor

To the constructor of DefaultMockitoSessionBuilder? Are you advocating for API modification:

//current
Mockito.session().initMocks(this).strictness(Strictness.WARN).startMocking();

//suggested by Tim
Mockito.session().strictness(Strictness.WARN).startMocking(this);
//or
Mockito.session(this).strictness(Strictness.WARN).startMocking();

Moreover, it does not notify framework authors when they are using the listener

This feedback is unclear to me.

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Mockito.session(this).strictness(Strictness.WARN).startMocking(); yes that's what I meant.

This feedback is unclear to me.

To me the usage of the listener is confusing and thus far I do not understand how it is used and why. Probably I am missing an implementation detail.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Mockito.session(this).strictness(Strictness.WARN).startMocking(); yes that's what I meant.

This feedback is unclear to me.

To me the usage of the listener is confusing and thus far I do not understand how it is used and why. Probably I am missing an implementation detail.

This comment has been minimized.

@bric3

bric3 Jan 22, 2017

Contributor

This is confusing to me Mockito.session(this), especially since the instance could be null, if one wants it that way. And this is a builder.

I like this part of the API the way it is. The listener could be extracted in a private method ?

@bric3

bric3 Jan 22, 2017

Contributor

This is confusing to me Mockito.session(this), especially since the instance could be null, if one wants it that way. And this is a builder.

I like this part of the API the way it is. The listener could be extracted in a private method ?

This comment has been minimized.

@mockitoguy

mockitoguy Jan 23, 2017

Member

To me the usage of the listener is confusing and thus far I do not understand how it is used and why. Probably I am missing an implementation detail.

Good feedback. I'll try to rework the code / add code comments to make this part clearer.

@mockitoguy

mockitoguy Jan 23, 2017

Member

To me the usage of the listener is confusing and thus far I do not understand how it is used and why. Probably I am missing an implementation detail.

Good feedback. I'll try to rework the code / add code comments to make this part clearer.

This comment has been minimized.

@mockitoguy

mockitoguy Jan 23, 2017

Member

"initMocks(this)" is optional, the session will work without it. It also communicates the intent best when reading the code. @TimvdLippe, are you worried that users will forget to call this method?

"Mockito.session(this)" does not communicate the intent well and looks somewhat awkward. "startMocking(this)" is slightly better but suffers from similar problem. I'd say we stick to the current API in the PR.

@mockitoguy

mockitoguy Jan 23, 2017

Member

"initMocks(this)" is optional, the session will work without it. It also communicates the intent best when reading the code. @TimvdLippe, are you worried that users will forget to call this method?

"Mockito.session(this)" does not communicate the intent well and looks somewhat awkward. "startMocking(this)" is slightly better but suffers from similar problem. I'd say we stick to the current API in the PR.

This comment has been minimized.

@bric3

bric3 Jan 23, 2017

Contributor

I agree with @szczepiq.

@bric3

bric3 Jan 23, 2017

Contributor

I agree with @szczepiq.

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 23, 2017

Contributor

Okay fair points, let's leave it as is.

@TimvdLippe

TimvdLippe Jan 23, 2017

Contributor

Okay fair points, let's leave it as is.

Show outdated Hide outdated .../java/org/mockito/internal/session/DefaultMockitoSessionBuilderTest.java
public class DefaultMockitoSessionBuilderTest {
@Test public void creates_sessions() throws Exception {

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Why does this test not have any asserts?

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Why does this test not have any asserts?

This comment has been minimized.

@mockitoguy

mockitoguy Jan 22, 2017

Member

It is a test for the builder, if it creates objects that we can call startMocking() and finishMocking() on, it is a sufficient verification.

I'll improve it so that it is better and does not raise question like yours. Good feedback!

@mockitoguy

mockitoguy Jan 22, 2017

Member

It is a test for the builder, if it creates objects that we can call startMocking() and finishMocking() on, it is a sufficient verification.

I'll improve it so that it is better and does not raise question like yours. Good feedback!

//happy path
new DefaultMockitoSessionBuilder().initMocks(this).startMocking().finishMocking();
new DefaultMockitoSessionBuilder().initMocks(new Object()).startMocking().finishMocking();
new DefaultMockitoSessionBuilder().strictness(Strictness.LENIENT).startMocking().finishMocking();

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

What happens if you invoke for example startMocking twice? Would that be legal?

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

What happens if you invoke for example startMocking twice? Would that be legal?

This comment has been minimized.

@mockitoguy

mockitoguy Jan 22, 2017

Member

We have coverage for this scenario at org.mockitousage.stubbing.StrictStubbingEndToEndTest#detects_unfinished_mocking

However, it does not have correct documentation. Thank you for raising this. Nice catch. I will improve this scenario.

@mockitoguy

mockitoguy Jan 22, 2017

Member

We have coverage for this scenario at org.mockitousage.stubbing.StrictStubbingEndToEndTest#detects_unfinished_mocking

However, it does not have correct documentation. Thank you for raising this. Nice catch. I will improve this scenario.

mockito.finishMocking();
//TODO - currently we warn about "Unused" instead of "Arg mismatch" below
//because it was simpler to implement. This can be improved given we put priority to improve the warnings.

This comment has been minimized.

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Is it too big of a hassle to fix now instead of delaying?

@TimvdLippe

TimvdLippe Jan 22, 2017

Contributor

Is it too big of a hassle to fix now instead of delaying?

This comment has been minimized.

@mockitoguy

mockitoguy Jan 22, 2017

Member

Yes. Given that warnings are not that much useful, I would probably never prioritize it (unless users ask).

@mockitoguy

mockitoguy Jan 22, 2017

Member

Yes. Given that warnings are not that much useful, I would probably never prioritize it (unless users ask).

mockitoguy added a commit that referenced this pull request Jan 24, 2017

Continued to update the documentation
Updated the docs for consistency across multiple files. See game plan at #865.
@mockitoguy

This comment has been minimized.

Show comment
Hide comment
@mockitoguy

mockitoguy Jan 24, 2017

Member

If there is no more feedback, I'd like to merge. Thanks guys for comprehensive review and pointing out improvements!!!

Member

mockitoguy commented Jan 24, 2017

If there is no more feedback, I'd like to merge. Thanks guys for comprehensive review and pointing out improvements!!!

mockitoguy added some commits Dec 29, 2016

Mockito strictness without JUnit
In order to take advantage of Mockito strict stubbing (debuggability, cleaner tests) developers must use JUnit integration. This means that it is not available for people on different unit test frameworks / people who don't use Mockito's JUnit integration (runner, rule).

- Reviewable initial implementation
- Pending javadoc, API/method naming tuning
Avoided redundant exceptions
- split test code into separate test classes
- added TODOs
- added missing functionality in a form of an integ test
Renamed the name
The exception is no longer tied to JUnit rules
Fixed the test
We need to invoke finish mocking otherwise there is a leftover state
Added more coverage
Added integ test coverage to prevent leaking of strictness setting to surrounding tests
Safeguard for unfinished mocking
Added safeguard for unfinished mocking. Pending tidy-up.
Improved documentation
- javadoc
- exception messages

More javadoc work is pending

mockitoguy added some commits Jan 16, 2017

Refactorings per code review
Addressed code review feedback and refactored the code for readability and safety.
New MockitoSession public API
- Changed the public API to MockitoSession per our discussion at #857.
- MockitoSession offers meaningful name and semantics as opposed to somewhat cryptic MockitoMocking interface
- New builder style interface for constructing session instances

Pending

- lots of javadoc (TODOs in code)
- coverage for builder edge cases (e.g. when someone does not provide strictness or init mocks, etc.)
Added documentation and default behavior
- Documented new public API methods. More documentation is pending.
- Ensured predictable behavior for non-happy path. When 'null' values are configured, the builder will use sensible defaults.
- Set the default strictness to be "strict stubs" so that users can take advantage for cleaner tests and better productivity by default.
Updated the documentation
- added comprehensive documentation for new MockitoSession API
- mentioned new API from existing relevant methods (runner, the rule, mockito annotations)
Continued to update the documentation
1. Work still in progress
2. Mockito Strictness spans currently multiple classes and it's not easy to document it cohesively. Following types need to be reviewed for consistency:

  - UnnecessaryStubbingException
  - PotentialStubbingProblem
  - MockitoJUnitRunner and subclasses
  - MockitoSession
  - MockitoRule
  - Strictness
Continued to update the documentation
Updated the docs for consistency across multiple files. See game plan at #865.
Addressed Brice feedback
Some javadoc was unclear
Refactoring, addressed Brice feedback
Replaced package-local fields with setter and getter for clarity. Indeed it looks better because after all I needed only 2 out of 4 setter/getter methods and the public footprint of the class is smaller. Thank you for the suggestion!
Finalized "strictness" documentation
Made a pass through the docs, attempted to drive cohesive and comprehensive documentation.
Improved an internal test utility
- reduced duplication
- added new assertion method I will need today
MockitoSession validates framework usage
Ensured MockitoSession detects any incorrect framework usage caused by user's mistake
Main documentation includes MockitoSession
Main documentation in the Mockito class links to MockitoSession from the paragraph that describes strictness. This way the main documentation is complete and describes the entire Mockito API.
More documentation
Documented why JUnitRule does not use MockitoSession API, linked to separate ticket #898
Bumped minor version
Signified important feature, new API.
Improved the documentation and tests
Improved the documentation, exception message and tests wrt unfinished mocking session scenario.
Added code comments
Added code comments based on pull request feedback
Coverage for concurrent use of sessions
- added coverage demonstrating that mockito session can be used concurrently, so long single thread has single active mockito session
- tidied up the existing test: removed sys out println, fixed the naming issue
Improved assertion message
I noticed this test failed on CI, I cannot reproduce it locally

@mockitoguy mockitoguy changed the title from Strict stubbing support without JUnit to New strict stubbing API - MockitoSession Jan 29, 2017

@mockitoguy mockitoguy merged commit 3818e07 into release/2.x Jan 29, 2017

4 checks passed

codecov/patch 90.00% of diff hit (target 86.59%)
Details
codecov/project 86.59% (+<.01%) compared to d8f529e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@TimvdLippe TimvdLippe removed the in progress label Jan 29, 2017

@szpak szpak deleted the strict-stubbing branch May 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment