Should you have duplicate unit tests for seperate methods that share the same private implementation?

Question

I am building a business layer for our large enterprise application, and we're currently sitting at just under 500 unit tests. The scenario is that we have two public methods, public AddT(T) and public UpdateT(T) that both make an internal call to private AddOrUpdateT(T) since a lot of the core logic is the same between both, but not all; they are different.

Because they are separate public API (regardless of private implementation), I wrote unit tests for each API, even though they are the same. This might look like

[TestMethod]
public void AddT_HasBehaviorA() 
{
}

[TestMethod]
public void UpdateT_HasBehaviorA() 
{ 
}

Currently for this particular business object there are around 30 unit tests for adding, and 40 unit tests for updating, where 30 of the update tests are the same as the add tests.

Is this normal, or should I be abstracting the common behavior out into a public helper class that is unit tested in isolation, and the two API's just use that helper class instead of a private method that has the implementation?

What is considered a best practice for these situations?

Answer 1

First of all, it's important to understand why you want to cover those methods by unit tests, because that's going to influence the answer. Only you and your team knows this, but if we assume that at least part of the motivation for unit testing is to get a trustworthy regression test suite, you should test the observable behaviour of the System Under Test (SUT).

In other words, unit tests should be black box tests. The tests shouldn't know about the implementation details of the SUT. Thus, the naive conclusion you could draw from this is that if you have duplicate behaviour, you should also have duplicate test code.

However, the more complex your system becomes, and the more it relies on common behaviour and strategies, the harder it becomes to implement this testing strategy. This is because you'll have a combinatorial explosion of possible ways through the system. J.B. Rainsberger explains it better than I do.

A better alternative is often to listen to your tests (a concept popularized by GOOS). In this case, it sounds like it would be valuable to extract the common behaviour into a public method. This, however, doesn't itself solve the problem of combinatorial explosion. While you can now test the common behaviour in isolation, you also need to prove that the two original methods (Add and Update) use the new public method (instead of, e.g., some copy-and-pasted code).

The best way to do that is to compose the methods with the new Strategy:

public class Host<T>
{
    private readonly IHelper<T> helper;

    public Host(IHelper<T> helper)
    {
        this.helper = helper;
    }

    public void Add(T item)
    {
        // Do something
        this.helper.AddOrUpdate(item);
        // Do something else
    }

    public void Update(T item)
    {
        // Do something
        this.helper.AddOrUpdate(item);
        // Do something else
    }
}

(Obviously, you'll need to give the types and methods better names.)

This enables you to use Behaviour Verification to prove that the Add and Update methods correctly use the AddOrUpdate method:

[TestMethod]
public void AddT_HasBehaviorA() 
{
    var mock = new Mock<IHelper<object>>();
    var sut = new Host<object>(mock.Object);
    var item = new object();

    sut.Add(item);

    mock.Verify(h => h.AddOrUpdate(item));
}

[TestMethod]
public void UpdateT_HasBehaviorA() 
{ 
    var mock = new Mock<IHelper<object>>();
    var sut = new Host<object>(mock.Object);
    var item = new object();

    sut.Update(item);

    mock.Verify(h => h.AddOrUpdate(item));
}

Answer 2

It is a good practice to avoid duplication as much as possible. This aids code readability and maintainability (and probably other *ablilities ;-)). It also makes testing easier as you can start to test the common functionality in one place and not have to have so many duplicate tests. Also, duplication in tests is bad as well.

Another best practice is to only write unit tests for functionality that has unique logic. If in your Add and Update methods, you are only calling another method, there is no need to write unit tests at that level, you should be focusing on the called method.

Which brings up back to the initial point of not duplicating code and if you have private methods that share duplicate code, it may be a good idea to break it out into something else that you can run tests against.

Answer 3

"Since a lot of the core logic is the same between both, but not all; they are different."

I think you should have separate unit tests for these, because as you say they are not exactly the same. Also, what if you later change the implementation to call two different methods? Then your unit tests will only be testing one of them because they are coupled to the implementation detail of the two methods. The tests will pass, but you may have introduced a bug.

I think a better approach is to test both methods but add helper methods/classes to do the common work.