What to mock and why not to mock data objects?

Recently when reviewing my code before opening a pull request, I have noticed a few methods that I felt were untested. I have run all tests with coverage and indeed 7 methods in the class were not covered. The class was a data object and all these 7 methods were mostly getters and setters so the following questions arose:

• Should I have a unit test for data object?
• Should I be testing getters and setters?
• Why I never had to do this before?

I am in the middle of implementing a larger feature, acceptance tests are still disabled – these should be failing if my getters/setters were incorrect, but it could take some time to find where the actual problem is. I have never been testing getters/setters and they were always covered by tests. I recalled that data objects are tested indirectly by classes that use them. So I have reviewed all usages of this data class and have noticed that all unit tests mock this data.

My data class, let’s call it State, represents data to be saved to the database. Saving to database is done using Reflection API and hence it doesn’t depend on the object – so no work to do here. I had a single converter that was converting this data to another format before returning it through REST endpoint. I had also a few other classes (let’s call one of them Handler) that were using State, however they have been only accessing a specific parts of the State, never everything. As I was changing the internal representation of the State, I added extra methods that expose necessary data only. The class looked like this, there were some others fields as well, but I will skip them for simplicity:

public class State {

    private final DataInfo xsInfo;
    private final DataInfo ysInfo;

    public State(DataInfo xsInfo, DataInfo ysInfo) {
        this.xsInfo = xsInfo;
        this.ysInfo = ysInfo;
    }

    public DataInfo getXsInfo() {
        return xsInfo;
    }

    public DataInfo getYsInfo() {
        return ysInfo;
    }

    public int getXsValue() {
        return xsInfo.getValue();
    }

    public int getYsValue() {
        return ysInfo.getValue();
    }

    public void setXsValue(int value) {
        xsInfo.setValue(value);
    }

    public void setYsValue(int value) {
        ysInfo.setValue(value);
    }

    public boolean hasAnyValue() {
        return xsInfo.getValue() != null || ysInfo.getValue() != null;
    }

}

First two methods are just ordinary getters which were used by the converter. The next two are setters which were used in the other part of the system that was updating the State. The next two methods are “delegating getters”. The last method is a bit arguable whether it is already business logic or not yet – I would say that this is a method operating directly on the data to return the simplified information about it so it’s fine. Now I have the Handler which uses only last method – it handles incoming request, checks the State and does some operations depending on what the request and State are.

If I wanted to test the last method, I would need at least 3 tests – both values not null, first value null and second not, first value not null and second null. Duplicating one of the unit tests for the Handler sounded as bad idea, as these tests have quite large setups. Adding unit tests for State sounded wrong as well – we don’t test getters and setters.

After some analysis and discussion with other team members, we have found that the problem actually was Handler‘s unit tests mocking the State and defining only the return value for hasAnyValue() method. The actual implementation could be return false; and Handler‘s unit test would be green, but it should be testing State as well.

So, I have changed the setup for Handler‘s unit tests, removed mocks and used the constructors, however it looked a bit ugly:

private final State state = new State(new DataInfo(null, null, 15, null, 0, false), new DataInfo(null, null, 17, null, 0, false));

All the default values are that part of the State that Handler does not care about. I am still not happy about how the setup for this test looks like, but now my extra methods on State are tested, I didn’t have to write unit tests for getters/setters on the data object and finally, all 3-4 classes like Handler that are using only part of the State, were not depended on internal representation of this data object. So if someone wanted to change State class, they would have to change only the converter and unit tests’ setups for Handler (I assume constructor would change), but the actual Handler‘s implementation would not be affected.

To sum up:

• avoid mocking data objects (do so only if this is tested somewhere else)
• mock only classes that have unit tests