Practices for refactoring unit tested code

Question

Suppose I have a function like this:

public void AddEntry(Entry entry)
{
    if (entry.Size < 0)
        throw new ArgumentException("Entry size must be greater than zero");
    DB.Entries.Add(entry);
}

And a corresponding unit test:

[TestMethod]
[ExpectedException(typeof(ArgumentException), "Entry size must be greater than zero")]
public void AddEntry_TermSizeLessThanZero_ThrowException()
{
    Entry e = new Entry(-5);

    AddEntry(e);
}

And then I refactor the validation code out:

public void AddEntry(Entry entry)
{
    Validate(entry);
    DB.Entries.Add(entry);
}

public void Validate(Entry entry)
{
    if (entry.Size < 0)
        throw new ArgumentException("Entry size must be greater than zero");
}

The unit test no longer describes the validation code.

What's the best thing to do in this case? Do I just leave Validate() to be tested through AddEntry?

Edit: to clarify, supposing I had a reason to make the refractored code public (a bit contrived in this situation), would I want to duplicate the test code to be thorough?

Answer 1

Since you made the Validate() method a public member of your object (for whatever reason) you should certainly add a seperate test for it. The test for AddEntry should remain in place.

A unit test should only test the interface of a unit and shall not assume any implementation details. So in this case you should leave your test for AddEntry in place the way it is, because it describes the behavior of your interface. But you should not assume or test that AddEntry() calls Validate().

Answer 2

At this point I wouldn't add a test to Validate(), since that code is already hit by an existing test. I like that test since it corresponds closely to the class's requirements.

This changes when you start using Validate() in other functions in the class, or in other classes. Validate() may also be extended to validate different issues, you'll want to address this.

When Validate() has multiple callers, and Validate() starts testing multiple conditions, you'll probably want:

• 1 Validate() test per validation condition
• 1 test per call to Validate
  • you might verify someone calls Validate by passing in one of the failure conditions
    • upside of this approach is less fake abstraction
    • downside is those tests could all become invalid if the Validate() criteria changes
    • consider putting the code generating the invalid input in a test utility method
  • you might also verify someone calls Validate by passing in a mock object for the validator
    • upside of this approach is decoupling of what is validated versus who is doing the validation, making tests more robust versus some changes
    • downside is yet another layer of abstraction making the code more complex

It seems adding tests in this manner keeps good coverage while scaling linearly with the number of requirements tested.

Answer 3

As this code evolves, there are two possibilities:

1. Validate() stays simple and acts as a private helper, of which external callers (including tests) should not be aware. In this case, Validate() should not have its own tests, but instead have its behavior tested through other methods that call it.

2. Validate() becomes complex enough that you have to duplicate a lot of test code to verify that it is called in each place it should be. In this case, I would consider extracting an interface+class to perform the validation. Validate() can then be tested thoroughly in its new home and a fake IValidator (or whatever) can be used in your test of AddEntry, asserting that Validate() is called as necessary.