15

I'm verifying that a function was called using Mockito, but Mockito is telling me that the function I'm verifying was never called and that other functions were called. But it seems to me that I'm calling the right function...

Here's the stack trace for the error I'm getting:

Wanted but not invoked:
relationshipAutoIndexer.getAutoIndex();
-> at org.whispercomm.manes.server.graph.DataServiceImplTest.testInitIndices(DataServiceImplTest.java:117)

However, there were other interactions with this mock:
-> at org.whispercomm.manes.server.graph.DataServiceImpl.updateIndexProperties(DataServiceImpl.java:136)
-> at org.whispercomm.manes.server.graph.DataServiceImpl.updateIndexProperties(DataServiceImpl.java:144)
-> at org.whispercomm.manes.server.graph.DataServiceImpl.updateIndexProperties(DataServiceImpl.java:148)
-> at org.whispercomm.manes.server.graph.DataServiceImpl.updateIndexProperties(DataServiceImpl.java:149)
-> at org.whispercomm.manes.server.graph.DataServiceImpl.initIndices(DataServiceImpl.java:121)

    at org.whispercomm.manes.server.graph.DataServiceImplTest.testInitIndices(DataServiceImplTest.java:117)

It occurs at

verify(relAutoIndexer).getAutoIndex();

of the test class code shown below.

Here is my code (I have a tendency to leave things out by accident. Please ask me for any code you think I'm missing and I'll add it):

public DataServiceImpl(GraphDatabaseService graphDb) {
    super();
    this.graphDb = graphDb;
    unarchivedParent = new UnarchivedParent(graphDb.createNode());
    archivedParent = new ArchivedParent(graphDb.createNode());
    packetParent = new PacketParent(graphDb.createNode());
    userParent = new UserParent(graphDb.createNode());
    this.initIndices();
}

/**
 * Initializes the node and relationship indexes.
 * 
 * Updates the set of indexed properties to match {@link DataServiceImpl}
 * .NODE_KEYS_INDEXABLE and {@link DataServiceImpl}.REL_KEYS_INDEXABLE.
 * 
 * Note: auto indices can also be configured at database creation time and
 * just retrieved at runtime. We might want to switch to that later.
 */
private void initIndices() {
    /* Get the auto-indexers */
    AutoIndexer<Node> nodeAutoIndexer = this.graphDb.index()
            .getNodeAutoIndexer();

    AutoIndexer<Relationship> relAutoIndexer = this.graphDb.index()
            .getRelationshipAutoIndexer();

    this.updateIndexProperties(nodeAutoIndexer,
            DataServiceImpl.NODE_KEYS_INDEXABLE);

    this.nodeIndex = nodeAutoIndexer.getAutoIndex();

    this.updateIndexProperties(relAutoIndexer,
            DataServiceImpl.REL_KEYS_INDEXABLE);

    this.relIndex = relAutoIndexer.getAutoIndex();
}

/**
 * Sets the indexed properties of an {@link AutoIndexer} to the specified
 * set, removing old properties and adding new ones.
 * 
 * @param autoIndexer
 *            the AutoIndexer to update.
 * @param properties
 *            the properties to be indexed.
 * @return autoIndexer, this given AutoIndexer (useful for chaining calls.)
 */
private <T extends PropertyContainer> AutoIndexer<T> updateIndexProperties(
        AutoIndexer<T> autoIndexer, Set<String> properties) {
    Set<String> indexedProps = autoIndexer.getAutoIndexedProperties();
    // Remove unneeded properties.
    for (String prop : difference(indexedProps, properties)) {
        autoIndexer.stopAutoIndexingProperty(prop);
    }

    // Add new properties.
    for (String prop : difference(properties, indexedProps)) {
        autoIndexer.startAutoIndexingProperty(prop);
    }

    // Enable the index, if needed.
    if (!autoIndexer.isEnabled()) {
        autoIndexer.setEnabled(true);
    }

    return autoIndexer;
}

And here's the code for the test class:

@Before
public void setup() {
   nA = mock(Node.class);
   nB = mock(Node.class);
   packetA = new PacketWrapper(nA);
   packetB = new PacketWrapper(nB);
   RelA = mock(Relationship.class);
   RelB = mock(Relationship.class);
   graphDb = mock(GraphDatabaseService.class);
   nodeAutoIndexer = (AutoIndexer<Node>) mock(AutoIndexer.class);
       relAutoIndexer = mock(RelationshipAutoIndexer.class);
}

@After
public void tearDown() {
  packetA = null;
  packetB = null;
}
/*
 * ---------------- Test initIndices() ---------------
 */
//TODO
@Test
public void testInitIndices() throws IllegalArgumentException, IllegalAccessException, InvocationTargetException, NoSuchMethodException {
   IndexManager indexManager = mock(IndexManager.class);
   when(graphDb.index()).thenReturn(indexManager);
   when(indexManager.getNodeAutoIndexer()).thenReturn(nodeAutoIndexer);
       when(graphDb.index().getRelationshipAutoIndexer()).thenReturn(relAutoIndexer);
   dataService = new DataServiceImpl(graphDb);
       verify(nodeAutoIndexer, atLeastOnce()).getAutoIndex();
       verify(relAutoIndexer).getAutoIndex();                       
}
Olimpiu POP
  • 5,001
  • 4
  • 34
  • 49
gsgx
  • 12,020
  • 25
  • 98
  • 149
  • I had a similar issue making polymorphic calls. I upgraded to the Mockito 1.9.0-rc1 version. This did not fix my issue. Is anyone else experiencing this ? –  Dec 07 '11 at 13:02

1 Answers1

26

Mockito, till version 1.8.5, had a bug in the case of polymorphic dispatch. It was fixed and is available in the first release candidate of the version 1.9.0. See issue 200.

So how does it happen in your code base. Note you are mocking these two classes

nodeAutoIndexer = (AutoIndexer<Node>) mock(AutoIndexer.class);
relAutoIndexer = mock(RelationshipAutoIndexer.class);

AutoIndexer happen to be a generic parent interface, in this interface there is this method ReadableIndex<T> getAutoIndex(). RelationshipAutoIndexer is a subtype of the AutoInexer where the generic part is fixed to Relationship, and override the getAutoIndex() method to return the covariant type ReadableRelationshipIndex.

See AutoIndexer and RelationshipIndexer.

Well, in your calling code you have these lines:

AutoIndexer<Node> nodeAutoIndexer = this.graphDb.index().getNodeAutoIndexer();
AutoIndexer<Relationship> relAutoIndexer = this.graphDb.index().getRelationshipAutoIndexer();
this.nodeIndex = nodeAutoIndexer.getAutoIndex();
this.relIndex = relAutoIndexer.getAutoIndex();

Both nodeAutoIndex in your production code and the mock nodeAutoIndexer in your test code have a reference of type AutoIndexer<Node>, so there's no problem regarding polymorphic dispatch. However relAutoIndex in your production code is referenced by the type AutoIndexer<Relationship> and the mock relAutoIndexer in your test code is referenced by the type RelationshipAutoIndexer, so the wrong call is registered on the mock and then fails verification.

Your solution is either to upgrade the mockito version; the 1.9.0 RC1 is very stable and a final release should be coming your way. Or you can migrate your reference type (in your production code) from :

AutoIndexer<Relationship> relAutoIndexer = this.graphDb.index().getRelationshipAutoIndexer();

to :

RelationshipAutoIndexer relAutoIndexer = this.graphDb.index().getRelationshipAutoIndexer();

A few other remarks.

  • You don't actually need to write an after method here as JUnit creates a new instance on each method run, so your method just adds code that will be done anyway. Note this isn't the case with TestNG.

  • Instead of creating your mocks in the before method, you might want to use Mockito annotations. Don't forget the runner.

For example :

@RunWith(MockitoJUnitRunner.class)
public class YourTest {
    @Mock SomeType someTypeMock;
    // ...
}
  • The stubbing code is a bit ugly for several reasons.

    • Your should write consistent stubs.

Why not write this in a cleaner way; for example referencing indexManager in both case :

IndexManager indexManager = mock(IndexManager.class);
when(graphDb.index()).thenReturn(indexManager);
when(indexManager.getNodeAutoIndexer()).thenReturn(nodeAutoIndexer);
when(indexManager.getRelationshipAutoIndexer()).thenReturn(relAutoIndexer);

Or don't reference it at all

IndexManager indexManager = mock(IndexManager.class);
when(graphDb.index()).thenReturn(indexManager);
when(graphDb.index().getNodeAutoIndexer()).thenReturn(nodeAutoIndexer);
when(graphDb.index().getRelationshipAutoIndexer()).thenReturn(relAutoIndexer);

Also having a mock that returns a mock is usually a sign of design smell. You are breaking the law of Demeter, and breaking it means you will experience difficult testing, bad maintainability, and difficult evolution. When I say that you could hear me whisper also (without the syllogisms) : it will cost you money. Don't write legacy code! If you are practicing TDD or BDD, you will identify these issues at design time for your own code, which is great to prevent them.

  • However if you are dealing with legacy code, you can use this deep stubs syntax :

Using the static methods you could write this

GraphDatabaseService graphdb = mock(GraphDatabaseService.class, RETURNS_DEEP_STUBS);

Or using the annotation you could write this :

@Mock(answer = RETURNS_DEEP_STUBS) GraphDatabaseService graphdb;

And the stubbing :

when(graphDb.index().getNodeAutoIndexer()).thenReturn(nodeAutoIndexer);
when(graphDb.index().getRelationshipAutoIndexer()).thenReturn(relAutoIndexer);
bric3
  • 40,072
  • 9
  • 91
  • 111
  • Thx, a good explanation was needed. Nope in fact a good explanation is always needed ;) – bric3 Oct 31 '11 at 14:48
  • This is the probably the best answer I've ever received on stackoverflow. My original question was solved, but when I'm calling graphDb.index().getNodeAutoIndexer(), these are objects/methods in a downloaded library (neo4j), so it's not my own code. So is it still bad practice to have a mock return a mock? I don't see any other way to test this. – gsgx Nov 01 '11 at 02:54
  • There's even another saying: _Don't mock types you don't own._ However to avoid statements such as `graphDb.index().getNodeAutoIndexer()`, you have to separate the concerns of your code. For example does your `GraphDatabaseService` needs the `GraphDB`, maybe you could only have a reference to the `IndexManager`. If not possible you can separate your indexing concerns in a new object that will take an `IndexManager`. Or you can let that here, because your overal design is good enough that it won't suffer breaking the Law of Demeter at this very place. It is your call ;) – bric3 Nov 01 '11 at 17:57