10

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?

Improve this question

5 Answers 5

Reset to default
6

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.

Improve this answer
2
  • I modified the code a little to make it closer to my actual practical code. Now a second input salary to the function calculateTax() was added. This way I think I'll be duplicating the test code for the original function and the 3 implementations of the strategy class. Commented Aug 27, 2012 at 10:28
  • @Songo, please see my update. Commented Aug 27, 2012 at 11:58
5

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.

Improve this answer
2
  • hmmmm so you mean I should make the test a behavioral test instead (testing that certain functions were called) and make the value assertions on the delegated classes? Commented Aug 27, 2012 at 10:15
  • Yes, that's what I would do. I would indeed write seperate tests for the NameHandlerFactory and the NameHandler. When you have those, there's no reason to test their functionality again in the Employee.calculateTax() method. That way you don't need to add extra Employee-tests when you introduce a new NameHandler. Commented Aug 27, 2012 at 11:22
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.

Improve this answer
2

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.

Improve this answer
2

My opinion is that you should do nothing, meaning you should not add any new tests.

I stress that this is an opinion, and it actually depends on the way you perceive the expectations from the object. Do you think the user of the class would like to supply a strategy for tax calculation? If he doesn't care, then the tests should reflect that, and the behaviour reflected from the unit tests should be that they should not care that the class has started to use a strategy object to calculate tax.

I actually ran into this problem several times when using TDD. I think the main reason is that a strategy object is not a natural dependency, as opposed to say an architectural boundary dependency like an external resource (a file, a DB, a remote service, etc.). Since it is not a natural dependency, I usually don't base the behaviour of my class on this strategy. My instinct is that I should only change my tests if the expectations from my class have changed.

There's a great post from Uncle Bob, that talks exactly about this problem when using TDD.

I think that the tendency to test each separate class is what's killing TDD. The whole beauty of TDD is that you use tests to spur up design schemes and not vice versa.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.