Repository: mockito/mockito
Issue: improve Mockito.mockingDetails API
Labels: enhancement

In order to make the API cleaner & expose useful information:
1. MockingDetails.getInvocations() throws meaningful exception when passed object is not a mock and documents this behavior
2. getMockedType() and getExtraInterfaces() are replaced with getMockCreationSettings() that returns MockCreationSettings instance.


Issue comments:
OK but let's deprecate them instead of removing them (possibly in mockito 3).

Given that getMockedType() and getExtraInterfaces() are unreleased yet (e.g. 2.0), do you still opt for deprecation?

good point, let's remove them then !


Pull request review comments:
I think the assertions should be on the constructor. This enable a fail fast behavior.

We cannot have them on constructor at the moment because isMock / isSpy methods permit null / non-mock arguments. Options:
1. separate isMock / isSpy API somewhere else
2. return empty / null result from getInvocations / getCreationSettings - then we don't need any exception
3. leave things as they are (merge PR :D)
4. ?

My thinking process

1 - API discoverability suffers
2 - poor API, no distinction between non-mock and mock without invocations, null handling required for the client code
3 - late exception not ideal but acceptable IMO

Good feedback!

> We cannot have them on constructor at the moment because isMock / isSpy methods permit null / non-mock arguments. 

Good point. That was a suggestion only. But that work too !

Discoverability won't suffer that much. Imho

@szczepiq We should use the ExpectedException Rule here. WDYT?

@ChristianSchwarz I'm not a fan of `ExpectedException` rule, this make the test a bit awkward, try/catch is a bit more bulky but it offers more control.

Also when we switch to Java8 after mockito 2, we can bump to AssertJ 3.x which has interesting assertions like : 

``` java
Assertions.assertThatThrownBy(() -> mockingDetails(null).getInvocations())
          .isInstanceOf(NotAMockException.class)
          .hasMessageContaining("Argument passed to Mockito.mockingDetails() should be a mock, but is null!")
```


Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/MockingDetails.java b/src/main/java/org/mockito/MockingDetails.java
index ee8496380e..534cbd02f8 100644
--- a/src/main/java/org/mockito/MockingDetails.java
+++ b/src/main/java/org/mockito/MockingDetails.java
@@ -5,9 +5,9 @@
 package org.mockito;
 
 import org.mockito.invocation.Invocation;
+import org.mockito.mock.MockCreationSettings;
 
 import java.util.Collection;
-import java.util.Set;
 
 /**
  * Provides mocking information.
@@ -36,27 +36,24 @@ public interface MockingDetails {
     /**
      * All method invocations on this mock.
      * Can be empty - it means there were no interactions with the mock.
+     * <p>
+     * This method is useful for framework integrators and for certain edge cases.
      *
      * @since 1.10.0
      */
     Collection<Invocation> getInvocations();
 
     /**
-     * Returns the type that is mocked. It is the type originally passed to
-     * the <code>{@link Mockito#mock(Class)}</code> or <code>{@link Mockito#spy(Class)}</code> function,
-     * or the type referenced by a Mockito annotation.
-     *
-     * @return The mocked type
-     * @since 2.0.0
-     */
-    Class<?> getMockedType();
-
-    /**
-     * Returns the extra-interfaces of the mock. The interfaces that were configured at the mock creation
-     * with the <code>{@link MockSettings#extraInterfaces(Class[])}</code>.
-     *
-     * @return The extra-interfaces
+     * Returns various mock settings provided when the mock was created, for example:
+     *  mocked class, mock name (if any), any extra interfaces (if any), etc.
+     * See also {@link MockCreationSettings}.
+     * <p>
+     * This method is useful for framework integrators and for certain edge cases.
+     * <p>
+     * If <code>null</code> or non-mock was passed to {@link Mockito#mockingDetails(Object)}
+     * then this method will throw with an appropriate exception.
+     * After all, non-mock objects do not have any mock creation settings.
      * @since 2.0.0
      */
-    Set<Class<?>> getExtraInterfaces();
+    MockCreationSettings<?> getMockCreationSettings();
 }
diff --git a/src/main/java/org/mockito/internal/util/DefaultMockingDetails.java b/src/main/java/org/mockito/internal/util/DefaultMockingDetails.java
index f7a91dfbba..5117ccbf96 100644
--- a/src/main/java/org/mockito/internal/util/DefaultMockingDetails.java
+++ b/src/main/java/org/mockito/internal/util/DefaultMockingDetails.java
@@ -5,12 +5,14 @@
 package org.mockito.internal.util;
 
 import org.mockito.MockingDetails;
+import org.mockito.exceptions.misusing.NotAMockException;
+import org.mockito.internal.InternalMockHandler;
 import org.mockito.invocation.Invocation;
-
-import static org.mockito.internal.util.MockUtil.getMockHandler;
+import org.mockito.mock.MockCreationSettings;
 
 import java.util.Collection;
-import java.util.Set;
+
+import static org.mockito.internal.util.MockUtil.getMockHandler;
 
 /**
  * Class to inspect any object, and identify whether a particular object is either a mock or a spy.  This is
@@ -36,17 +38,25 @@ public boolean isSpy(){
 
     @Override
     public Collection<Invocation> getInvocations() {
-        return getMockHandler(toInspect).getInvocationContainer().getInvocations();
+        return mockHandler().getInvocationContainer().getInvocations();
     }
 
     @Override
-    public Class<?> getMockedType() {
-        return getMockHandler(toInspect).getMockSettings().getTypeToMock();
+    public MockCreationSettings<?> getMockCreationSettings() {
+        return mockHandler().getMockSettings();
     }
 
-    @Override
-    public Set<Class<?>> getExtraInterfaces() {
-        return getMockHandler(toInspect).getMockSettings().getExtraInterfaces();
+    private InternalMockHandler<Object> mockHandler() {
+        assertGoodMock();
+        return getMockHandler(toInspect);
+    }
+
+    private void assertGoodMock() {
+        if (toInspect == null) {
+            throw new NotAMockException("Argument passed to Mockito.mockingDetails() should be a mock, but is null!");
+        } else if (!isMock()) {
+            throw new NotAMockException("Argument passed to Mockito.mockingDetails() should be a mock, but is an instance of " + toInspect.getClass() + "!");
+        }
     }
 }
 


Test patch:
diff --git a/src/test/java/org/mockito/MockingDetailsTest.java b/src/test/java/org/mockito/MockingDetailsTest.java
deleted file mode 100644
index ba870517a3..0000000000
--- a/src/test/java/org/mockito/MockingDetailsTest.java
+++ /dev/null
@@ -1,51 +0,0 @@
-package org.mockito;
-
-import org.junit.Test;
-import org.mockito.internal.MockitoCore;
-import org.mockito.invocation.Invocation;
-import org.mockitoutil.TestBase;
-
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-
-import static junit.framework.TestCase.*;
-import static org.mockito.Mockito.mock;
-
-public class MockingDetailsTest extends TestBase {
-    
-    @Test
-    public void should_provide_invocations() {
-        List<String> methodsInvoked = new ArrayList<String>() {{
-            add("add");
-            add("remove");
-            add("clear");
-        }};
-
-        @SuppressWarnings("unchecked")
-        List<String> mockedList = (List<String>) mock(List.class);
-        
-        mockedList.add("one");
-        mockedList.remove(0);
-        mockedList.clear();
-        
-        MockingDetails mockingDetails = new MockitoCore().mockingDetails(mockedList);
-        Collection<Invocation> invocations = mockingDetails.getInvocations();
-        
-        assertNotNull(invocations);
-        assertEquals(invocations.size(),3);
-        for (Invocation method : invocations) {
-            assertTrue(methodsInvoked.contains(method.getMethod().getName()));
-            if (method.getMethod().getName().equals("add")) {
-                assertEquals(method.getArguments().length,1);
-                assertEquals(method.getArguments()[0],"one");
-            }
-        }    
-    }
-
-    @Test
-    public void should_handle_null_input() {
-        //TODO 541, decide how to handle it and ensure the there is a top level integ test for the mockingDetails().getInvocations()
-        //assertTrue(new MockitoCore().mockingDetails(null).getInvocations().isEmpty());
-    }
-}
diff --git a/src/test/java/org/mockito/internal/util/DefaultMockingDetailsTest.java b/src/test/java/org/mockito/internal/util/DefaultMockingDetailsTest.java
index 3c151a96f5..225f1a0b48 100644
--- a/src/test/java/org/mockito/internal/util/DefaultMockingDetailsTest.java
+++ b/src/test/java/org/mockito/internal/util/DefaultMockingDetailsTest.java
@@ -1,16 +1,17 @@
 package org.mockito.internal.util;
 
+import junit.framework.TestCase;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Spy;
 import org.mockito.exceptions.misusing.NotAMockException;
 import org.mockito.runners.MockitoJUnitRunner;
+import org.mockitousage.IMethods;
 
-import java.util.*;
-
+import static junit.framework.TestCase.fail;
 import static org.junit.Assert.assertEquals;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.mockingDetails;
 
 @SuppressWarnings("unchecked")
 @RunWith(MockitoJUnitRunner.class)
@@ -18,6 +19,7 @@ public class DefaultMockingDetailsTest {
 
     @Mock private Foo foo;
     @Mock private Bar bar;
+    @Mock private IMethods mock;
     @Spy private Gork gork;
 
     @Test
@@ -42,28 +44,51 @@ public void should_check_that_a_spy_is_also_a_mock() throws Exception {
     }
 
     @Test
-    public void should_get_mocked_type() throws Exception {
-        assertEquals(Bar.class, mockingDetails(bar).getMockedType());
-    }
+    public void provides_invocations() {
+        //when
+        mock.simpleMethod(10);
+        mock.otherMethod();
 
-    @Test(expected = NotAMockException.class)
-    public void should_report_when_not_a_mockito_mock_on_getMockedType() throws Exception {
-        mockingDetails("not a mock").getMockedType();
+        //then
+        assertEquals(0, mockingDetails(foo).getInvocations().size());
+        assertEquals("[mock.simpleMethod(10);, mock.otherMethod();]", mockingDetails(mock).getInvocations().toString());
     }
 
     @Test
-    public void should_get_extra_interfaces() throws Exception {
-        Bar loup = mock(Bar.class, withSettings().extraInterfaces(List.class, Observer.class));
-        assertEquals(setOf(Observer.class, List.class), mockingDetails(loup).getExtraInterfaces());
+    public void provides_mock_creation_settings() {
+        //smoke test some creation settings
+        assertEquals(Foo.class, mockingDetails(foo).getMockCreationSettings().getTypeToMock());
+        assertEquals(Bar.class, mockingDetails(bar).getMockCreationSettings().getTypeToMock());
+        assertEquals(0, mockingDetails(mock).getMockCreationSettings().getExtraInterfaces().size());
     }
 
     @Test(expected = NotAMockException.class)
-    public void should_report_when_not_a_mockito_mock_on_getExtraInterfaces() throws Exception {
-        mockingDetails("not a mock").getExtraInterfaces();
+    public void fails_when_getting_creation_settings_for_incorrect_input() {
+        mockingDetails(null).getMockCreationSettings();
     }
 
-    private <T> Set<T> setOf(T... items) {
-        return new HashSet<T>(Arrays.asList(items));
+    @Test
+    public void fails_when_getting_invocations_when_null() {
+        try {
+            //when
+            mockingDetails(null).getInvocations();
+            //then
+            fail();
+        } catch (NotAMockException e) {
+            TestCase.assertEquals("Argument passed to Mockito.mockingDetails() should be a mock, but is null!", e.getMessage());
+        }
+    }
+
+    @Test
+    public void fails_when_getting_invocations_when_not_mock() {
+        try {
+            //when
+            mockingDetails(new Object()).getInvocations();
+            //then
+            fail();
+        } catch (NotAMockException e) {
+            TestCase.assertEquals("Argument passed to Mockito.mockingDetails() should be a mock, but is an instance of class java.lang.Object!", e.getMessage());
+        }
     }
 
     public class Foo { }