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.

Thanks.

Answer 1

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.

Answer 2

I would say the "best practice" is to test something of significance with the object that is different now that it stored the entity in the list.

In other words, what behavior is different about the class now that it stored it, and test for that behavior. The storage is an implementation detail.

That being said, it isn't always possible to do that.

You can use reflection if you must.

Answer 3

You could have an internal property which exposes the private variable added for DEBUG compiles.

#if DEBUG
internal IList<object> InsertedItems { get { return this.insertedItems; } }
#endif DEBUG

Another approach is to adopt Microsoft's CodeContracts:

public bool OnSave(object entity, object id)
{
    // either the entity is null OR our insertItems contains
    // the entity in question
    Contract.Ensures(!(entity is IAuditable)
        || insertItems.Contains(entity as IAuditable));

    var auditable = entity as IAuditable;
    if (auditable != null) insertItems.Add(entity);

    return false;            
}

Answer 4

If I'm not mistaken, what you really want to test is that it only adds items to the list when they can be cast to IAuditable. So, you might write a few tests with method names like:

• NotIAuditableIsNotSaved
• IAuditableInstanceIsSaved
• IAuditableSubclassInstanceIsSaved

... and so forth.

The problem is that, as you note, given the code in your question, you can only do this by indirection - only by checking the private insertItems IList<object> member (by reflection or by adding a property for the sole purpose of testing) or injecting the list into the class:

public class ClassToBeTested
{
    private IList _InsertItems = null;

    public ClassToBeTested(IList insertItems) {
      _InsertItems = insertItems;
    }
}

Then, it's simple to test:

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

     List<object> testList = new List<object>();
     myClassToBeTested     = new MyClassToBeTested(testList);

     // ... create audiableObject here, etc.
     myClassToBeTested.OnSave(auditableObject, null);

     // Check auditableObject has been added to testList
}

Injection is the most forward looking and unobtrusive solution unless you have some reason to think the list would be a valuable part of your public interface (in which case adding a property might be superior - and of course property injection is perfectly legit too). You could even retain a no-argument constructor that provides a default implementation (new List()).

It is indeed a good practice; It might strike you as a bit overengineered, given that it's a simple class, but the testability alone is worth it. Then on top of that, if you find another place you want to use the class, that will be icing on the cake, since you won't be limited to using an IList (not that it would take much effort to make the change later).

Answer 5

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, the 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 6

If the list is an internal implementation detail (and it seems to be), then you shouldn't test it.

A good question is, what is the behavior that would be expected if the item was added to the list? This may require another method to trigger it.

    public void TestMyClass()
    {
        MyClass c = new MyClass();
        MyOtherClass other = new MyOtherClass();
        c.Save(other);

        var result = c.Retrieve();
        Assert.IsTrue(result.Contains(other));
    }

In this case, i'm asserting that the correct, externally visible behavior, is that after saving the object, it will be included in the retrieved collection.

If the result is that, in the future, the passed-in object should have a call made to it in certain circumstances, then you might have something like this (please forgive pseudo-API):

    public void TestMyClass()
    {
        MyClass c = new MyClass();
        IThing other = GetMock();
        c.Save(other);

        c.DoSomething();
        other.AssertWasCalled(o => o.SomeMethod());
    }

In both cases, you're testing the externally visible behavior of the class, not the internal implementation.