Add Tests to Legacy Code — Hands-On Software Testing, Part 10
Updated: 2 hours ago
Illustrative composite: Tolvern Freight's rate engine — an eleven-year-old surcharge calculator nobody wants to touch and everybody needs to change.
Before you start
You need:
A specific reason to change this code soon. Legacy code you aren't going to touch doesn't need tests; it needs leaving alone. This is the filter that stops the exercise becoming infinite.
The ability to call the function at all from a test. If you can't instantiate it, Step 2 is the whole tutorial and may take a day.
Version control. You'll be running the code repeatedly to find out what it does.
Permission to leave a known bug in place for now. Step 5 depends on it.
About 90 minutes for a first function. Examples are Python.
What you'll build
A characterization suite around calculate_surcharge() that locks in current behaviour — including one output everyone agrees is wrong — so the function can be refactored with the diff visible.
Step 1: Pick by upcoming change, not by ugliness (5 min)
Tolvern's rate engine has forty untested functions. Thirty-nine of them are fine untested, because nothing is going to touch them this quarter.
The one that matters is calculate_surcharge(), because a fuel-surcharge rule change lands next month and nobody can predict what else it will move.
Check: name the change you're about to make to this code. If you can't, stop — you're about to spend ninety minutes on coverage nobody will use.
Step 2: Find the seam before writing any assertion (15 min)
Most legacy code can't be tested because it can't be reached: it reads a global config at import, opens its own database connection, or calls the clock.
You need a seam — a place to substitute a dependency without rewriting the logic. In order of preference:
# Before — unreachable: builds its own dependencies
def calculate_surcharge(shipment_id):
conn = db.connect(settings.DSN) # global config
rate = conn.query("SELECT ...")
today = datetime.now() # the clock
...
# After — same logic, parameters added with defaults. Callers unchanged.
def calculate_surcharge(shipment_id, conn=None, today=None):
conn = conn or db.connect(settings.DSN)
today = today or datetime.now()
...
That change is deliberately the smallest one that makes the function callable. Do not refactor while you have no tests. Adding a defaulted parameter is safe in a way that restructuring is not.
Check: call the function from a test with your own connection and a fixed date. If it still reads a global anywhere, find the next seam. You cannot proceed until this works.
Step 3: Ask the code what it does (20 min)
A characterization test records current behaviour. You are not deciding what's correct — you're taking a photograph.
The fastest way to learn the current output is to assert something obviously wrong and read the failure:
def test_surcharge_standard_10kg():
result = calculate_surcharge(shipment(weight=10, tier="standard"), conn=conn, today=JAN_1)
assert result == "TBD" # placeholder
E AssertionError: assert Decimal('4.20') == 'TBD'
Now write down what it actually does:
def test_surcharge_standard_10kg():
result = calculate_surcharge(shipment(weight=10, tier="standard"), conn=conn, today=JAN_1)
assert result == Decimal("4.20")
Note the fixed date. Any function that reads the clock will produce different output next Tuesday, and a characterization suite that drifts is worse than none.
Check: run the suite twice on different days — or fake the clock forward a month. Same results both times, or you have an unpinned dependency.
Step 4: Cover the branches you're about to disturb (20 min)
Not all of them. The ones near your change.
import pytest
@pytest.mark.parametrize("weight,tier,expected", [
(10, "standard", Decimal("4.20")),
(10, "negotiated", Decimal("3.15")),
(0, "standard", Decimal("0.00")),
(999, "standard", Decimal("419.58")), # no cap — surprising, and real
(-1, "standard", Decimal("0.00")), # negative weight silently zeroed
])
def test_surcharge_characterization(weight, tier, expected):
assert calculate_surcharge(shipment(weight=weight, tier=tier),
conn=conn, today=JAN_1) == expected
The last two rows are the valuable ones. Nobody would design that behaviour, and both are now locked in — which means if your refactor changes them, you'll be told.
Use coverage as a map, not a target: run it, look at which branches near your change are still unvisited, and add cases for those only.
Check: the parametrised list includes at least one case whose result surprised you. If nothing surprised you, you haven't probed the edges hard enough — try zero, negative, empty, and enormous.
Step 5: The decision point — document or fix (10 min)
You will find something wrong. Tolvern's: tier="premium" falls through to the standard rate, so premium customers have been undercharged for years.
Two plausible responses, and taking the wrong one turns a ninety-minute task into a three-week one.
The signal that decides it: is fixing this part of the change you came here to make?
No → characterize it, with a comment saying it's wrong and a ticket reference. You are recording reality so your refactor stays honest.
def test_premium_falls_through_to_standard_rate():
"""Characterizes a KNOWN BUG (SHIP-4021): premium should be 0.85x.
Locked in deliberately so the refactor doesn't change it silently."""
assert calculate_surcharge(shipment(weight=10, tier="premium"),
conn=conn, today=JAN_1) == Decimal("4.20")
Yes → fix it as its own change, with its own test asserting the correct value, separate from the refactor.
What you must not do is fix it quietly inside the refactor. Then the diff contains both a restructuring and a behaviour change, and when billing queries the numbers next month nobody can tell which caused what.
Four Tolvern customers had been quoting from that undercharged rate for two years. Fixing it silently would have raised their prices with no announcement.
Check: every characterization test asserting known-wrong behaviour has a comment saying so and a ticket. Without that, the next reader will assume it's the specification.
Step 6: Prove the suite would notice (10 min)
A characterization suite you haven't seen fail is a guess.
# Temporarily, in the source:
- return base * Decimal("0.42")
+ return base * Decimal("0.43")
Check: run the suite. Multiple tests must fail, and the message must show the numbers. Revert. If nothing failed, your tests aren't reaching the code — most often because the seam in Step 2 is still bypassed on the path you're exercising.
Step 7: Now refactor, one step at a time (10 min)
With the suite green and proven, extract, rename and restructure — running the tests after each step, not at the end.
The rule that keeps this safe: the tests do not change during a refactor. If a test needs updating, you have changed behaviour, and that is a separate commit with a separate justification.
Check: your refactor commit touches source files only. Any diff that changes both source and characterization assertions in one commit is doing two things at once.
The wrong approach beside the right one
Test what it should do | Characterize what it does | |
Written from | the spec, or a guess | the code's actual output |
Known bugs | test fails immediately | locked in, commented, ticketed |
Can you refactor behind it | no — it's already red | yes |
Tells you the refactor changed behaviour | no | yes |
When the bug is fixed | test finally passes | test is updated, deliberately, in its own commit |
You're done when
Every test passes against the unmodified legacy code.
A one-character change to the calculation turns several tests red.
Every deliberately-wrong assertion carries a comment and a ticket.
Running the suite a month later gives identical results.
Your refactor commit changes no test assertions.
Troubleshooting
You can't call the function at all. Step 2 is the work. Add defaulted parameters — never restructure while untested.
Results differ between runs. Something unpinned: the clock, a random seed, dictionary ordering, or shared test data.
Coverage is 30% and you feel behind. You're meant to. Cover what your change will disturb; the rest can wait for its own reason.
A test fails and you can't tell whether that's good. You've mixed refactoring with behaviour change. Revert to the last green state and do one at a time.
To keep new code from becoming the next legacy problem, see Writing Testable Code: Patterns That Help.
The known-bug test looks like a spec to a new joiner. The comment is missing. This is the most likely way this suite misleads someone.
Next
Once the function is safe to change, the next question is usually whether the wider flow still works — Write a Useful Integration Test. And when deciding how much of this to automate at all, the coupling argument is what makes the call.
