Correct way to unit test class with inner class

Question

Class A has inner class B. Class A has a private list of class B objects that it makes available through getBs, addB, and removeB methods. How do I unit test the removeB method?

I was hoping to create two equal mocks of class B, add each, and then remove one of them twice (result being to remove both). However, I've since learned through failure that the equals method will not be called on mock objects.

Was it foolish (or impossible) to try and isolate an outer class from its inner class for unit testing?

---

Sample code follows

Class to test:

import java.util.ArrayList;
import java.util.List;

public class Example {
    private List<Inner> inners = new ArrayList<Inner>();

    public List<Inner> getInners() {
        return inners;
    }

    public void addInner(Inner i) {
        inners.add(i);
    }

    public void removeInner(Inner i) {
        inners.remove(i);
    }

    /**
     * equalityField represents all fields that are used for testing equality
     */
    public class Inner {
        private int equalityField;
        private int otherFields;

        public int getEqualityField() {
            return equalityField;
        }

        public Inner(int field) {
            this.equalityField = field;
        }

        @Override
        public boolean equals(Object o) {
            if (o == null)
                return false;
            if (o.getClass() != this.getClass())
                return false;
            Inner other = (Inner) o;
            if (equalityField == other.getEqualityField())
                return true;
            return false;
        }
    }
}

Test case that didn't work out so great:

import static org.junit.Assert.*;
import org.junit.Test;
import org.easymock.classextension.EasyMock;

public class ExampleTest {
    @Test
    public void testRemoveInner() {
        Example.Inner[] mockInner = new Example.Inner[2];
        mockInner[0] = EasyMock.createMock(Example.Inner.class);
        mockInner[1] = EasyMock.createMock(Example.Inner.class);        

        Example e = new Example();
        e.addInner(mockInner[0]);
        e.addInner(mockInner[1]);
        e.removeInner(mockInner[0]);
        e.removeInner(mockInner[0]);
        assertEquals(0, e.getInners().size());
    }
}

Solution 1

Why do you have to mock you inner class? If I was faced with this issue I would just use the class as is and test that the behaviour of the outer class works as expected. You don't need to mock the inner class to do that.

As an aside: when overriding the equals() method it is advisable to also override the hashCode() method, too.

Solution 2

First, the answer to your question: Yes, it is generally a bad idea to try and separate an inner and outer class when unit testing. Usually, this is because the two are intimately linked, for instance Inner only makes sense in the context of Outer, or Outer has a factory method which returns an implementation of an interface, an Inner. If the two aren't really linked, then separate them out into two files. It makes your testing life easier.

Secondly, (using the above code as an example), you don't actually need to mock the above code. Just create some instances and away you go. It looks like you have enough to work with. You can always do something like:

public void testRemoveInner() {
    Example.Inner[] inner = new Example.Inner(45);

    Example e = new Example();
    e.addInner(inner);
    e.addInner(inner);
    e.removeInner(inner);
    assertEquals(0, e.getInners().size());
}

No mocks necessary.

Thirdly, try and work out what you are actually testing. In the above code, you're testing that if I add something to a list, then I can remove it. By the way, you state that if there are multiple objects which are 'equal'; the above code doesn't do that, from the definition of Collection#remove():

Removes a single instance of the specified element from this collection, if it is present (optional operation). More formally, removes an element e such that (o==null ? e==null : o.equals(e)), if this collection contains one or more such elements.

Is that really what you want to test?

Fourthly, if you implement equals, implement hashCode as well (see Overriding equals and hashCode in Java).

Solution 3

As a TDD newb who has just encountered this situation for the first time, I have found that I arrived at having "untestable" inner classes as a result of refactoring, and if using a "pure" TDD approach I wonder if you could end up with inner classes any other way.

The trouble is that, assuming one or more references are made to the outer class object from the inner class, this particular refactoring will often break one or more tests. The reason for this is pretty simple: your mock object, if a spy, is actually a wrapper around a real object

MyClass myClass = spy( new MyClass() );

... but the inner classes will always reference the real object, so it will often be the case that attempting to apply mocks to myClass won't work. Worse, even without mocks there is a high likelihood that the thing will fail utterly and inexplicably just going about its normal business. Also be aware that your spy will not have run the real constructor method for itself: lots to go wrong.

Given that our development of our tests is an investment in quality, it seems to me that it would be a terrible shame just to say: "OK, I'm just going to drop that test".

I suggest there are two options:

1. if you replace your inner class's direct access to outer class fields with getter/setter methods (which can be private, quite oddly) this will mean that it is the mock's methods which will be used... and so the mock's fields. Your existing tests should then continue to pass.

2. the other possibility is to refactor that inner class to make it a free-standing class, an instance of which replaces your inner class, and transferring one or more test methods to a new test class for this new class. You will then be faced with the (simple) task of rigging things up so that the references to the outer class object are parameterised (i.e. 99% of cases, passed as a constructor parameter) and then can be mocked suitably. This shouldn't be too difficult. Although you may need to add suitable getter/setter methods for private fields in the outer class and/or make one or more of its private methods package-private. From that point on the inner class becomes a "black box" as far as testing the outer class is concerned.

By using either approach you've suffered no loss of quality.

Comments

[phoet]

totally unrelated, use static inner classes if you don't need a reference to the outer class

[Guillaume]

About equals/hashcode: It is more than advisable to override hashcode too, not doing it can break one or the most important contracts of Java, make your class unusable in hashtables, and generate some weird bugs at runtime.

[Paradise]

Several years too late, but I thought I should provide some reasoning for why someone would want the mock: The inner class as it's responsible for some network IO that should not happen in a test environment.

[user239558]

I don't think this answer makes sense. There will be some interaction between an inner and outer class, and it's obvious that that might need testing. Why is this an accepted answer?

[Jeffrey Blattman]

There are a million reasons to mock an inner class in Android.