views:

143

answers:

3

Consider the following class:

public class MyIntSet
{
    private List<int> _list = new List<int>();

    public void Add(int num)
    {
        if (!_list.Contains(num))
            _list.Add(num);
    }

    public bool Contains(int num)
    {
        return _list.Contains(num);
    }
}

Following the "only test one thing" principle, suppose I want to test the "Add" function. Consider the following possibility for such a test:

[TestClass]
public class MyIntSetTests
{
    [TestMethod]
    public void Add_AddOneNumber_SetContainsAddedNumber()
    {
        MyIntSet set = new MyIntSet();
        int num = 0;

        set.Add(num);
        Assert.IsTrue(set.Contains(num));
    }
}

My problem with this solution is that it actually tests 2 methods: Add() and Contains(). Theoretically, there could be a bug in both, that only manifests in scenarios where they are not called one after the other. Of course, Contains() now servers as a thin wrapper for List's Contains() which shouldn't be tested in itself, but what if it changes to something more complex in the future? Perhaps a simple "thin wrap" method should always be kept for testing purposes ?

An alternative approach might suggest mocking out or exposing (possibly using InternalsVisibleTo or PrivateObject) the private _list member and have the test inspect it directly, but that could potentially create test maintainability problems if someday the internal list is replaced by some other collection (maybe C5).

Is there a better way to do this? Are any of my arguments against the above implementations flawed?

Thanks in advance, JC

+6  A: 

Your test seems perfectly OK to me. You may have misunderstood a principle of unit testing.

A single test should (ideally) only test one thing, that is true, but that does not mean that it should test only one method; rather it should only test one behaviour (an invariant, adherence to a certain business rule, etc.) .

Your test tests the behaviour "if you add to a new set, it is no longer empty", which is a single behaviour :-).

To address your other points:

  • Theoretically, there could be a bug in both, that only manifests in scenarios where they are not called one after the other.
    True, but that just means you need more tests :-). For example, add two numbers, then call Contains, or call Contains without Add.

  • An alternative approach might suggest mocking out or exposing (possibly using InternalsVisibleTo) the private _list member and have the test inspect it directly, but that could potentially create test maintainability problems[...]
    Very true, so don't do this. A unit test should always be against the public interface of the unit under test. That's why it's called a unit test, and not a "messing around inside a unit"-test ;-).

sleske
If he ran `set.Contains(num)`, checked the return was `false`, then the `Add()`, followed by the `Assert.IsTrue(Contains(num))` would that adhere to the test one thing principle and remove the need for another separate test?
Rob Allen
@sleske- thanks, upvoted@Rob - That would only test Contains() in an isolated manner from Add() (for the specific scenario where the set is empty). The testing of Add() would still be dependent on Contains().
ohadsc
A: 

There are two possibilities.

  1. You've exposed a flaw in your design. You should carefully consider if the actions that your Add method is executing is clear to the consumer. If you don't want people adding duplicates to the list, why even have a Contains() method? The user is going to be confused when it's not added to the list and no error is thrown. Even worse, they might duplicate the functionality by writing the exact same code before they call .Add() on their list collection. Perhaps it should be removed, and replaced with an indexer? It's not clear from your list class that it's not meant to hold duplicates.

  2. The design is fine, and your public methods should rely on each other. This is normal, and there is no reason you can't test both methods. The more test cases you have, theoretically the better.

As an example, say you have a functions that just calls down into other layers, which may already be unit tested. That doesn't mean you don't write unit tests for the function even if it's simply a wrapper.

womp
It's not a list class, rather a set class, so the meaning should be clear. But that's beside the point since the class merely serves as an example to convey a general unit-testing general questionOther than that I appreciate the input
ohadsc
+1  A: 

In practice, your current test is fine. For something this simple it's very unlikely that bugs in add() and contains() would mutually conspire to hide each other. In cases where you are really concerned about testing add() and add() alone, one solution is to make your _list variable available to your unit test code.

[TestClass]
public void Add_AddOneNumber_SetContainsAddedNumber() {
    MyIntSet set = new MyIntSet();
    set.add(0);
    Assert.IsTrue(set._list.Contains(0));
}

Doing this has two drawbacks. One: it requires access to the private _list variable, which is a little complex in C# (I recommend the reflection technique). Two: it makes your test code dependent on the actual implementation of your Set implementation, which means you'll have to modify the test if you ever change the implementation. I'd never do this for something as simple as a collections class, but in some cases it may be useful.

Nelson
I would always almost recommend against poking into the unit under test. A unit test should test that the unit does what it promises, and that can be done using public methods. It's okay to add public methods for testing, though (such as a Contains method, even if your application does not otherwise need it).
sleske
+1As an aside, the the reflection technique can be implemented via MSTest's PrivateObject: http://msdn.microsoft.com/en-us/library/microsoft.visualstudio.testtools.unittesting.privateobject.aspx
ohadsc