Repository: mockito/mockito
Issue: Allow spying on interfaces so that it is convenient to work with Java 8 default methods
Labels: new feature
java-8
noteworthy
please contribute

## Problem

While Mockito supports and stubs by default the _default methods_ in interfaces, the stubbing code can be a bit non intuitive, e.g.

``` java
interface DM {
    int contract();
    default int default_contract() { return contract() + 1; }
}

DM dm = mock(DM.class);
given(dm.contract()).willReturn(2);

// necessary otherwise default method is stubbed
given(dm.default_contract()).willCallRealMethod();

assertThat(dm.default_contract()).isEqualTo(3);
```

This behavior is unintuitive to users who expect the default methods to trigger real implementation (callRealMethod() by default). See also user report at #940.

## Suggested plan

:1st_place_medal: Contributions are welcome!

- [x] relax validation and allow interfaces to be spied. This way users can invoke spy(SomeInterface) or use @Spy with interfaces. This way, we don't need to mark default methods with "callRealMethod".
- [x] ensure test coverage for mocking
  - interfaces with and without default methods
  - concrete classes that extend interface with default methods (perhaps already covered)
- [x] document this use case. On main Mockito javadoc the use can search for "default" and find information about default methods behavior. Suggested by user at #940.
- [ ] create a separate ticket for Mockito 3 ("2.* incompatible" label) to discuss whether we should change the default behavior for defender/default methods. Perhaps they should automatically call real method regardless if someone uses spy() or mock() with the interface. Also we should consider mocking/spying on concrete classes that extend from interface with default methods.

## Discontinued ~~original plan~~

Below idea was discontinued:

Replace `DM` by `Map`, `default_contract()` by `getOrDefault()`, `contract()` by `get()` or `containsKey()` and you have a problem with designs that are used in the JDK itself.

I think mockito can improve on this by configuring the mock to invoke concrete default methods rather than stubbing them. This could be done the following way (_api naming in progress_) : 
- `mock(DM.class, USE_DEFAULT_METHODS)`
  
  The issue with that approach is that a default answer is mutually exclusive with other answers. e.g. if one wants to use `RETURNS_SMART_NULLS` and default methods this cannot work with the current design.
- `mock(DM.class, withSettings().useDefaultMethods())`
  
  This approach is interesting as it allows to configure the behaviour with possibly any answer. However this may require some changes with our internal answers, not a deal breaker though.


Issue comments:
> mock(DM.class, withSettings().useDefaultMethods())

I like that. 'spyDefaultMethods'?

BTW. What's better/more intuitive default for default methods? real method invocation or empty/unstubbed value?

I think that depends on the mocked type. I'd say :
- interface => use default methods by default
- concrete => I'm not sure I'd rather not make those default

`spyDefaultMethod` is wrong because the primary objective is not to spy them.

So potentially this is Mockito 3 backwards incompatible change (when changing defaults). Thanks for bringing this up!

For this thing only this could be potntially backward incompatible. Yet for this thing only I'm not yet sure we should change the default between 2.x and 3.x whatever the introduction or not of `useDefaultMethods` in `2.x`. 

I'm a bit undecided at that moment. I may have a better opinion when the code is actually done. 

Right now the code base of mockito 2.x runs Java6 so we don't have Java 8 tests, Mockito 3 (master) is Java 8 and we'll introduce Java 8 specific tests.

Makes perfect sense.

We already have code in 2.x that detects java 8. Up to you how to push that further ;)

If starting from scratch, I couldn't think of a use case that'd benefit from stubbing the default methods by default.

But maybe that use case does exist, or at least for backward compatibility it's too late to change?

Either way, I propose we do not need to add a default Answer or anything to mock(). 

Default methods in interfaces are much like concrete methods in abstract classes. `spy(AnAbstractClass.class)` today already _by default_ invokes concrete methods unless they are explicitly stubbed. It seems natural to me that we just relax the restriction on `spy(SomeInterface.class)` to accept interface as a parameter instead of throwing illegal usage exception as it does today.

@fluentfuture Can you elaborate on that : 

> It seems natural to me that we just relax the restriction on `spy(SomeInterface.class)` to accept interface as a parameter instead of throwing illegal usage exception as it does today.

Yes backward compatiblity is a primary concern. Yet if the change makes sense and doesn't impact users in a bad way it can be discussed.
Also maybe we could backport behavior and API from master to release/2x.
Sure.

Today, if java.util.Consumer were an abstract class defined as:

```java
public abstract class Consumer<T> {
  public Consumer<T> andThen(Consumer<? super T> that) {
    return v -> {accept(v); that.accept(v);};
  }

  public abstract void accept(T value);
}
```

I can `@Spy` it to have `andThen()` called by default:
```java
@Spy private final Consumer<String> consumer1;
@Spy private final Consumer<String> consumer2;

@Test public void testCompose() {
  consumer1.andThen(consumer2).accept("hi");
  verify(consumer1).accept("hi");
  verify(consumer2).accept("hi");
}
```

It doesn't work with Consumer being interface with `andThen()` being an equivalent default method:

```java
public interface Consumer<T> {
  default Consumer<T> andThen(Consumer<? super T> that) {
    return v -> {accept(v); that.accept(v);};
  }

  void accept(T value);
}
```

But that is just because we are having a validation that rejects spy(Consumer.class). That validation can be removed because it doesn't hurt even if you spy(AnInterface.class) and even if that interface has no default method, because it will just work the same way as mock(AnInterface.class).

Here's a [commit](https://github.com/fluentfuture/mockito/commit/afdd5ebc32de38d098c517bda32c83e194371574) in my fork that removes the validation and the above test passes.


By the way, I retract my suggestion to make `mock(AnInterface.class)` to call default methods, because then it would break the symmetry with `mock(AnEquivalentAbstractClass.class)`, which can be surprising.
Ah I see.

Imho spying an interface `spy(Interface.class)` is wrong, even when there's default methods, it doesn't feel like a a spy. As spy have more meaning to spy concrete code.

Instead mocking an interface `mock(Interface.class)` to call default methods only seems ok. And maybe backward compatible. Although I would like opt-in/out mechanism.
But mocking an abstract class `mock(AnEquivalentAbstractClass.class)` should never call concrete code.
I agree that the spy() name is kinda misleading for the behavior of "mock only abstract methods".

I originally proposed fake() or other names but spy() was chosen none-the-less.

I don't defend the spy() name but it's what we have, so there may be some value to consistency.


`spy` is a specialized mock that seems to _speak_ for everyone, maybe `fake` can be another compelling specilization.

At this moment not sure if we will, but maybe in the future.
Here's how I found peace with the spy() name:

Yes it calls the non-abstract methods, which isn't spying (more like fake). But it also allows you to explicitly stub them with `when()` or to `verify()` them, so in that sense, it can still be called a spy.
Regarding "interface" vs. "abstract class", I personally think we should try not to be picky about the difference.

Some project scoped or company scoped APIs out there aren't designed to be implemented/subclassed directly by users. For these APIs, going from an abstract class to interface or vice versa is reserved as an implementation detail change.

The worst that should happen with changing abstract class to interface is that a handful of direct subclasses break compilation and need to be fixed to use the "implements" keyword.

It'd be a much bigger burden if all the sudden a lot of tests that used to spy() on the abstract class fail just because of the abstract -> interface change.

Specifically, now with Java 8, I'd expect some abstract classes to become interfaces with default methods because the only reason they were abstract classes in the first place was to be able to have some default methods (for fluent chaining and stuff).
Interesting feedback. I still have mixed feeling about relaxing spy to allow an interface.
I'll sleep on it, and think about it as your migration use case is legitimate.

And anyway this address only a part of the original issue.
@bric3 Can you explain which part isn't addressed in the original issue, assuming we put aside the mock() vs. spy() naming difference?
@fluentfuture I meant regarding configurability of stubbing default method for a mock, not a spy.

Yet the use cases discussed above are defintely additional scenario to account for when we will implement the stuff.
@bric3 Are you okay with allowing @Spy on interfaces? If so, I'll send a PR.
@fluentfuture yes. I have worked on it in #906. But I won't have a computer near me for a week still.
Great discussion. 

+1 to allow spying on interfaces
-1 to adding new public API method like 'fake' - it would make the api more complicated just for the sake of a single use case
+ 0.5 to changing the default behavior of Mockito 3 to 'spy' interface defender methods by default. This way it is the most intuitive behavior users expect. See https://github.com/mockito/mockito/issues/940
I attempted to update the ticket description to reflect the outcome of the discussion. Feel free to change if needed.
If I were going to add another method I would call it `stub` based on [Martin Fowler's article describing the 3]( https://martinfowler.com/articles/mocksArentStubs.html).

Seems like `@Incubating Mockito.spy( Class<?> clazz )` more or less does what I expect. Ultimately finding that behavior though required quite a bit of research.

Whether or not adding `Mockito.stub( Class<?> clazz )` method would be worth it I can't say because I don't feel like I'm familiar enough with the underlying implementation/behavior to know how different `mock` and `spy` are. If those are just configuration differences then it's probably worth it, if there's more to it, maybe not. If `stub` could `CALL_REAL_METHODS`, but only work for `abstract` and `interface` and not be able to do `verify` that would make it different from a `spy`, imo.

Either way, more docs seem necessary. I might try to put those together for #940 
I somehow missed this ticket. 

Mockito should imho follow the idea of defender methods and allow API-evolution without  breaking client code/test. I other words if a class under test evolves and calls defender methods on a mocked interface the tests should not fail or should not be rewritten by replacing `@Mock` with `@Spy` or setting up special mocking settings like `withSettings().useDefaultMethods()`.

I think it is possibe to rewrite the `mock(..)` implementation to generate stubbed calls to default methods  without special mock settings.  

```java
public <T> T mock(Class<T> type){
    T mock=MOCKITO_CORE.mock(type);
    if (!typeToMock.isInterface())
            return mock;

    for (Method method : typeToMock.getDeclaredMethods()) {
        if (method.isDefault()) {
            doCallRealMethod().when(mock);
            callWithAnyMatchers(mock, method);
        }
    }
    return mock;
}

private static void callWithAnyMatchers(Object mock, Method method) {
    Class<?>[] parameterTypes = method.getParameterTypes();
    List<Object> defaultValues = new ArrayList<Object>();
    for (Class<?> type : parameterTypes) {
            defaultValues.add(Mockito.any(type));
    }
    
    try {
        method.invoke(mock, defaultValues.toArray());
    } catch (Exception e) {
        throw new MockitoException(e.getMessage(), e);
    }
}


```
This way clients can redefine the defender stubbings like overriding it in an implementation. 

@bric3 
@szczepiq 
@fluentfuture 
What do you think?



Is it any different from API-evolution of abstract classes?

In template-method pattern, if the SUT calls an abstract method, and then later evolves to call a default method that indirectly calls the abstract method, the exact same problem can happen, no?

This kind of evolution has always been possible. And it's not clear to me that the user always means to only stub/verify the abstract method. Sometimes the user may just want to stub/verify the method directly called, abstract or not.

It seems to me not the mocking framework's responsibility to guess the user's intention. Instead, if the user wants to stub/verify the direct method invocation, use `@Mock;` while if the user wants to stub/verify abstract methods, use `@Spy`.


> Is it any different from API-evolution of abstract classes?
>
>In template-method pattern, if the SUT calls an abstract method, and then later evolves to call a default method that indirectly calls the abstract method, the exact same problem can happen, no?

I agree if you see it like that they are pretty equal.

I want to avoid that `@Spy` is misused to stub in the first place. The word "spying" implies that there is something to spy on (the SUT), but for interfaces the "spying" make no sense we would spy on a mocked interface. 

>It seems to me not the mocking framework's responsibility to guess the user's intention. 

I agree here too! What about a littel helper that create stubbings that call the real default method:  `Mockito.delegateDefaultMethodsToReal(mockedInterface)`

I still suggest `stub` as the method name for that per previous reasons cited
Did a bit of archeology. #106 has a discussion both about the API and the naming.

In that thread I argued against `spy`: "All the use cases I know of for partial mocking don't need spying". 

At the time "stub" wasn't suggested but now I think about it, it seems to make sense.

Although, changing it may mean to introduce a new `@Stub` annotation, and deprecate `@Spy AbstractClass`, if we are willing to consider the ship not sailed yet.
Interesting discussion! Thank you for all suggestions.

>the tests should not fail or should not be rewritten by replacing @Mock with @Spy or setting up special mocking settings like withSettings().useDefaultMethods().

You are right. Ideally the tests only fail when a bug is introduced.

I have a feeling the discussion is getting broader and I am not sure if still discusses the issue reported :) My immediate reaction to new `@Stub` interface and potential deprecation of `@Spy` is -1 because I don't see clear value. However, please formulate a separate ticket with the use case, code samples, and the team will for sure review it!

Coming back to the original issue: relaxing spy annotation for interfaces is useful for spying on interfaces with default methods. The use case is reasonable and team is +1 to the change. Do we have new data / use cases that indicate that this change inappropriate?

Pull request review comments:
Nice! Thank you for updating the test!
Nice and clean! Thank you!

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/Mockito.java b/src/main/java/org/mockito/Mockito.java
index 8582cce078..35697eafd3 100644
--- a/src/main/java/org/mockito/Mockito.java
+++ b/src/main/java/org/mockito/Mockito.java
@@ -64,7 +64,7 @@
  *      <a href="#27">27. Delegate calls to real instance (Since 1.9.5)</a><br/>
  *      <a href="#28">28. <code>MockMaker</code> API (Since 1.9.5)</a><br/>
  *      <a href="#29">29. BDD style verification (Since 1.10.0)</a><br/>
- *      <a href="#30">30. Spying or mocking abstract classes (Since 1.10.12)</a><br/>
+ *      <a href="#30">30. Spying or mocking abstract classes (Since 1.10.12) and Java 8 default methods (Since release 2.x)</a><br/>
  *      <a href="#31">31. Mockito mocks can be <em>serialized</em> / <em>deserialized</em> across classloaders (Since 1.10.0)</a></h3><br/>
  *      <a href="#32">32. Better generic support with deep stubs (Since 1.10.0)</a></h3><br/>
  *      <a href="#32">33. Mockito JUnit rule (Since 1.10.17)</a><br/>
@@ -1008,7 +1008,7 @@
  *
  *
  *
- * <h3 id="30">30. <a class="meaningful_link" href="#spying_abstract_classes" name="spying_abstract_classes">Spying or mocking abstract classes (Since 1.10.12)</a></h3>
+ * <h3 id="30">30. <a class="meaningful_link" href="#spying_abstract_classes" name="spying_abstract_classes">Spying or mocking abstract classes (Since 1.10.12) and Java 8 default methods (Since release 2.x)</a></h3>
  *
  * It is now possible to conveniently spy on abstract classes. Note that overusing spies hints at code design smells (see {@link #spy(Object)}).
  * <p>
@@ -1021,6 +1021,9 @@
  * //convenience API, new overloaded spy() method:
  * SomeAbstract spy = spy(SomeAbstract.class);
  *
+ * // Mocking abstract methods, spying default methods of an interface
+ * Function<Foo, Bar> function = spy(Function.class);
+ *
  * //Robust API, via settings builder:
  * OtherAbstract spy = mock(OtherAbstract.class, withSettings()
  *    .useConstructor().defaultAnswer(CALLS_REAL_METHODS));
diff --git a/src/main/java/org/mockito/internal/configuration/SpyAnnotationEngine.java b/src/main/java/org/mockito/internal/configuration/SpyAnnotationEngine.java
index b27307662c..6723be77f2 100644
--- a/src/main/java/org/mockito/internal/configuration/SpyAnnotationEngine.java
+++ b/src/main/java/org/mockito/internal/configuration/SpyAnnotationEngine.java
@@ -55,7 +55,6 @@ public void process(Class<?> context, Object testInstance) {
                 Object instance;
                 try {
                     instance = field.get(testInstance);
-                    assertNotInterface(instance, field.getType());
                     if (MockUtil.isMock(instance)) {
                         // instance has been spied earlier
                         // for example happens when MockitoAnnotations.initMocks is called two times.
@@ -72,14 +71,7 @@ public void process(Class<?> context, Object testInstance) {
         }
     }
 
-    private static void assertNotInterface(Object testInstance, Class<?> type) {
-        type = testInstance != null ? testInstance.getClass() : type;
-        if (type.isInterface()) {
-            throw new MockitoException("Type '" + type.getSimpleName() + "' is an interface and it cannot be spied on.");
-        }
-    }
-
-    private Object spyInstance(Field field, Object instance) {
+    private static Object spyInstance(Field field, Object instance) {
         return Mockito.mock(instance.getClass(),
                             withSettings().spiedInstance(instance)
                                                            .defaultAnswer(CALLS_REAL_METHODS)
@@ -142,9 +134,9 @@ private static boolean typeIsPrivateAbstractInnerClass(Class<?> type, int modifi
     }
 
     //TODO duplicated elsewhere
-    private void assertNoIncompatibleAnnotations(Class<? extends Annotation> annotation,
-                                                 Field field,
-                                                 Class<? extends Annotation>... undesiredAnnotations) {
+    private static void assertNoIncompatibleAnnotations(Class<? extends Annotation> annotation,
+                                                        Field field,
+                                                        Class<? extends Annotation>... undesiredAnnotations) {
         for (Class<? extends Annotation> u : undesiredAnnotations) {
             if (field.isAnnotationPresent(u)) {
                 throw unsupportedCombinationOfAnnotations(annotation.getSimpleName(),


Test patch:
diff --git a/src/test/java/org/mockitousage/annotation/SpyAnnotationTest.java b/src/test/java/org/mockitousage/annotation/SpyAnnotationTest.java
index 157e08a65c..82c530b1f3 100644
--- a/src/test/java/org/mockitousage/annotation/SpyAnnotationTest.java
+++ b/src/test/java/org/mockitousage/annotation/SpyAnnotationTest.java
@@ -28,7 +28,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-@SuppressWarnings({"unchecked", "unused"})
+@SuppressWarnings("unused")
 public class SpyAnnotationTest extends TestBase {
 
     @Spy
@@ -59,19 +59,16 @@ public void should_init_spy_and_automatically_create_instance() throws Exception
     }
 
     @Test
-    public void should_prevent_spying_on_interfaces() throws Exception {
+    public void should_allow_spying_on_interfaces() throws Exception {
         class WithSpy {
             @Spy
             List<String> list;
         }
 
         WithSpy withSpy = new WithSpy();
-        try {
-            MockitoAnnotations.initMocks(withSpy);
-            fail();
-        } catch (MockitoException e) {
-            assertThat(e.getMessage()).contains("is an interface and it cannot be spied on");
-        }
+        MockitoAnnotations.initMocks(withSpy);
+        when(withSpy.list.size()).thenReturn(3);
+        assertEquals(3, withSpy.list.size());
     }
 
     @Test
diff --git a/src/test/java/org/mockitousage/annotation/WrongSetOfAnnotationsTest.java b/src/test/java/org/mockitousage/annotation/WrongSetOfAnnotationsTest.java
index 5f8595dbf7..ef0a646ec3 100644
--- a/src/test/java/org/mockitousage/annotation/WrongSetOfAnnotationsTest.java
+++ b/src/test/java/org/mockitousage/annotation/WrongSetOfAnnotationsTest.java
@@ -14,7 +14,6 @@
 
 import static junit.framework.TestCase.fail;
 
-@SuppressWarnings({"unchecked", "unused"})
 public class WrongSetOfAnnotationsTest extends TestBase {
 
     @Test(expected=MockitoException.class)
@@ -25,19 +24,13 @@ public void should_not_allow_Mock_and_Spy() throws Exception {
     }
 
     @Test
-    public void should_not_allow_Spy_and_or_InjectMocks_on_interfaces() throws Exception {
+    public void should_not_allow_Spy_and_InjectMocks_on_interfaces() throws Exception {
         try {
             MockitoAnnotations.initMocks(new Object() { @InjectMocks @Spy List<?> mock; });
             fail();
         } catch (MockitoException me) {
             Assertions.assertThat(me.getMessage()).contains("'List' is an interface");
         }
-        try {
-            MockitoAnnotations.initMocks(new Object() { @Spy List<?> mock; });
-            fail();
-        } catch (MockitoException me) {
-            Assertions.assertThat(me.getMessage()).contains("'List' is an interface");
-        }
     }
 
     @Test
diff --git a/src/test/java/org/mockitousage/constructor/CreatingMocksWithConstructorTest.java b/src/test/java/org/mockitousage/constructor/CreatingMocksWithConstructorTest.java
index 064cf7d2be..2295593e3f 100644
--- a/src/test/java/org/mockitousage/constructor/CreatingMocksWithConstructorTest.java
+++ b/src/test/java/org/mockitousage/constructor/CreatingMocksWithConstructorTest.java
@@ -127,6 +127,13 @@ public void abstract_method_stubbed() {
         assertEquals("abstract me", thing.fullName());
     }
 
+    @Test
+    public void interface_method_stubbed() {
+        List<?> list = spy(List.class);
+        when(list.size()).thenReturn(12);
+        assertEquals(12, list.size());
+    }
+
     @Test
     public void calls_real_interface_method() {
         List list = mock(List.class, withSettings().defaultAnswer(CALLS_REAL_METHODS));