How to unit test a function that is refactored to strategy pattern?

Question

If I have a function in my code that goes like:

class Employee{

    public string calculateTax(string name, int salary)
    {
        switch (name)
        {
            case "Chris":
                doSomething($salary);
            case "David":
                doSomethingDifferent($salary);
            case "Scott":
               doOtherThing($salary);               
       }
}

Normally I would refactor this to use Ploymorphism using a factory class and strategy pattern:

public string calculateTax(string name)
{
    InameHandler nameHandler = NameHandlerFactory::getHandler(name);
    nameHandler->calculateTax($salary);
}

Now if I were using TDD then I would have some tests that work on the original calculateTax() before refactoring.

ex:

calculateTax_givenChrisSalaryBelowThreshold_Expect111(){}    
calculateTax_givenChrisSalaryAboveThreshold_Expect111(){}

calculateTax_givenDavidSalaryBelowThreshold_Expect222(){}   
calculateTax_givenDavidSalaryAboveThreshold_Expect222(){} 

calculateTax_givenScottSalaryBelowThreshold_Expect333(){}
calculateTax_givenScottSalaryAboveThreshold_Expect333(){}

After refactoring I'll have a Factory class NameHandlerFactory and at least 3 implementation of InameHandler.

How should I proceed to refactor my tests? Should I delete the unit test for claculateTax() from EmployeeTests and create a Test class for each implementation of InameHandler?

Should I test the Factory class too?

Answer 1

The old tests are just fine for verifying that calculateTax still works as it should. However, you don't need many test cases for this, only 3 (or maybe some more, if you want to test error handling too, using unexpected values of name).

Each of the individual cases (at the moment implemented in doSomething et al.) must have their own set of tests too, which test the inner details and special cases related to each implementation. In the new setup these tests could / should be converted into direct tests on the respective Strategy class.

I prefer to remove old unit tests only if the code they exercise, and the functionality it implements, completely ceases to exist. Otherwise, the knowledge encoded into these tests is still relevant, only the tests need to be refactored themselves.

Update

There may be some duplication between the tests of calculateTax (let's call them high level tests) and the tests for the individual calculation strategies (low level tests) - it depends on your implementation.

I guess the original implementation of your tests asserts the result of the specific tax calculation, implicitly verifying that the specific calculation strategy was used to produce it. If you keep this schema, you will have duplication indeed. However, as @Kristof hinted, you may implement the high level tests using mocks too, to verify only that the right kind of (mock) strategy was selected and invoked by calculateTax. In this case there will be no duplication between high and low level tests.

So if refactoring the affected tests isn't too costly, I would prefer the latter approach. However, in real life, when doing some massive refactoring, I do tolerate a small amount of test code duplication if it saves me enough time :-)

Should I test the Factory class too?

Again, it depends. Note that the tests of calculateTax effectively test the factory. So if the factory code is a trivial switch block like your code above, these tests may be all you need. But if the factory does some more tricky things, you may want to dedicate some tests specifically for it. It all boils down to how much tests you need to be confident that the code in question really works. If, upon reading the code - or analysing code coverage data - you see untested execution paths, dedicate some more tests to exercise these. Then repeat this until you are fully confident in your code.

Answer 2

I'll start by saying that I'm no expert on TDD or unit testing, but here's how I would test this (I'll use pseudo-like code):

CalculateTaxDelegatesToNameHandler()
{
    INameHandlerFactory fakeNameHandlerFactory = Fake(INameHandlerFactory);
    INameHandler fakeNameHandler = Fake(INameHandler);

    A.Call.To(fakeNameHandlerFactory.getHandler("John")).Returns(fakeNameHandler);

    Employee employee = new Employee(fakeNameHandlerFactory);
    employee.CalculateTax("John");

    Assert.That.WasCalled(fakeNameHandler.calculateTax());
}

So I'd test that the calculateTax() method of employee class correctly asks its NameHandlerFactory for a NameHandler and then calls the calculateTax() method of the returned NameHandler.

Answer 3

Before writing any code, I would start with a test for a Factory. Mocking the stuff I need I would force myself to think about the implementations and usecases.

Than I would implement a Factory and continue with a test for each implementation and finally the implementations themselves for those tests.

Finally I would remove the old tests.

Answer 4

You're taking one class (employee that does everything) and making 3 groups of classes: the factory, the employee (that just contains a strategy) and the strategies.

So make 3 groups of tests:

1. Test the factory in isolation. Does it handle inputs correctly. What happens when you pass in an unknown?
2. Test the employee in isolation. Can you set an arbitrary strategy and it works as you expect? What happens if there is no strategy or factory set? (if that's possible in code)
3. Test the strategies in isolation. Does each perform the strategy you expect? Do they handle odd boundary inputs in a consistent manner?

You can of course make automated tests for the whole shebang, but these are now more like integration tests and should be treated as such.