Test setups and design smells

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:

 

image

 

Look at the Authenticated_Shipping_With_EmtpyCart test. It’s short and straightforward. However, notice how the test class has a TestBase:

 

image

 

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:

image

 

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.

7 thoughts on “Test setups and design smells

  1. Pingback: Too many dependencies? « Hadi Hariri's Blog

  2. uluhonolulu

    Unit or not, I’m for using containers. This way, whenever I change some class’ constructor signature, I don’t have to fix it in all tests that use it. On the other hand, I don’t have a room full of juniors that I have to babysit. As for myself, writing a constructor for a class like that is painful enough, so I don’t need to write a test to make me want to fix it.

    On a side note, whenever I want to reuse some setup code, I prefer to put it in a separate class and use composition instead of inheritance. Stole it from Ayende.

    Reply
  3. Pingback: Dealing with the “Too many dependencies” problem « Hadi Hariri's Blog

  4. Pingback: JUnit for C# Developers 7 – Law of Demeter and Temporal Mocking | DaedTech

  5. Marc Chouteau (@chouteau)

    Hello I am the person who coded these tests.
    I actually agree with the design unit tests it was not good, I modified it with the V4, now execute the test exactly what the website execute.
    The inheritance of a class “TestBase” is no longer an aid to access to the helpers for creating a MockHttpContext
    Thank you for your comments

    Reply

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s