http://qs321.pair.com?node_id=1068189


in reply to How to deal with old OO code that mixes instance methods with class methods

It sounds like you have quite the mess on your hands. You've mentioned that it has a number of reported bugs, so leaving it be is not an option.

When I find situations like this, I generally use the following strategy.

For each bug:
  1. Find the responsible module
  2. Write decent, full-stack integration (not unit!) tests to expose the bug
  3. If you cannot write an integration test that exposes the bug, you likely have some bad assumptions somewhere
  4. Write a unit test that exposes the bug (if feasible)
  5. Fix bug
  6. Watch the relevant tests pass (or return to previous step until they do)

I then keep repeating the above steps for each bug. This might seem like a strange approach, but here's the rationale.

Integration tests, by their very nature, cover a lot more code and they're often your only resort if the code cannot be unit tested. Plus, they tell you if the individual components integrate (i.e., can talk to each other). The downside of integration tests is that they tend to expose more bugs, but they make it harder to find the source of the bug or to nail down the exact behavior of a single piece of code. You then fall back to a unit test (if possible) to focus on the bug. Relying on a unit test alone runs the risk of "fixing" the behavior in such a way that doesn't always work for other parts of the system (since, by their very nature, unit tests ignore integration).

By repeating these steps for each bug, you slowly start to build up an integration test suite that tends to cover the known fragile parts of the code. When you're ready to refactor something, you can start fleshing out your integration tests to cover enough code to feel moderately confident that you won't damage things too much.

It's a slow, tedious process, but refactoring legacy test suites is hard and the "better safe than sorry" approach is warranted.

As an aside, I've often found that focusing too much on unit tests means that you can waste time unit testing dead code. Worse, I see people running Devel::Cover and reporting very high code coverage over unit tests, but that obscures how much of that code can actually be called. If you put your full-stack integration tests in a separate directory, you can run your code coverage over that and when you find parts of your code aren't covered, you can then ask "am I missing tests or is this dead code?"