Reviewing some code today, I keep stumbling across private methods that are encapsulating test setups. A probable reason for this is that the authors want to prevent having to repeat the same code over and over again. It seems like common sense. I mean after all, that’s one way of complying with DRY.
The problem however is that often this kind of encapsulation is hiding an underlying design problem.
As an example, I’m going to use one of my all time favorite code-bases that I reference in talks I give on MVC and design in general:
Look at the Authenticated_Shipping_With_EmtpyCart test. It’s short and straightforward. However, notice how the test class has a TestBase:
I chopped off the remaining part as it wouldn’t fit on the screen. You can see the entire class here. It’s just wrong on so many levels.
If you look at the controller this is testing (CheckoutController), you’ll see:
If I were to write that out every time I would need to test the controller, alarm signals would raise that I’m doing something wrong. If I’m hiding it away in a base class or a method, it’s harder to notice.
So next time you feel the urge to encapsulate some setup in a method or base class, ask yourself why. If you don’t come up with a good reason, re-think your design.
Side-note: Some people have lately been complaining that IoC Containers lead to these kind of design smells also, allowing developers to by injection-happy when dealing with dependencies. It might be true, although I believe that if you are correctly writing the necessary tests, you’d notice this problem early on. The code above also uses a container in the tests (and doesn’t even use conventions for setting it up). That’s another reason I’m against using containers in unit tests.