Mockito Spy'ing on the object being unit tested

Question

Is it a code smell to spy on an object that is being unit tested? For example say I have a LineCounter class whose job is to simply count the number of lines in a string. --

class LineCounter {
    public int getNumLines(String string) {
        String metadata = getStringMetadata(string);

        // count lines in file
        return numLines;
    }

    /** Expensive operation */
    protected String getStringMetadata(String string) {
        // do stuff with string
    }
}

Now I want to write a JUnit 4 test for this to test the getNumLines method while mocking out the expensive getStringMetadata call. I decide to use Mockito's spy mechanism to have getStringMetadata return a dummy value.

class LineCounterTests {
    @Test public void testGetNumLines() {
        LineCounter lineCounterSpy = Mockito.spy(new LineCounter());

        // Mock out expensive call to return dummy value.            
        Mockito.when(lineCounterSpy.getStringMetadata(Mockito.anyString()).thenReturn("foo");

        assertEquals(2, lineCounterSpy.getNumLines("hello\nworld");
    }
}

Is this a reasonable thing to do? I feel pretty weird testing a Spy object rather than the actual class, but I can't really think of a reason against it.

This might be a case of testing driving an improvement to your code. It looks to be like the job of obtaining string metadata should be extracted to another class, which is then delegated to by `LineCounter`. At that point you've created a "seam" and can mock that dependency out in a more conventional (and less smelly) way — millhouse, Oct 09 '13 at 00:43
@millhouse Yeah totally agree that way does seem like the *right* way to do it. I'm wondering though, is there anything inherently wrong with my example where the object being tested is a spy? — Matthew, Oct 09 '13 at 01:43

score 4 · Answer 1 · answered Oct 09 '13 at 10:34

4

I will answer the question in two parts. First, yes it is code smell to mock or spy the class under test. That does not mean that it cannot be done correctly but that it is risk prone and should be avoided whenever possible.

WRT your specific example, I would see how the spy could be correctly used but that would be predicated on the assertion that you have elsewhere fully unit tested getStringMetadata. This then begs the question, if you have fully unit tested getStringMetadata elsewhere then you must know how to test it and therefore why not test getNumLines without the spy.

All this being said, millhouse makes a good point but either way you have to unit test the expensive code somewhere. His suggestion goes a long way to help isolate the expensive code and ensure that you only have to test / exercise it once.

answered Oct 09 '13 at 10:34

John B

32,493
6
77
98

1

Suppose I have tested `getStringMetadata` elsewhere. I would still want to test `getNumLines` without having it perform the expensive call to `getStringMetadata` since I already know that that method behaves as expected. @millhouse's suggestion is valid, and I do like the approach of splitting `getStringMetadata` into another class, but my original question still remains. Is there anything *wrong* with my example. Or maybe a better way to ask this is, why is extracting `getStringMetadata` to another class and mocking that dependency when testing `LineCounter` a better approach? – Matthew Oct 09 '13 at 17:42
You also mention that spying the class under test is risk prone. Can you elaborate on that? – Matthew Oct 09 '13 at 17:42
You asked if it has "code smell". I suggest that it does. As soon as someone sees it they will think "Wait, what is going on here". That doesn't mean that it can't be done right, just that in general it should be avoided and so will catch a reviewer's attention. – John B Oct 10 '13 at 10:46
1

It is risk-prone because you might not be testing the class under test but testing your mock / spy instead. Even if you do it right the first time, someone else could come behind you and make a change to the class or test that would inadvertently cause the test to execute spy code where it is intending to test production code. – John B Oct 10 '13 at 10:47

score 3 · Answer 2 · answered Mar 15 '16 at 07:52

In this situation, it is perfectly legitimate to stub the method that is called by the method under test. It is even the only way I can think of to test it in isolation. You just don't want to extract a single method into it's own class for the sole purpose of testing.

Beware of the side effects in the stubbed method though. It might not be sufficient to stub the returned value, if the stubbed method has side effects then you have to stub the side effects as well. It might even be a reason against it in some situations where the side effects are very complex, but that would most likely be an indication of a code smell in the implementation of the class under test itself.

To answer your question, I find it easy to find reasons for it, but hard to find reasons against it. It's the technique I use every day, it helps me split my implementation in small methods that are tested individually in complete isolation, and I haven't seen any limitation to it yet.

I disagree. When you stub a method in the class under test that is called by the method under test, then this means that you are doing a white box test instead of a black box test. You test the implementation of the class under test, not its interface. This has the disadvantage that refactoring the class under test might require changes to the test, too. — olenz, Feb 29 '20 at 05:33
@olenz But aren't all Mockito (or any mocking framework) tests white box tests ? That being said, I agree with you that if you can you should use black box tests. — Thomas Naskali, Mar 09 '20 at 09:43

Mockito Spy'ing on the object being unit tested

2 Answers2

Linked