Using tests to help refactor a legacy codebase

I have been tidying up a legacy code base recently. Its actually my own code, and there’s plenty to be done. Of course it was written under hard time pressure and I was working on my own so the quality has suffered. My discipline was not at the level at which I would apply to a commercial project. Which is an interesting learning in itself.

Here are some (quite lengthy) notes about the process I went through to clean up this code.

The first thing I noticed about the codebase was that although I had a build file which did lots of cool things, it didn’t run the tests automatically.

Step One – re-factor the build script

I wanted to tidy up the build script. It was quite long and hard to understand. Also I want to distribute this project on Google code to people and so I wanted to make it possible for people to simply checkout the code, and type “go” to get a distributable package built.

One of the issues with this is the dependant libraries. So I introduced Ivy to manage this for me (other posts to come on this topic).

Secondly I wanted to compile and run source code, unit tests functional tests (i.e. tests which may use external resources and involve a set of objects working together) and hopefully acceptance tests.

In ant, doing all this can require quite a bit of duplication. So I broke out some macrodefs to handle compilation and execution (again I’ll post this as a separate item hopefully).

Ok so the build looks sweet. When I finally run it and get it to open the junit reports in a browser for me, I see heaps of errors and failures! oh dear. I obviously hadn’t been running the tests every time I checked in. Bad developer!

So, step one with the legacy code base is to get all the tests runnable from the command line ON THE DEV BOX (I have seen a project where this was not simple to do and so the devs relied on the “CI” build to do this for them. not fast enough feedback).

I would like to get a CI build set up for it, but not sure where I’m going to host it. I think that the precedence should be FIRST dev build, THEN CI. If you cant get feedback on your dev box you are asking for integration problems.

Step Two – Make the tests green

Most tests were easy to fix. There were several types of failure / error:

1) Error because of missing resources. Simple to fix, just needed to copy them onto the classpath.

2) Changed functionality where test not updated. Often I had changed the underlying code (e.g. formatting a value) and not updated the tests. This is mostly caused by not running ALL the tests every time a change is made (see kent becks’ JUnit Max

3) Incomprehensible failure – Ah. One of the tests had such a complex set-up that I just couldn’t remember what the hell it was supposed to be testing. Well, I could see what it was trying to test, but the scenario was so complex that it was hard to work out why it was failing.

Solution ? Rip it out. This might seem a little drastic. Infact in one case I simplified it greatly but in a couple of other places I simply removed the test.

My rationale for this is that a failing test that I cannot fix is providing NO VALUE. Infact it is probably providing NEGATIVE value and actually creating NOISE.

Besides, its always in version control if I REALLY want to get it back later.

Step Three – Remove compilation warnings

My project was written in Java 1.4 I was using “enum” all over the shop as a variable name, which is obviously not cool in 1.5. So I removed all the warnings. Again, Warnings are NOISE, adding accidental complexity to my codebase. A health warning here. I once worked on a codebase where development was done on a branch. Going round removing all the warnings everywhere on a branch can seriously affect your merge because there are heaps of small changes which can lead to conflicts. Although relatively safe in terms of refactoring the cost in terms of merge hell is probably not worth it.

I did not remove deprecations yet. Thats another step.

Step Four – Refactor the tests

This is the interesting one and the one that prompted this post.

When running the unit tests, I saw that a few of them were taking quite a long time. These tended to be of the kind which were testing random number generators. To do this, they were generating say 100,000 random numbers in a Gaussian distribution and then analysing the standard deviation of the results.

Also I noticed that some of the tests had many test methods all testing different functionality (for example I had a “Math” class which does heaps of things).

Firstly I created a new source folder for “Functional Tests” to put these longer running, more high level tests in. As I was adding the new Test Cases, I realised that what I was doing was re-organising the functional areas of the tests. For example I extracted a class called “GaussianRandomGeneratorTest” which used to be a single method on my uber-test case.

I realised that what I was doing was Refactoring the Test Cases to represent a new, structure, without refactoring any of the source. Interesting. This means that what I am doing is 100% safe, there is NO WAY I can introduce a bug into the code because I am not mutating it. And yet I am building up a new understanding and description of the code which can point the way towards how the code might look one day.

This is potentially a powerful technique. I realise that I have used the same technique on a previous legacy source base, without really conceptualising it. We extracted all the tests that worked for the area we wanted to change, and then re-structured some of them to make more sense.

In summary, refactor the tests! Its 100% safe and can give you the freedom to experiment with levels of abstraction and cohesion without risking any changes to the code.

Step Five – Remove unused code

This one is quite hard. But luckily in my case I had used deprecation to mark classes I no longer needed. So this stage will involve removing all deprecated classes. Note that I have the luxury to do this as no-one depends on my project. At a minimum I should make sure that nothing inside the codebase uses these deprecated classes.

Share

Test Naming

I was just naming some test cases.

I had a test called :

testShouldReturnInvalidSubmissionIfACompulsoryQuestionIsNotAnswered

I didnt like the way it had Return in it. THis is not very behavioural, it sounds like some implementation detail.

We have been using the pattern testShould a lot which i like, but here i thought, maybe i can call it:

testSubmissionShouldBeInvalidIfACompulsoryQuestionIsNotAnswered

Which breaks the pattern but looks ok. So i started renaming all the methods in the test. Halfway through i thought “what extra information is the ‘Submission’ word adding to my names ? And realised that actually my test was testing a class which was a submission validator and was returning a submission! do i really need to mention it all the time ? No!

so now its :

testShouldBeInvalidIfACompulsoryQuestionIsNotAnswered

all the tests are now named nicely and more concisely.

Share

Easy? Mock

Just writing a test case for a method that is a effectively a controller, it retrieves data from one class and uses another class to convert it to a different kind of object.

Easy mock makes it hard to test each interaction in isolation.

also anyway its a pain.

See here:

http://code.google.com/p/mockito/

for a better way!

nice1 Szczepan

Share