Does it make sense to write tests for legacy code when there is no time for a complete refactoring?

Question

I usually try to follow the advice of the book Working Effectively with Legacy Code. I break dependencies, move parts of the code to @VisibleForTesting public static methods and to new classes to make the code (or at least some part of it) testable. And I write tests to make sure that I don't break anything when I'm modifying or adding new functions.

A colleague says that I shouldn't do this. His reasoning:

• The original code might not work properly in the first place. And writing tests for it makes future fixes and modifications harder since devs have to understand and modify the tests too.
• If it's GUI code with some logic (~12 lines, 2-3 if/else block, for example), a test isn't worth the trouble since the code is too trivial to begin with.
• Similar bad patterns could exist in other parts of the codebase, too (which I haven't seen yet, I'm rather new); it will be easier to clean them all up in one big refactoring. Extracting out logic could undermine this future possibility.

Should I avoid extracting out testable parts and writing tests if we don't have time for complete refactoring? Is there any disadvantage to this that I should consider?

Answer 1

Here's my personal unscientific impression: all three reasons sound like widespread but false cognitive illusions.

1. Sure, the existing code might be wrong. It might also be right. Since the application as a whole seems to have value to you (otherwise you'd simply discard it), in the absence of more specific information you should assume that it is predominantly right. "Writing tests makes things harder because there's more code involved overall" is a simplistic, and very wrong, attitude.
2. By all means expend your refactoring, testing and improvement efforts in the places where they add the most value with the least effort. Value-formatting GUI subroutines are often not the first priority. But not testing something because "it's simple" is also a very wrong attitude. Virtually all severe errors are committed because people thought they understood something better than they actually did.
3. "We will do it all in one big swoop in the future" is a nice thought. Usually the big swoop stays firmly in the future, while in the present nothing happens. Me, I'm firmly of the "slow and steady wins the race" conviction.

Answer 2

A few thoughts :

When you're refactoring legacy code, it doesn't matter if some of the tests you write happen to contradict ideal specifications. What matters is that they test the program's current behavior. Refactoring is about taking tiny iso-functional steps to make the code cleaner; you don't want to engage in bugfixing while you're refactoring. Besides, if you spot a blatant bug, it won't be lost. You can always write a regression test for it and temporarily disable it, or insert a bugfix task in your backlog for later. One thing at a time.

I'd agree that pure GUI code is hard to test and perhaps not a good fit for "Working Effectively ..." -style refactoring. However, this doesn't mean you shouldn't extract behavior that has nothing to do in the GUI layer and test the extracted code. And "12 lines, 2-3 if/else block" is not trivial. All code with at least a bit of conditional logic should be tested.

In my experience, big refactorings are not easy and they rarely work. If you don't set yourself precise, tiny goals, there's a high risk that you embark on a never-ending, hair-pulling rework where you'll never land on your feet in the end. The bigger the change, the more you risk breaking something and the more trouble you will have finding out where you failed.

Making things progressively better with ad hoc small refactorings isn't "undermining future possibilities", it's enabling them - solidifying the swampy ground where your application lies. You should definitely do it.

Answer 3

Also re: "The original code might not work properly" - that doesn't mean you just change the code's behaviour without worrying about the impact. Other code may rely on what appears to be broken behaviour, or side-effects of the current implementation. Test coverage of the existing application ought to make it easier to refactor later, because it'll help you find out when you've accidentally broken something. You should test the most important parts first.

Answer 4

Kilian's answer covers the most important aspects, but I want to expand on points 1 and 3.

If a developer wants to change (refactor, extend, debug) code, she has to understand it. She has to make sure her changes affect exactly the behavior she wants (nothing in the case of refactoring), and nothing else.

If there are tests, then she has to understand the tests as well, sure. At the same time, the tests should help her understand the main code, and tests are far easier to understand than functional code anyway (unless they're bad tests). And the tests help show what changed in the behavior of the old code. Even if the original code is wrong, and the test tests for that wrong behavior, that is still an advantage.

However, this requires that the tests are documented as testing preexisting behavior, not a specification.

Some thoughts on point 3, too: in addition to the fact that the "big swoop" rarely ever actually happens, there's also another thing: it isn't actually easier. To be easier, several conditions would have to apply:

• The antipattern to be refactored needs to be easily found. Are all your singletons named XYZSingleton? Is their instance getter always called getInstance()? And how do you find your overly deep hierarchies? How do you search for your god objects? These require code metrics analysis and then manually inspecting the metrics. Or you just stumble over them as you work, as you did.
• The refactoring needs to be mechanical. In most cases, the hard part of refactoring is understanding the existing code well enough to know how to change it. Singletons again: if the singleton is gone, how do you get the required information to its users? It often means understanding the local callgraph so that you know where to get the information from. Now what is easier: searching out the ten singletons in your app, understanding the uses of each (which leads to needing to understand 60% of the codebase), and ripping them out? Or taking the code that you already understand (because you're working on it right now) and ripping the singletons being used there out? If the refactoring isn't so mechanical that it requires little to no knowledge of the surrounding code, there's not use in bunching it up.
• The refactoring needs to be automated. This is somewhat opinion-based, but here goes. A bit of refactoring is fun and satisfying. A lot of refactoring is tedious and boring. Leaving the piece of code you just worked on in a better state gives you a nice, warm feeling, before you move on to more interesting stuff. Trying to refactor an entire code base will leave you frustrated and angry at the idiot programmers who wrote it. If you want to do a big swoop refactoring, then it needs to be largely automated so as to minimize the frustration. This is, in a way, a meld of the first two points: you can only automate the refactoring if you can automate finding the bad code (i.e. easily found), and automate changing it (i.e. mechanical).
• Gradual improvement makes for a better business case. The big swoop refactoring is incredibly disruptive. If you refactor a piece of code, you invariably get into merge conflicts with other people working on it, because you just split the method they were changing into five parts. When you refactor a reasonably-sized piece of code, you get conflicts with a few people (1-2 when splitting up the 600-line megafunction, 2-4 when breaking up the god object, 5 when ripping out the singleton from a module), but you would have had those conflicts anyway because of your main edits. When you do a codebase-wide refactoring, you conflict with everyone. Not to mention that it ties up a few developers for days. Gradual improvement causes each code modification to take a little longer. This makes it more predictable, and there isn't such a visible time period when nothing happens except cleanup.

Answer 5

There is a culture in some companies where they're reticent to allow developers any time to enhance code that doesn't directly deliver additional value e.g. new functionality.

I'm probably preaching to the converted here, but that is clearly false economy. Clean and concise code benefits subsequent developers. It is just that the payback isn't immediately evident.

I personally subscribe to the Boy Scout Principle but others (as you've seen) don't.

That said, software suffers from entropy and builds up technical debt. Previous developers short on time (or perhaps just lazy or inexperienced) may have implemented sub-optimal buggy solutions over well designed ones. Whilst it may seem desirable to refactor these, you risk introducing new bugs to what is (to the users anyway) working code.

Some changes are lower risk than others. For example, where I work there tends to be a lot of duplicated code that can safely be farmed out to a subroutine with minimal impact.

Ultimately, you have to make a judgement call as to how far you take the refactoring but there is undeniably value in adding automated tests if they don't exist already.

Answer 6

In my experience a characterisation test of some sort works well. It gives you a broad but not very specific test coverage relatively quickly, but can be tricky to implement for GUI applications.

I would then write unit tests for parts that you want to change and do so every time you want to make a change thereby increasing your unit test coverage over time.

This approach gives you a good idea if changes are affecting other parts of the system and let's you get into a position to make any required changes sooner.

Answer 7

Well, yes. Answering as a software test engineer. Firstly you should test everything you ever do anyway. Because if you don't, you don't know whether it works or not. This may seem obvious to us but I have colleagues who see it differently. Even if your project is a little one that may never be delivered, you have to look the user in the face and say you know it works because you tested it.

Non-trivial code always contains bugs (quoting a guy from uni; and if there are no bugs in it, it's trivial) and our job is to find them before the customer does. Legacy code has legacy bugs. If the original code doesn't work the way it should, you want to know about it, believe me. Bugs are ok if you know about them, don't be afraid to find them, that's what release notes are for.

If I remember rightly the Refactoring book says to test constantly anyway., so it's part of the process.

Answer 8

Re: "The original code might not work properly":

Tests are not written in stone. They can be changed. And if you tested for a feature that was wrong, it should be easy to rewrite the test more correctly. Only the expected result of the tested function should have changed, after all.

Answer 9

Do the automated test coverage.

Beware wishful thinking, both your own and by your customers and bosses. Much as I would love to believe that my changes will be correct the first time and I'll only have to test once, I've learned to treat that kind of thinking the same way I treat Nigerian scam emails. Well, mostly; I've never gone for a scam email but recently (when yelled at) I gave in on not using best practices. It was a painful experience that dragged (expensively) on and on. Never again!

I have a favorite quote from the Freefall web comic: "Have you ever worked in a complex field where the supervisor has only a rough idea of the technical details? ... Then you know the surest way to cause your supervisor to fail is to follow his every order without question."

It's probably appropriate to limit the amount of time you invest.

Answer 10

If you're dealing with large amounts of legacy code that isn't currently under test, getting test coverage now instead of waiting for a hypothetical big rewrite in the future is the right move. Starting by writing unit tests is not.

Without automated testing, after making any changes to the code you need to do some manual end to end testing of the app to make sure it's working. Start by writing high level integration tests to replace that. If your app reads files in, validates them, processes the data in some fashion, and displays the results you want tests that capture all of that.

Ideally you'll either have data from a manual test plan or be able to get a sample of actual production data to use. If not, since the app's in production, in most cases it's doing what it should be, so just make up data that will hit all the high points and assume the output is correct for now. It's no worse than taking a small function, assuming it's doing what it's name or any comments suggest it should be doing, and writing tests assuming it's working correctly.

IntegrationTestCase1()
{
    var input = ReadDataFile("path\to\test\data\case1in.ext");
    bool validInput = ValidateData(input);
    Assert.IsTrue(validInput);

    var processedData = ProcessData(input);
    Assert.AreEqual(0, processedData.Errors.Count);

    bool writeError = WriteFile(processedData, "temp\file.ext");
    Assert.IsFalse(writeError);

    bool filesAreEqual = CompareFiles("temp\file.ext", "path\to\test\data\case1out.ext");
    Assert.IsTrue(filesAreEqual);
}

Once you've got enough of these high level tests written to capture the apps normal operation and most common error cases the amount of time you'll need to spend pounding on the keyboard to try and catch errors from the code doing something other than what you thought it was supposed to do will go down significantly making future refactoring (or even a big rewrite) much easier.

As you're able to expand unit test coverage you can pare down or even retire most of the integration tests. If your app's reading/writing files or accessing a DB, testing those parts in isolation and either mocking them out or having your tests begin by creating the data structures read from the file/database are an obvious place to start. Actually creating that testing infrastructure will take a lot longer than writing a set of quick and dirty tests; and every time you run a 2 minute set of integration tests instead of spending 30 minutes manually testing a fraction of what the integration tests covered you're already making a big win.