Spy with interface / spy final class / mock with delegate

[GoogleCodeExporter]

This is definitely an enhancement request, but here goes

1. Start with a final class that implements an interface
2. I want to spy an implementation of that final class
3. And fake some method calls.

=== Given:
final class FooImpl implements Foo {
  @Override Object doStuff() { .... }
  @Override void close() { .... }
}

=== For example:
FooImpl real = new FooImpl();
Foo mock = Mockito.mock(Foo.class, withSettings()
    .spiedInstance(real)
    .defaultAnswer(Mockito.CALLS_REAL_METHODS));
doNothing().when(mock).close();

// give mock to somebody, have then do work
// then validate the state of 'real' before it is closed

=== Error:
org.mockito.exceptions.base.MockitoException: 
Mocked type must be the same as the type of your spied instance.
Mocked type must be: FooImpl, but is: Foo
  //correct spying:
  spy = mock( ->ArrayList.class<- , withSettings().spiedInstance( ->new ArrayList()<- );
  //incorrect - types don't match:
  spy = mock( ->List.class<- , withSettings().spiedInstance( ->new ArrayList()<- );
    at com.tripwire.cap.logger.impl.pipeline.ResendEnvelopeOrderingTest.shouldAdjustPendingForSerial(ResendEnvelopeOrderingTest.java:48)

===
Given the error message, it looks like you've considered and rejected the usage 
I'm proposing, but I couldn't find anything in issues documenting your 
rationale.  

For myself, I'm a strong proponent of final classes (and am not alone, given 
"Item 17: Design and document for inheritance or else prohibit it" in Effective 
Java).  It seems unfortunate to make FooImpl non-final to be able to spy it 
directly.

Thanks,
-dg

Original issue reported on code.google.com by gil...@gmail.com on 15 Sep 2011 at 9:01

• Merged into: #145

[GoogleCodeExporter]

Hi,

I have mixed feeling about that one, however it might be ok to implemented for 
this case, if there is a consensus in the team.

About the exception, it's just here to prevent usage not provided by mockito, 
it can change if the feature is implemented or for some other reasons.

Mockito's core can only subclass existing implementations, for this reason we 
cannot work with final classes. With PowerMock which alter the class definition 
you could spy final classes.

While I'm some kind of a fervent admirer of Joshua Bloch, I think test driven 
development allows you balance well code with an open design, and classes non 
extendable through inheritance. In this last case, those object tend to be 
small in feature and code and then never mocked. Finally as I'm not a relly 
into inheritence, the result is that in essence the overall design is musch 
better.

Anyway thx for sharing your ideas, they have a point.

Original comment by brice.du...@gmail.com on 15 Sep 2011 at 10:57

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Thanks for the comment.   Hopefully the low API weight and the fact that you 
don't actually need (in this instance) to make a subclass of the spied type -- 
since it should work if you use a proxy based on the interface which delegates 
all non-intercepted methods to the real class -- will be enough to get this in.

I've only had to do something like this twice: once where the interface was 
small enough I could do it by hand easily, and once when it was large enough to 
use a java.lang.reflect.Proxy-based implementation.  I did miss the 
expressiveness of Mockito, but it was not crisis.

So, yeah.  Not a core use perhaps.  Thanks for the consideration.

Original comment by gil...@gmail.com on 16 Sep 2011 at 3:46

[GoogleCodeExporter]

Hi, potential feature in issue 275 can help you this case. What do you think of 
this :

mock(Foo.class, new ForwardingAnswer(real));

Or more user friendly :

mock(Foo.class, withSettings().forwardTo(real));

Original comment by brice.du...@gmail.com on 3 Oct 2011 at 8:41

[GoogleCodeExporter]

Original comment by brice.du...@gmail.com on 3 Oct 2011 at 8:41

[GoogleCodeExporter]

Both `mock(Foo.class, new ForwardingAnswer(real));` and `mock(Foo.class, 
withSettings().forwardTo(real));` read good to me.  I think both are 
understandable.  

==Ignorable babbling below==

It's (slightly) unfortunate that neither are type safe -- mock(Class,Answer) 
parameterizes Class but Answer is parameterized on the Answer type (duh) rather 
than the target type.  And MockSettings isn't type safe anyhow.  

In fairness, neither was my suggested syntax.

As an implementation aside, does `new ForwardingAnswer(real)` need to know 
about Foo.class?  I ask b/c it seems like InvocationOnMock#getMethod will 
return a method of Foo.  And so I guess you just `getMethod().invoke(real, 
getArguments())` and if that doesn't work (say, real no longer implements Foo) 
then that's an IllegalArgumentException at runtime.  

I think that's acceptable for something so edgy. 

Thanks again,
-dg

(P.S. The more I understand about the Mockito code base, the more I like it.  
Bravo!)

Original comment by gil...@gmail.com on 3 Oct 2011 at 6:00

[GoogleCodeExporter]

You raise a point, I guess using MockSettings is more reasonable on this 
matter; we could actually do some additional checks at mock creation.

Original comment by brice.du...@gmail.com on 4 Oct 2011 at 9:02

[GoogleCodeExporter]

I'm jumping in quite late, sorry about that. 

Brice, did you check why we force the types to be equal? I don't see a reason 
why we should but it might be dictated by the internal design. It feels that we 
should simply allow the suggested use case. I wonder what's going to happen if 
we remove that check and run all tests :D

Original comment by szcze...@gmail.com on 23 Oct 2011 at 7:50

[GoogleCodeExporter]

I think the real issue here is that the real instance is final and as such it 
could neither be mocked nor be spied.

About the control over the types, it's is important because the mock is created 
from the mockedType, and then fields from the original instance are copied to 
the mock.

So if I write this :

LinkedList<String> subtype = new LinkedList<String>() {{ add("Yup"); }};
List<String> list = mock(List.class, 
withSettings().spiedInstance(subtype).defaultAnswer(CALLS_REAL_METHODS));
assertNotNull(list.get(0));

Then it won't work because List.class has no real implementation or the wrong 
implementation. Same can be said if using an upped type such as AbstractList.

And anyway without defining the defaultAnswer a mock is just created but it 
might be without some or all fields of the spied instance.

Original comment by brice.du...@gmail.com on 25 Oct 2011 at 1:50

[GoogleCodeExporter]

>Then it won't work because List.class has no real implementation or the wrong 
implementation.

Hmm, not sure... (feel free to correct me if I'm wrong) 

list.get(0) is proxied because it is a method on a List. If it's proxied then 
our implementation can delegate to whoever implement List and is the 'spied 
instance'.

Makes sense?

Original comment by szcze...@gmail.com on 25 Oct 2011 at 8:44

[GoogleCodeExporter]

That's the trick here :)

For now we can only write this :
mock(ArrayList.class, withSettings().spiedInstance(new ArrayList()));

It works because ArrayList has the real implementations (for a spy). If we 
allow different types, such as the following :
mock(List.class, withSettings().spiedInstance(new ArrayList()));

Then we will only have a mock, the LenientCopy tool won't fail when copying the 
state of the original instance. After that the original spiedInstance is lost.

> list.get(0) is proxied because it is a method on a List. If it's proxied then 
our implementation can delegate to whoever implement List and is the 'spied 
instance'.

Agreed. But not with this kind of way, I find it cumbersome :
mock(List.class, 
withSettings().spiedInstance(newArrayList()).defaultAnswer(CALLS_REAL_METHODS));

Beside to make this work, it would require some rewriting on how spies are 
made. (For now without changing how spies are created the following call 
list.get(0) raise an exception because the implementation of the method is not 
found (tested))

That's why I was proposing this way of delegating to a real non proxied 
instance :
mock(List.class, withSettings().forwardTo(realArrayList));

Original comment by brice.du...@gmail.com on 26 Oct 2011 at 8:25

[GoogleCodeExporter]

Ok, I understand now. AFAIR, the very first implementation of spying in mockito 
was based on plain 'forwarding' idea. However, it proved limited due to 
non-proxied methods (final & private methods) accessing private state and 
breaking badly in runtime. Hence we 'improved' spying and added the logic that 
copied over the private state. I think that's the one of the reasons we needed 
exactly the same types.

Let's say we introduce 'forwardTo'. What would be the difference between 
'spiedInstance' + defaultAnswer? Do you want it to be just an alias?

Original comment by szcze...@gmail.com on 26 Oct 2011 at 9:13

[GoogleCodeExporter]

Yeah, makes sense to me.

About the future API, it shouldn't be aliases as I'm not sure if we could 
introduce regression, if we relax the spying api. And I'd like to think that 
people use Mockito.spy (or@Spy) to create spies, we should encourage that 
behavior in the message thrown by MockCreationValidator.
So I think the forwardTo, should actually prepare an answer that will forward 
to a real object reference. At the same time it will have the benefit of 
solving the case for people working with JNI/JNA stuff, as in issue 275.

Original comment by brice.du...@gmail.com on 26 Oct 2011 at 1:54

[GoogleCodeExporter]

In the end, duplicate of issue 145 ?

Original comment by brice.du...@gmail.com on 5 Mar 2012 at 4:25

[GoogleCodeExporter]

I think it's a good idea to merge them

Original comment by szcze...@gmail.com on 7 Mar 2012 at 11:49

[GoogleCodeExporter]

Ok, done

Original comment by brice.du...@gmail.com on 7 Mar 2012 at 11:52

• Changed state: Duplicate

[GoogleCodeExporter]

Hi all, 

We had a very nice "hacketgarten" yesterday in Paris, with a mockito workshop. 
Brice coached me to implement this new feature, which I did. I sent him the 
patch file for validation. Maybe it will make it to the source tree ? 

José

Original comment by Jose.Pau...@gmail.com on 8 Mar 2012 at 7:11