Unit testing and checking private variable value

Question

I am writing unit tests with C#, NUnit and Rhino Mocks. Here are the relevant parts of a class I am testing:

public class ClassToBeTested
{
    private IList<object> insertItems = new List<object>();

    public bool OnSave(object entity, object id)
    {
        var auditable = entity as IAuditable;
        if (auditable != null) insertItems.Add(entity);

        return false;            
    }
}

I want to test the values in insertItems after a call to OnSave:

[Test]
public void OnSave_Adds_Object_To_InsertItems_Array()
{
     Setup();

     myClassToBeTested.OnSave(auditableObject, null);

     // Check auditableObject has been added to insertItems array            
}

What is the best practice for this? I have considered adding insertItems as a Property with a public get, or injecting a List into ClassToBeTested, but not sure I should be modifying the code for purposes of testing.

I have read many posts on testing private methods and refactoring, but this is such a simple class I wondered what is the best option.

Answer 1

The quick answer is that you should never, ever access non-public members from your unit tests. It totally defies the purpose of having a test suite, since it locks you into internal implementation details that you may not want to keep that way.

The longer answer relates to what to do then? In this case, it is important to understand why the implementation is as it is (this is why TDD is so powerful, because we use the tests to specify the expected behavior, but I get the feeling that you are not using TDD).

In your case, the first question that comes to mind is: "Why are the IAuditable objects added to the internal list?" or, put differently, "What is the expected externally visible outcome of this implementation?" Depending on the answer to those questions, that's what you need to test.

If you add the IAuditable objects to your internal list because you later want to write them to an audit log (just a wild guess), then invoke the method that writes the log and verify that the expected data was written.

If you add the IAuditable object to your internal list because you want to amass evidence against some kind of later Constraint, then try to test that.

If you added the code for no measurable reason, then delete it again :)

The important part is that it is very beneficial to test behavior instead of implementation. It is also a more robust and maintainable form of testing.

Don't be afraid to modify your System Under Test (SUT) to be more testable. As long as your additions make sense in your domain and follow object-oriented best practices, there are no problems - you would just be following the Open/Closed Principle.

Answer 2

You shouldn't be checking the list where the item was added. If you do that, you'll be writing a unit test for the Add method on the list, and not a test for your code. Just check the return value of OnSave; that's really all you want to test.

If you're really concerned about the Add, mock it out of the equation.

Edit:

@TonE: After reading your comments I'd say you may want to change your current OnSave method to let you know about failures. You may choose to throw an exception if the cast fails, etc. You could then write a unit test that expects and exception, and one that doesn't.