Tuesday, 7 June 2011

My Top Ten TDD Code Smells

I'm setting up an online training course for test driven development, and as part of the preparation I've been formalising my thinking around TDD. The easiest way to do this for me is to think about all the things I've seen, and done, in the past that have caused problems with the TDD process.

I know there are loads more, and I'd love to hear your top ten, but these are my major teeth grinders!

Not writing the tests first!
Sounds obvious right, test driven development means driving the code through tests, but it's surprising how often this goes out of the window even with experienced people. Honestly though it's entirely understandable. Almost every other item in this list (spoiler alert) is just a code design issue that can be applied to production code as well as test code. Writing the tests first is a real mental shift that can take years to engrain. This seems especially hard if you've been hacking away in semi-productive bliss with the adrenaline fuelled highs of squashing bugs late into the night to meet a release deadline for the past however many years.

The sobering fact is if you want to get the main benefits of TDD: clean design, simple code, safe refactoring, minimal code, you need to write the test first. By forcing yourself to write the tests first you are always thinking of your code from the client classes perspective, which leads to cleaner interfaces. You are also focusing on the concrete requirements of any calling code. Writing the test first helps you drive out a specification of exactly what you need to implement. This is a great antidote to developer gold plating.

Always thinking about your code from the perspective of a client class first will help when trying to write the tests first.

Only test the happy path
You can make a surprising amount of progress very quickly if you don't worry about edge cases! If you are paying for the support costs, then good luck to you. Out here in the real world, professional software developers know that we need to think about the exceptional cases where an API we call might return null, or what happens if a user enters a negative number, etc. With a test driven approach there is no excuse for not exercising these paths in your code. Automated unit tests make it hundreds of times faster to check these conditions than the old way of writing the code then running it to check the result manually. 

Use the power of test driven development to build confidence in the robustness of your code from the very start.

Meaningless test names
The tests we write are supposed to be a living form of documentation of the system. You should be able to understand the behaviours of any class by going directly to it's associated test class and viewing the method names found there. Therefore, tests that are named 'testTheMethodRunsOK' are less than desirable. 

Everyone has their own style of naming tests, that's fine, just make sure they are legible.

Testing test code
I've seen two flavours of this. One is a waste of time, the other is just plain ridiculous. The common scenario for the first occurs after a number of support classes have been built to make the tests easier to write. This is a good thing. However, what comes next is not so hot. The test support classes get so complicated that someone suggests writing tests for the test support classes. This is a big no no. If you have test support classes that are so hard to understand that you need to write tests for them, it's time to simplify.

The other scenario is utterly incredible and I have only seen it happen twice. Tests that test production logic that has been duplicated inline in the test. Let me try and explain. Imagine a method that takes two numbers, performs a calculation and then returns the result. Now imagine a test in the associated test class that defines the two inputs and has the calculation logic copied from the production code into the body of the test and then checks the result of the calculation is as expected. What happens when the calculation in the production code changes? Of course the tests still pass, because they are testing the duplicated version of the logic hard coded in the test! 

Make sure your tests are exercising production code, or don't bother writing them.

Mocking madness
Mocking is a great tool for isolating the code under test, giving you confidence that your test is only exercising the code that you really want to test and freeing you from side effects. A common use case is to create a mock instance of a collaborating class and inject it into your class under test allowing you to control the behaviour of the collaborator to create the correct conditions for your test. 

You know you are in the realms of mocking madness when test after test, the first few lines of your test is dedicated to setting up mock behaviours, just so you can get your test to run. With the explosion in popularity of dependancy injection frameworks like Spring it becomes more and more common for multiple dependencies to be injected into a class creating spider web like dependencies between collaborators. When this happens it's an obvious loose coupling violation. The good news is that TDD exposes this issue very early on as you get fed up of constantly tweaking dependant mock objects. Just make sure you address the underlying coupling issue before you either throw your computer out of the window, or worse still, throw your tests out of the window!

Refactor early to make your tests small and easy to write. Look for commonly paired collaborators and either combine or encapsulate them with composition.

Big fat mothers
This smell goes hand in hand with my deep mistrust of inheritance but for some reason it seems so much easier to go super class crazy in test code than it is in production code. Perhaps it's due to programmers putting less thought into the design of their test code (more on that later). 

The first whiff of this smell comes when you see all of the test classes extending a base class. Are all your tests really testing the same thing? Do they all need the same set of helper functions? This makes me nervous but I can live with it so long as the super class is kept to a minimum, but the problem with super classes is they are sneaky. They are very good at two things. They love to suck up more responsibility than is good for them and they are very good at spawning offspring. Before you know where you are you'll be struggling with a set of unwieldy base classes full of methods that look really useful, but never seem to pay their way when the next test class comes along. Somewhere in this tragic story, a bright spark will suggest putting setUp and tearDown methods in the base classes and now you are really in for a world of hurt trying to figure out what exactly the state of your test is before it starts.

Keep an eye out for complex class hierarchy structures in your tests. Start building test helper utilities through composition and only create simple super classes to encapsulate common use cases when they become painfully obvious.

The Über test
A test method is supposed to test one thing and one thing only. The über test laughs at such restrictive pedagogy. Why test one thing when you can test ten! 

I'll admit it is tempting, in the name of getting things done, to roll a number of related tests up into one so you don't have to duplicate the setup code. Unfortunately, as with many shortcuts, the problems start coming later. As you refactor you'll find that it takes longer and longer to maintain these tests if the underlying code changes. What tends to happen is that the first assertion fails, and so the test fails. You fix the first problem, and then the next assertion fails, and so on and so on. These tests also end up being horribly complex and hard to follow as you cram more and more into them. Not only are there many assertions to fail, but the purpose of the test becomes unclear and you are unsure which parts are still relevant and which parts can be removed. Once these sorts of questions start bubbling up, you can bet it's only a short while before you either spend a chunk of time rewriting the tests so you can understand them, or even worse, binning them.  

If you can keep tests small and focused on expressing one intention they will be far easier to maintain and will be much more useful as a form of living documentation in the future.

Cling film tests
When was the last time you used cling film in the kitchen? You pull it out from the roll, rip it off to put over a bowl and you end up with it tangled all round your hand and up your arm. Five minutes later you are still trying to unravel it. There are tests like this. There are a lot of tests like this. You'll notice them when you try and change a commonly used interface in your production code and you spend a disproportionate amount of time wading through your tests making them compile because of the interface change. Test driven development is supposed to make it easy to refactor, but when you don't isolate the touch points between your code and tests you are going to start having problems. 

If you have ever experienced this, and I find it hard to believe that you haven't, the next time you are in your IDE do a search to find usages of the public interface of a core class in your system. You will probably find ten or more times as many uses of the interface in your test code than in your production code. If not you should either be ashamed (you don't have enough coverage) or be feeling very smug (your test code is isolated from your production code and refactoring will be easy).

Try to make sure you encapsulate calls from test code to production code to make refactoring easier as the design of your system evolves. 

Spaghetti test data
It's common for test data, i.e. strings, numbers etc. to be hard coded in tests as input and then compared as output. Fine, no problem. It's good to have the context that data provides close to the test for clarity. What you need to avoid though is duplicating this information within tests, and even worse, duplicating the data between test data files and your test code. The number of times I have seen tests that define an input string when creating an object to pass into a test, and then the same string hardcoded in the expectation of the test is mind boggling. If either of these string definitions changes independently of the other then the test will fail, for no good reason! We don't put up with magic numbers and static strings in our production code, it should not be tolerated in our test code. 

Use constants, or at least variables to reference test data that you will use more than once in a test.

Not refactoring
Red, green, refactor is a common mantra in TDD. Red: write the test first and see it fail. Green: make the test pass. Refactor: given what you have learned, do you need to refactor the code. We often get so caught up in the rhythm of writing the tests and seeing the green bar when they pass that we forget about the refactoring step. Take the time to refactor as you go. It will improve the design of your code and make your tests easier to change. 

If you are finding tests hard to manage while refactoring, they will be much easier to improve while the context of the task is fresh in your head. 

Treating test code as a second class citizen
If you're keeping count you'll notice this is number 11. It's a freebie, a bonus. It is also quite possibly the most important item in this list. There is a common attitude among developers that test code is somehow less important than production code. I am always hearing statements like "I'ts only test code". This attitude is used to excuse poor design, hacks and generally illiterate code in tests. What's more important, the code that proves that your system works or the code that makes your system work? How many lines of production code is in your project? How many lines of test code? If you're practicing TDD I'd guess the amount of test code far outweighs the production code. We've already seen that each public interface is exercised many times more by test code than production code. 

Almost all of the smells I've listed are really just code design issues. Each one of them will reduce your teams productivity and slow development progress down to a crawl. As your tests grow out of control they will smother your production code and start sapping value from the project, rather than providing the benefits of security and freedom to refactor. If you want to keep your project moving at steady pace over time you are going to have to take the design of your test code seriously and start treating it like a first class citizen. 

Apply all the coding and design techniques that you reserve for your production code to your test code to make your tests easier to maintain.

What's next?
I've identified these smells from my own experience and am guilty of falling into many of the traps myself, but we learn by doing. Hopefully this list will provide some context for your design decisions in the future when writing test code. I'd love to hear any TDD code smells you have come across. I'm always looking for more early warning signals. 

Join my online training course to find out how to address some of these issues. 

0 comments:

Post a Comment