Partial #106

Closed
wants to merge 18 commits into
from

Projects

None yet

3 participants

@fluentfuture

Hi, Here's the implementation of the partial mock support I was talking about:

partialMock(Class);
partialMock(Object enclosingInstance, Class innerClass);

The second overload is the one solution to the arbitrary constructor parameter issue:

@Mock private Collaborator1 mockCollaborator1;
@Mock private Collaborator2 mockCollaborator2;
@PartialMock private MockWorker worker;

abstract class MockWorker extends Worker {
  MockWorker() {
    super(mockCollaborator1, mockCollaborator2);
  }
}

I didn't name it spy() for the following reasons:

  1. spy() doesn't work for final methods. It would be surprising if user's partial mock works without "final" and stops working when "final" is added. The non-abstract methods aren't meant to be mocked after all.
  2. With partial mock, one goal is to allow having state in the object. The shallow copying of spy() won't always work when there is mutable state, especially if there are private methods in the class that spy cannot intercept.
  3. "Spy" conceptually needs a target object to spy on. Without a target, spy(Foo.class) feels non-intuitive.
  4. All the use cases I know of for partial mocking don't need spying.
  5. If really needed, one can always explicitly spy(partialMock(Foo.class)).

Annotation support isn't added yet.

fluentfuture added some commits Oct 13, 2014
fluentfuture PartialMocker 7d16822
fluentfuture old version of CallbackFilter adef09a
fluentfuture javadoc eed4208
fluentfuture formatted b4f9110
@fluentfuture

Another use case I considered but would like to get some feedback before adding to the javadoc.

As the FAQ puts it:

Technically speaking Mockito is a Test Spy framework...
Test Spy framework allows to verify behaviour (like mocks) and stub methods (like good old hand-crafted stubs).

But it leaves one question open: should one ever use the "good old hand-crafted stubs" at all? If yes, under what scenario?

A main reason that people use the when().thenReturn() DSL instead of the hand-crafted stubs is that for interfaces with many methods, hand-crafting the stub means to write lots of extra boilerplate override methods that aren't interesting to the test.

But with partial mock, the above is no longer true. It's easy enough to hand-roll an abstract stub class, implementing only the interesting methods, and then partialMock() it.

Between the two different ways of stubbing (plain old @Override vs. when().thenReturn()), here's a guideline I've been following with success:

  • when().thenReturn() is best used to stub data logically related to the current test. That is, it's best used in the individual test methods. The DSL reads nicely in the BDD given-when-then style.
  • For stubbing shared data that aren't logically specific to any individual test, plain old abstract classes work better, because the when().thenReturn() DSL can usually lead to messy setUp() method with stubbing of several different objects crammed together. In other words, prefer partialMock(StubFoo.class) over when().thenReturn() statements in setUp().
@szczepiq
Member

Hey

Thanks so much for taking the time to implement this. Here's some feedback about your design decisions:

  1. I respect the arguments for avoiding reusing 'spy' nomenclature. For good or bad, Mockito 'spies' are called 'partial mocks'. This pattern is also used in other mocking libraries, like spock. IMHO, adding a separate method may confuse users.
  2. I think that the message that hand stubs are recommended for stubbing that typically ends up in setUp() method is a bit too strong. From my experience, rolling out hand stubs typically indicates design smell - there's too much 'ask' style interactions, and too little 'tell' style interactions. The latter better isolate responsibilities leading to cleaner design.

Now let's look into the code :)

@szczepiq
Member

Oh right, I reviewed the codes :)

One thing that stands out is that partial mock seems to be driving the API and the implementation towards a new kind of mock. Ideally, we would like to avoid it. In Mockito, we're trying to generalize more and avoid creation of new kinds of mocks.

To accept the PR, we would like to simplify the API and implementation. API:

  1. Mockito.spy(Foo.class, Object ... constructorArgs)
  2. withSettings().constructorArgs(Object ... args)
  3. withSettings().outerInstance(Object object)
  1. delegates to: mock(Foo.class, withSettings().constructorArgs(Object ... constructorArgs).defaultAnswer(CALLS_REAL_METHODS)
    Also, this should work with regular mocks, too. E.g. if someone specifies the constructorArgs (we can call it differently, e.g. 'usesConstructor', etc.) then the instance is created via the constructor, instead of by using the Objenesis.

This can be implemented incrementally. For example:

  1. add 'constructorArgs/usesConstructor' support
  2. add 'outerInstance/enclosingInstance' support
  3. add spy method

This is great stuff. Thanks for this PR. Sorry I cannot merge it in this form just yet. Perhaps I'll add some code to get this started.

@fluentfuture

Thanks for spending the time reviewing this code!

I was thinking that I cannot make spy work while also keeping final methods in fakes work.

But reading your reply a few times convinced me that I misunderstood how spies work. And it's actually pretty trivial to implement.

So, in short. I was wrong. spy(Class) tastes sweet!

What do you think of the new revision?

  • Mockito.spy(Foo.class, Object ... constructorArgs)

Again. I don't feel we need to support arbitrary constructor args. It's not statically type safe or refactoring friendly; there are ambiguity issues w.r.t constructor overloads . And lastly while 0-constructor-arg abstract classes without state can be fine (like, AbstractList), abstract classes with state feels more often a design smell.

  • withSettings().outerInstance(Object object)

Hmm. Kinda feel that the API of spy(outer, InnerClass.class) reads more intuitive: the outer object is the qualifier for the inner class, just as how we say outer.new InnerClass().

Also, unlike other mock settings that apply to every method that accepts it, outerInstance() is only meaningful to spy(). mock(Foo.class, settings.outerInstance(outer)) would be meaningless.

@szczepiq
Member

on my radar, I'll reply tomorrow ;)

On Sat, Oct 18, 2014 at 1:31 AM, fluentfuture notifications@github.com
wrote:

Thanks for spending the time reviewing this code!

I was thinking that I cannot make spy work while also keeping final
methods in fakes work.

But reading your reply a few times convinced me that I misunderstood how
spies work. And it's actually pretty trivial to implement.

So, in short. I was wrong. I like spy()!

What do you think of the new revision?

About the 3 items in your suggested API:

  1. Mockito.spy(Foo.class, Object ... constructorArgs)

Again. I don't feel we need to support arbitrary constructor args. It's
not type safe. I fear that adding this support would encourage bad-designed
abstract classes. If 0-constructor-arg abstract classes without state are
fine (like, AbstractList), abstract classes with state feel almost always a
smell.

  1. withSettings().outerInstance(Object object)

Hmm. Kinda feel that the API of spy(outer, InnerClass.class) reads more
intuitive: the outer object is the qualifier for the inner class, just as
how we say "outer.new InnerClass()". Agree? It's your call. If you prefer
outerInstance(), I'll change.

Reply to this email directly or view it on GitHub
#106 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

@szczepiq
Member

Again. I don't feel we need to support arbitrary constructor args. It's not statically type safe or refactoring friendly; there are ambiguity issues w.r.t constructor overloads . And lastly while 0-constructor-arg abstract classes without state can be fine (like, AbstractList), abstract classes with state feels more often a design smell.

I agree with everything you wrote. Thing is, we cannot really objectively say that stateful abstract classes are ok only when have 0-arg constructor. Consequently, we cannot restrict the API based on this assumption. Users will come up with use case for argumented abstract classes and it will be hard to defend it. Also, Mockito needs to balance out the 'toolkit' approach (unopinionated) with 'framework' approach (opinionated). To describe this further: from the standpoint of API design we can declare that in our opinion mocking abstract classes is a design smell and we don't want to support it officially (we could however, open some extension points for users to implement it 'unofficially'). But we cannot really say that some abstract classes are ok to mock and some not. At least, this is my view on the subject and I respect if you have a different POV.

IMHO, We don't have to provide constructor args in the first iteration but we should plan for it API-wise so that adding this feature later does not incur any deprecation or API awkwardness. We should also avoid documenting that we are not planning to do the constructor args because we believe it is a smell. However, we can document that typically mocking abstract classes hints at design smell, etc.

Hmm. Kinda feel that the API of spy(outer, InnerClass.class) reads more intuitive

spy() method and the withSettings() method are not "alternative" implementations :) All spy() methods need to delegate to a mock() method that has specific, spy-related withSettings(). Other words: every possible kind of mock can be created using this entry point of the API: http://mockito.github.io/mockito/docs/current/org/mockito/Mockito.html#mock(java.lang.Class,%20org.mockito.MockSettings) Think about the spy() method (and overloaded mock() methods) as a convenience methods / syntax sugar over the main mock creation entry point.

spy(outer, InnerClass.class)

I'd rather avoid overloaded spy() method that uses outer instance initially. It feels that mocking inner abstract classes is somewhat an edge case (arguably). We could provide an API for this via withSettings() though. From my objective standpoint it feels that it's more important to support constructor arguments than to support mocking inner classes (e.g. I would dare to say that there are more use cases that support the former feature).

I haven't yet looked in the code :)

@szczepiq szczepiq commented on an outdated diff Oct 22, 2014
src/org/mockito/Mockito.java
+ * FakeUserService service = Mockito.spy(FakeService.class);
+ * when(service.getUser("id")).thenReturn(AsyncFutures.failure(new Exception()));
+ * ...
+ * }
+ *
+ * abstract static class FakeUserService {
+ * public void getUser(String id, AsyncCallback<UserAccount> callback) {
+ * getUser(id).get(callback);
+ * }
+ *
+ * abstract AsyncFuture<UserAccount> getUser(String id);
+ * }
+ * }}</pre>
+ */
+ public static <T> T spy(Class<T> type) {
+ if (needsEnclosingInstance(type)) {
@szczepiq
szczepiq Oct 22, 2014 Member

Let's not do any validation in top level Mockito class. We should push that down the layers (possibly to the settings object) as it is done for other kinds of validation.

@szczepiq szczepiq and 1 other commented on an outdated diff Oct 22, 2014
src/org/mockito/Mockito.java
+ * <p>Because non-abstract methods aren't mocked, they cannot be {@link #when stubbed} or
+ * {@link #verify verified}. To stub or verify these methods, use a spy around the partial
+ * mock (only if the method isn't final).
+ */
+ public static <T> T spy(Object enclosingInstance, Class<T> innerClass) {
+ if (enclosingInstance == null) {
+ throw new NullPointerException("enclosingInstance");
+ }
+ if (!needsEnclosingInstance(innerClass)) {
+ throw new IllegalArgumentException(innerClass + " is not non-static inner class.");
+ }
+ if (!innerClass.getEnclosingClass().isInstance(enclosingInstance)) {
+ throw new IllegalArgumentException(
+ innerClass + " isn't inner class of " + enclosingInstance.getClass());
+ }
+ MockSettingsImpl<T> settings = new MockSettingsImpl<T>();
@szczepiq
szczepiq Oct 22, 2014 Member

I'd use withSettings() method to create the instance and use chained invocation / fluent api (take a look at other places in Mockito class).

@fluentfuture
fluentfuture Oct 22, 2014

This is because I wanted to expose spy(enclosingInstance, InnerClass.class) as the public API and hide settings.enclosingInstance() as internal implementation detail. And it's probably related to the merits discussion between inner class and constructor-args.

In a nutshell, what I'm trying to sell:

  • Inner class is an important core feature, a better alternative to constructor-args. Not an edge case.
  • enclosingInstance is only meaningful to spy(Class). So would be confusing if it's available to (mis)use in the other API methods.

I'll block this one our main discussion.

@szczepiq szczepiq commented on an outdated diff Oct 22, 2014
src/org/mockito/Mockito.java
+ */
+ public static <T> T spy(Object enclosingInstance, Class<T> innerClass) {
+ if (enclosingInstance == null) {
+ throw new NullPointerException("enclosingInstance");
+ }
+ if (!needsEnclosingInstance(innerClass)) {
+ throw new IllegalArgumentException(innerClass + " is not non-static inner class.");
+ }
+ if (!innerClass.getEnclosingClass().isInstance(enclosingInstance)) {
+ throw new IllegalArgumentException(
+ innerClass + " isn't inner class of " + enclosingInstance.getClass());
+ }
+ MockSettingsImpl<T> settings = new MockSettingsImpl<T>();
+ settings.defaultAnswer(new PartialMockAnswer(RETURNS_DEFAULTS));
+ settings.mockAbstractMethodsOnly();
+ settings.setEnclosingInstance(enclosingInstance);
@szczepiq
szczepiq Oct 22, 2014 Member

settings object does not use 'set' convention (for good or bad), we could rename the method to something more consistent.

@szczepiq szczepiq commented on an outdated diff Oct 22, 2014
src/org/mockito/Mockito.java
@@ -2233,4 +2362,8 @@ public static MockSettings withSettings() {
static MockitoDebugger debug() {
return new MockitoDebuggerImpl();
}
+
+ private static <T> boolean needsEnclosingInstance(Class<T> type) {
@szczepiq
szczepiq Oct 22, 2014 Member

this kind of method doesn't really belong to the top level mockito class. Mockito class responsibility is an API entry point.

@szczepiq szczepiq and 1 other commented on an outdated diff Oct 22, 2014
src/org/mockito/Mockito.java
+ * when(service.getUser("id")).thenReturn(AsyncFutures.failure(new Exception()));
+ * ...
+ * }
+ *
+ * abstract static class FakeUserService {
+ * public void getUser(String id, AsyncCallback<UserAccount> callback) {
+ * getUser(id).get(callback);
+ * }
+ *
+ * abstract AsyncFuture<UserAccount> getUser(String id);
+ * }
+ * }}</pre>
+ */
+ public static <T> T spy(Class<T> type) {
+ if (needsEnclosingInstance(type)) {
+ throw new IllegalArgumentException(
@szczepiq
szczepiq Oct 22, 2014 Member

Please ensure there's coverage for all new validation

@fluentfuture
fluentfuture Oct 22, 2014

Sure. I believe I have covered all of them.

@szczepiq szczepiq commented on an outdated diff Oct 22, 2014
...ockito/internal/creation/jmock/ClassImposterizer.java
private Object createProxy(Class<?> proxyClass, final MethodInterceptor interceptor) {
Factory proxy = (Factory) objenesis.newInstance(proxyClass);
proxy.setCallbacks(new Callback[] {interceptor, SerializableNoOp.SERIALIZABLE_INSTANCE });
return proxy;
}
-
- private Class<?>[] prepend(Class<?> first, Class<?>... rest) {
- Class<?>[] all = new Class<?>[rest.length+1];
- all[0] = first;
- System.arraycopy(rest, 0, all, 1, rest.length);
- return all;
- }
+
+ private interface UseConstructor {
@szczepiq
szczepiq Oct 22, 2014 Member

The constructor abstraction looks nice! Let's push that out of the imposterizer. At some point we need to start aligning the constructor code we have for initializing annotations.

@szczepiq szczepiq and 1 other commented on an outdated diff Oct 22, 2014
src/org/mockito/Mockito.java
+ * getUser(id).get(callback);
+ * }
+ *
+ * abstract AsyncFuture<UserAccount> getUser(String id);
+ * }
+ * }}</pre>
+ */
+ public static <T> T spy(Class<T> type) {
+ if (needsEnclosingInstance(type)) {
+ throw new IllegalArgumentException(
+ "Cannot mock non-static inner class " + type
+ + ". Please pass in the enclosing instance.");
+ }
+ MockSettingsImpl<T> settings = new MockSettingsImpl<T>();
+ settings.defaultAnswer(new PartialMockAnswer(RETURNS_DEFAULTS));
+ settings.mockAbstractMethodsOnly();
@szczepiq
szczepiq Oct 22, 2014 Member

'mockAbstractMethodsOnly' seems special-case'y and ambiguous to me:

a. If I create a mock of an abstract class and configure it to 'call real methods' then obviously real methods are called only for the non-abstract methods.
b. what does it mean 'mockAbstractMethods[Only]'? I can still "mock" the concrete, non-abstract methods (e.g. stub them). 'Only' implies that I cannot but I believe the implementation does not prevent it.

I really think that we should simply provide a method on settings object that informs mockito machinery that constructor should be used instead of objenesis.

@fluentfuture
fluentfuture Oct 22, 2014

That makes sense. How about useConstructor()?

@szczepiq
Member

I reviewed the code. Please don't get disheartened by the amount of review items :) I'm enjoying this conversation immensely and I'm really happy that you will contribute it! It will be a great feature in Mockito.

I've done some refactorings in Mockito regarding mock initialization and I will push it now. Merging your stuff will be hard and I'm sorry for this. I hope that the code after the refactorings will be easier for you to implement the feature ;)

@szczepiq
Member

Pushed. Take a look at org.mockito.internal.creation.cglib.ClassImposterizerTest#shouldCreateClassByConstructor

@fluentfuture

Thanks for explaining the rationale so clearly and patiently! Some further clarifications below in hope that I could sell the idea better. :)

I'll work on the refactoring and merging later.

Thing is, we cannot really objectively say that stateful abstract classes are ok only when have 0-arg constructor. Consequently, we cannot restrict the API based on this assumption. Users will come up with use case for argumented abstract classes and it will be hard to defend it. Also, Mockito needs to balance out the 'toolkit' approach (unopinionated) with 'framework' approach (opinionated). To describe this further: from the standpoint of API design we can declare that in our opinion mocking abstract classes is a design smell and we don't want to support it officially (we could however, open some extension points for users to implement it 'unofficially'). But we cannot really say that some abstract classes are ok to mock and some not. At least, this is my view on the subject and I respect if you have a different POV.

Agreed. The "opinion" is but one reason though. Another one is that it's not type safe, and would pose a difficulty to tools like IDE later when someone is trying to find references to a constructor.

While Mockito doesn't need to be an opinionated framework, I think it's fair to say that Mockito can choose to only implement a feature that it does better than alternatives.

I imagine if someday someone wants to spy a class with constructor args, my suggestion would be to create a simple subclass wrapper:

@Mock private Collaborator collaborator;
@Spy private MockFoo foo;
abstract class MockFoo extends AbstractFoo {
  MockFoo() {
    super(collaborator, blah);
  }
}

This way everything is still nicely compiler and tool friendly. One major difference between Mockito/EasyMock and jMock (at least the earlier versions) is that they are type safe. Supporting arbitrary constructor args using Object[] takes a step away from the type safety.

There may still be the 20% use case where the constructor args have to be provided dynamically so the inner class approach won't work. I wasn't sure if Mockito should charge itself to support this kind of rare edge cases. Starting from a minimalist perspective, I went with "when in doubt, leave it out".

I'm certainly biased. If that doesn't make the case. I respect your decision. :)

I'd rather avoid overloaded spy() method that uses outer instance initially. It feels that mocking inner abstract classes is somewhat an edge case (arguably). We could provide an API for this via withSettings() though. From my objective standpoint it feels that it's more important to support constructor arguments than to support mocking inner classes (e.g. I would dare to say that there are more use cases that support the former feature).

Regarding the pros/cons between constructor-args vs. inner class. My view is quite different. I see "constructor-args" as a crude half-baked product that no one except perhaps framework builders should ever use. There are even ambiguity problems we have to solve. What do we do for the following case?

abstract class WithParams {
  WithParams(Comparable, Runnable) {}
  WithParams(Serializable, Enum) {}
}

Comparable comparable = enum1;
Runnable runnable = enumThatImplementsRunnable;
spy(WithParams.class, comparable, runnable);

And what about generics?

abstract class WithParams {
  WithParams(Set<String>) {}
  WithParams(Iterable<Foo>) {}
}

spy(WithParams.class, fooSet);

Thanks to erasure, Mockito will have no way to figure out that fooSet's static type is Set<Foo>, so it'll still be puzzled even though from user's perspective there is no ambiguity whatsoever.

In both cases, the problem is that Mockito has no access to the declared "static" type that the compiler uses and users typically think in terms of. Mockito has access to the runtime types, which, if relied upon, can sometimes cause surprising behavior.

On the other hand, inner class is type safe, tool friendly, no surprise; constructor-args isn't.
We can use inner class to solve most use cases that seemingly need constructor-args.

Perhaps we can study a few use cases and see how the two contemplated APIs each pans out?

Re: withSettings(): I respect your preference for what the API looks like. So I'll just make sure my concerns are heard.

I have no concern passing these extra state through the settings object internally. The concern is with the overly wide API surface added if we allow users to do:

spy(anObject, withSettings().withEnclosingInstance(instance));

or.

mock(Foo.class, withSettings().withEnclosingInstance(...));

None of the above make logical sense so could turn out to be surprising to some users. If enclosingInstance is only logical to the spy(Class) method alone, do we want to make it possible to (mis)use to all the other API methods when users hit autocomplete?

Also consider this a slippery slope argument. If the other mock/fake methods later gain their own extra parameters that only make sense to individual API methods (as opposed to the cross-cutting settings today in MockSettings), do we cram them all into the MockSettings object so that users would have to bookmark in their mind things like: "setting 1 should only be used for API 1 and API 2 but not API 3 or API4; setting 2 is good for API 2 only; setting 3 is for API 2 and API 5".

It's contrived. But I feel by adding enclosingInstance to MockSettings, it's the first step toward that direction.

@fluentfuture

I fear my long reply did a poor job advertising the inner class support. Let me try in a mock FAQ style.

Why do I ever want to test a non-static inner class? Isn't it an implementation detail?

No. You don't.

The inner class support isn't really for testing your production inner classes. Rather, the idea is to reduce most abstract classes with constructor parameters to an inner class enclosed by the test.

Let's look at an example. How do you test the following class?

abstract class MyAbstractClass {
  MyAbstractClass(Collaborator collaborator) {...}

  abstract void doSomething();
}

The constructor has a parameter which is a collaborator. You can spy it by creating an inner class in the test as such:

public class MyAbstractClassTest {

  @Mock private Collaborator mockCollaborator;
  @Spy private MockAbstractClass foo;

  public void test1() {
    ...
    verify(foo).doSomething();
  }

  abstract class MockAbstractClass extends MyAbstractClass {
    MockAbstractClass() {
      super(mockCollaborator);
    }
  }
}

Why inner class? Why not just pass the constructor arguments directly?

A few problems with that approach:

  1. It's not statically type safe. We could have provided spy(Class, Object... constructorArgs). But then you lose static type checking. And when someone tries to figure out "who are the callers?". The spy() call evades that kind of static analysis and makes it harder to understand the bigger picture of the code.
  2. Overload ambiguities make it difficult to implement in a least astonishment way. Consider the following class:
abstract class MyAbstractClass {
  MyAbstractClass(Iterable<User> users);
  MyAbstractClass(Set<String> userIds);
}

The following spy call would be ambiguous:

Set<User> users = ...;
MyAbstractClass my = spy(MyAbstractClass.class, users);

While both you and compiler know for sure users is a set of User, thanks to type erasure, Mockito only knows users.getClass(), which is Set. Is it an Iterable<String> or Set<User>? No idea.

And that's just one parameter.

Sure. But you can throw an exception in case of ambiguity. I'm not sure how often people create potentially ambiguous overloads.

It can happen in practice that at day 1 there is only one constructor and then later on more are added. Despite the best practice of avoiding potentially ambiguous overloads, it occasionally happens, some even for good reasons. For example:

MyThing(Iterable<User> whenSizeIsUnknown) {...}
MyThing(Collection<User> whenSizeIsKnown) {...}

Seems harmless, right?

So overall, we figured that it's better (and easy enough with inner class) for the programmer to explicitly tell us which constructor to use and which parameters and types to pass to it, than Mockito making mostly-right-but-sometimes-surprising guesses.
Like:

public class TheTest {
  private Set<User> users;
  @Spy MockClass sut;
  ...

  abstract class MockClass extends MyAbstractClass {
    MockClass() {
      super(users);  // no amgibuity
    }
  }
}
@szczepiq
Member

Hey,

I admire your energy in selling the idea :). More feedback:

  • Majority of users (that I've seen) take advantage of @InjectMocks which already removes the type safety regarding the constructors.
  • We already have to deal with constructor selection for the @InjectMocks
  • Personally I find the choice between statically typed inner abstract classes VS duck-typed constructor parameters the matter of taste. Myself, I would choose test clarity and reduced boilerplate over better refactoring/searchability support for constructors.
  • I like this little pattern of yours. I think it is cool. Yet, I'm not prepared to shape Mockito API to force this pattern. It seems too opinionated. I'd rather give choice and have Mockito support both approaches.

We don't have to support constructor args. We can wait for community feedback. Perhaps nobody asks for it and everybody is happy :)

Let's conclude on the api:

spy(Foo.class)
mock(Foo.class, withSettings().innerOf(someObject).usingConstructor())
//options: "enclosingInstance", "constructor", "constructorArgs"

Makes sense? :)

@fluentfuture

We already have to deal with constructor selection for the @InjectMocks

It's not very comparable, right? In @InjectMocks, all the static types of the fields and the constructor signature are available to Mockito. If the constructor's signature is Foo(Set<User>), Mockito has full information of the signature. So there isn't any ambiguity problem.

When we do support duck typing, my suggestion would be something similar to EasyMock's ConstructorArgs (http://easymock.org/api/org/easymock/ConstructorArgs.html), which explicitly passes in the Constructor object to avoid constructor selection problem. It's not as handy, yes, but as you said, most users use @InjectMocks or @Spy anyway.

But yes, let's leave constructor-args for future iterations.

For now, the remaining gaps between use cases I'm having in mind and the suggested syntax:

  1. What if Foo's constructor is private and cglib cannot call it? My current implementation skips calling such constructor, as a way for users to suppress constructor when it does undesirable things. But if it does skip, useConstructor() would seem imprecise. useNonPrivateConstructor()? A bit mouthful, no?
  2. In the current implementation, spy(ClassWithParameteredConstructor.class) skips constructor, should it throw exception instead?
  3. In the current implementation, spy(InnerClass.class) throws exception, are you okay with that?

Thanks!

fluentfuture added some commits Oct 24, 2014
fluentfuture withSettings cebe8d2
fluentfuture merging e205c36
fluentfuture merged 2e95b84
fluentfuture ProxyMaker 3a46fd9
fluentfuture ProxyMaker e91eac2
@fluentfuture

Okay. I merged with upstream. I believe I've also implemented the suggested changes.

Please take another look.

Thanks!

@coveralls

Coverage Status

Changes Unknown when pulling e91eac2 on fluentfuture:partial into * on mockito:master*.

fluentfuture Some cleanups fd861ec
@coveralls

Coverage Status

Changes Unknown when pulling fd861ec on fluentfuture:partial into * on mockito:master*.

fluentfuture added some commits Oct 24, 2014
fluentfuture reformatted 131f144
fluentfuture reformatted e15bf6d
@szczepiq szczepiq commented on the diff Oct 24, 2014
src/org/mockito/Mockito.java
+ * public void getUser(String id, AsyncCallback<UserAccount> callback) {
+ * getUser(id).get(callback);
+ * }
+ *
+ * abstract AsyncFuture<UserAccount> getUser(String id);
+ * }
+ * }}</pre>
+ */
+ public static <T> T spy(Class<T> type) {
+ return mock(type, withSettings()
+ .useConstructor()
+ .defaultAnswer(new PartialMockAnswer(RETURNS_DEFAULTS)));
+ }
+
+ /**
+ * Spies {@code type} by constructing it as a partial mock.
@szczepiq
szczepiq Oct 24, 2014 Member

I need to think about better place for documenting such use cases.

@szczepiq szczepiq commented on an outdated diff Oct 24, 2014
src/org/mockito/internal/creation/MockSettingsImpl.java
@@ -141,6 +141,19 @@ public boolean hasInvocationListeners() {
return validatedSettings(typeToMock, this);
}
+ public MockSettings useConstructor() {
+ this.usesConstructor = true;
+ return this;
+ }
+
+ public MockSettings enclosingInstance(Object enclosingInstance) {
+ if (enclosingInstance == null) {
+ throw new NullPointerException("enclosingInstance");
@szczepiq
szczepiq Oct 24, 2014 Member

Let's throw a meaningful exception with tailored message. See how it's done in other places in MockSettingsImpl

@szczepiq szczepiq commented on an outdated diff Oct 24, 2014
...ockito/internal/creation/cglib/ClassImposterizer.java
@@ -80,75 +100,107 @@ private static String describeClass(Object instance) {
}
//TODO this method does not belong here
- public void setConstructorsAccessible(Class<?> mockedType, boolean accessible) {
+ public static void setConstructorsAccessible(Class<?> mockedType, boolean accessible) {
@szczepiq
szczepiq Oct 24, 2014 Member

this method does not belong here (you don't need to change it for this PR, just FYI :)

@szczepiq
Member

This is looking good. I should merge soon.

a. What if Foo's constructor is private and cglib cannot call it? My current implementation skips calling such constructor, as a way for users to suppress constructor when it does undesirable things.

If the users configures 'useConstructor' I think that mockito should attempt to find matching constructor. If it cannot be found or used, we I'd throw a meaningful exception.

b. In the current implementation, spy(ClassWithParameteredConstructor.class) skips constructor, should it throw exception instead?

I think we would apply the same strategy as for a)

c. In the current implementation, spy(InnerClass.class) throws exception, are you okay with that?

Yes, so long the exception message is clear and informs how to resolve the problem, etc.

@fluentfuture

Private constructor

There is a use case we are missing if we throw on private constructor: sometimes you may want to fake a class that didn't expose its constructor, by exposing a static factory instead, or, if the constructor does bad things (like, connect to database).

If we throw on private constructor, there would be absolutely no way to fake it, unless we create another way for user to say "fake it, but don't call constructor".

It's weird if the user called useConstructor() yet it ends up not using constructor. But if user only called spy(Foo.class), I think it's not weird if we skip private constructor.

What do you think of useConstructorIfPossible()?

@fluentfuture

Sorry I hadn't realize Travis wasn't happy with the broken javadoc link.

Fixed now.

Let me know what you think of the current design.

Thanks!

@szczepiq
Member
szczepiq commented Nov 6, 2014

Hey, I'll allocate some time to it soon. Needed to finish off some
automation work first. You've started on a core feature so it's lagging a
bit.

Cheers!

On Wed, Nov 5, 2014 at 4:29 AM, fluentfuture notifications@github.com
wrote:

Sorry I hadn't realize Travis wasn't happy with the broken javadoc link.

Fixed now.

Let me know what you think of the current design.

Thanks!

Reply to this email directly or view it on GitHub
#106 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

@szczepiq
Member

I've merged this PR in slightly changed form. Let me know if you want to improve it one way or another :)

@fluentfuture

Awesome! Thanks.

Do you like me to add the @Spy AbstractClass support, or you'll just do it?

@szczepiq
Member

Do you mean changing the @Spy annotation so that it out-of-the box works with AbstractClasses? For sure :) Let's do separate PR/ticket for it.

BTW. I'd like to mention you in the release notes by name. Can you reveal yourself (or configure your github account :)

@szczepiq szczepiq closed this Nov 17, 2014
@fluentfuture fluentfuture deleted the unknown repository branch Nov 18, 2014
@fluentfuture

Hi, thanks for taking over and cleaning it up!

I have a question about the current implementation.

If I'm reading the code correctly, spy(AbstractClass.class) can construct an instance using the proxy class's constructor.

But it should have mocked the abstract methods rather than CALL_REAL_METHOD on them.

The following test shows what I expect to happen but fails in the current master branch:

    static abstract class AbstractThing {
        abstract String message();
        String fullMessage() {
            return "Hi " + message();
        }
    }

    @Test
    public void abstract_method() {
        AbstractThing thing = Mockito.spy(AbstractThing.class);
        assertEquals("Hi null", thing.fullMessage());
    }

In summary, abstract methods by default should use the standard Mockito stubbing. Only the real non-abstract methods are by default invoked. The PartialMockAnswer class in this PR was supposed to implement this logic.

@szczepiq
Member

Good point! I missed that ;) Do you want to fix it in a separate PR?

@fluentfuture

I have a design question to run by you before creating the PR.

Do you think it makes sense to change CALLS_REAL_METHOD directly? It seems useless for it to call abstract methods only to throw an AbstractMethodError.

So perhaps CALLS_REAL_METHOD can be made a bit smarter to delegate to RETURNS_DEFAULTS when it knows it's pointless to call abstract method?

I guess then spy() doesn't need to change.

@szczepiq
Member

I think this is a good idea! I'm curious, what's the current behavior when CALLS_REAL_METHOD is used for abstract method? What's the behavior when someone tries to mock an interface and sets the default answer to be CALLS_REAL_METHOD?

Thanks for your insights!

@fluentfuture

Here's the error I just got from using CALLS_REAL_METHOD on interface methods:

org.mockito.exceptions.base.MockitoException: 
Cannot call abstract real method on java object!
Calling real methods is only possible when mocking non abstract method.
  //correct example:
  when(mockOfConcreteClass.nonAbstractMethod()).thenCallRealMethod();
@szczepiq
Member

Cool. We can make CALLS_REAL_METHOD smarter I think. Let's also update it's
documentation.

Thanks a lot for your help!!!!

On Sat, Nov 22, 2014 at 5:31 PM, Ben Yu notifications@github.com wrote:

Here's the error I just got from sing CALLS_REAL_METHOD on interface
methods:

org.mockito.exceptions.base.MockitoException:
Cannot call abstract real method on java object!
Calling real methods is only possible when mocking non abstract method.
//correct example:
when(mockOfConcreteClass.nonAbstractMethod()).thenCallRealMethod();

Reply to this email directly or view it on GitHub
#106 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

@fluentfuture

Sorry, Szczepan. Two more questions while implementing @Spy.

I think we agreed that for this code @Spy AbstractClass foo, we'll use Mockito.spy(AbstractClass.class).

The question is about @Spy NonAbstractClass foo.

Currently, @Spy fails on interface or abstract classes. For concrete classes, it first tries to instantiate an instance by calling no-arg constructor of it, and then spy on that instance.

Now with support for abstract class, there is a chance that we could handle all 3 cases consistently: just do Mockito.spy(TheType.class). This approach feels appealing to me because there is less special casing and is easier to understand.

Which approach would you prefer the @Spy annotation to do?

@szczepiq
Member

Your suggestion makes perfect sense (e.g. simplify, use new feature). BTW.
can you write such emails to the dev list?

On Sun, Nov 23, 2014 at 6:11 AM, Ben Yu notifications@github.com wrote:

Sorry, Szczepan. Two more questions while implementing @Spy.

I think we agreed that for this code @Spy AbstractClass foo, we'll use
Mockito.spy(AbstractClass.class).

The question is about @Spy NonAbstractClass foo.

Currently, @Spy fails on interface or abstract classes. For concrete
classes, it first tries to instantiate an instance by calling no-arg
constructor of it, and then spy on that instance.

Now with support for abstract class, there is a chance that we could
handle all 3 cases consistently: just do Mockito.spy(TheType.class). This
approach feels appealing to me because there is less special casing and is
easier to understand.

Which approach would you prefer the @Spy annotation to do?

Reply to this email directly or view it on GitHub
#106 (comment).

Szczepan Faber
Core dev@gradle; Founder@mockito

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