How should factored-out code be tested as part of the TDD refactoring step?

Question

When in the refactoring step of a TDD process, if we "factor out" some common functionality from two (or more) code modules, how do we avoid that "factored out" code not being properly covered by unit tests?

Here is a toy example to illustrate what I mean. Since it is a toy example, the factored-out code is very small, but we could imagine that it was a much larger set of code that was common between two modules. Let's say we have arrived at the following situation by following a TDD process:

# ----- foo.py -------
def foo(a: int, b:int) -> str:
    x = str(a + b)
    ... # some elided operations that modify x further
    return x

# ----- bar.py ------
def bar(a: int, b: int) -> str:
    x = str(a + b)
    ... # some different operations that modify x further
    return x

In the refactoring step, we want to break out the str(a + b) part of the two functions:

After:

# ---- baz.py ----
def stringy_sum(a: int, b: int) -> str:
    return str(a + b)

# ---- foo.py ----
def foo(a: int, b:int) -> str:
    x = stringy_sum(a, b)
    ... # some elided operations that modify x further
    return x

# ----- bar.py ------
def bar(a: int, b: int) -> str:
    x = stringy_sum(a, b)
    ... # some different operations that modify x further
    return x

Now we have a function stringy_sum that is not covered by any unit tests of its own. It's only covered by what could be regarded as integration tests (the tests for foo and bar still use the function, but do not actually test its operation). Where did we go wrong in this process from a TDD standpoint?

Answer 1

You went wrong by thinking of stringy_sum as something that should be tested. It's an internal implementation detail, and you shouldn't test internal implementation details.

An alternative way of looking at it: you went wrong by making stringy_sum part of your public interface. Hide it away so nobody other than the people working on the module know about it and you don't need to worry that it doesn't have its own unit tests, because your unit tests still cover all the behaviour of the module. (Yes, I know Python doesn't have a great way of saying "this is private, you can't use it"; that is mostly irrelevant here).

Answer 2

Philip Kendall's answer says the issue can be discussed away by interpreting stringy_sum as a private function, as an implementation detail which does not need a test on its own. That is a possible point of view.

However, we can also take the point of view that stringy_sum shall become a public function, intentionally, and that this is exactly what you want. For this case, we can approach the issue in a straightforward TDD way, just by making smaller "baby steps".

You can try it this way:

1. Plan the new function stringy_sum and write a test for it (which fails, since stringy_sum does not exist yet).

2. Implement stringy_sum by copying the code out of foo or bar and make some adaptions, if necessary. Now the new test should pass.

3. Modify foo by replacing the old code by a call to stringy_sum. Run all tests, they should pass (otherwise debug).

4. Do the same for bar.

This is TDD by the book - red, green, refactor - and results in a new function with a new test, which is exactly what you asked for.

To decide, however, which point of view is more adequate, toy examples don't provide enough context. In a real-world sitation, you can way more easily decide if your method to extract is something you want in public, or something you want to keep in private.

Answer 3

Rather than talk about what you can do, let’s talk about how to decide.

If this thing is going to be used by code you have no control over then it needs its own test, because it’s effectively part of an API.

Otherwise, if you have dominion over everything that will call this then you can treat it like an implementation detail and let it get tested when other things call it.

Thinking of it this way avoids depending on access modifiers that don't exist in every language. This is the real reason we care about things being public. If it's public we assume we'll lose control over how it's used. That isn't always true but it is what drove that knee jerk rule.

At the heart of this issue is how you define a unit. If you think every class is it's own unit you wrap every class in tests and no one wants to refactor anything in your code base because doing so always breaks some test.

Fowler⁼ called those solitary tests. He preferred sociable tests that allowed for many forms of abstraction. Sometimes a class abstracts many others (see The Façade Pattern⁼). Sometimes you just need to be sure the facade works and don't care about what's hiding behind it. Not caring allows for a lot of refactoring. So this was never about access modifiers anyway. It's always been about abstraction.

---

That is, until performance becomes an issue. While I am perfectly willing to allow that the unit under test can be larger than a single class I shudder to allow everything in the unit. The best rules for limiting what you allow in the unit come from Mitchel Feathers. He won’t let you call your test a unit test if:

• It talks to the database
• It communicates across the network
• It touches the file system
• It can't run at the same time as any of your other unit tests
• You have to do special things to your environment (such as editing config files) to run it.

artima.com - A Set of Unit Testing Rules

People have argued to death whether to call their tests unit tests or integration tests. Personally I don't care what they're called anymore. I care about how I can use them. I have one pile of tests that run so fast I can run them as often as I type a semicolon. I have another pile that I run when I merge code. Guess where the slow tests go?

If you don't separate these tests so you can run them at different times then you're doing it wrong.

So you decide if stringy_sum needs it's own test based on if you can control what calls it and if what calls it is fast enough that you're willing to run it over and over while you code.

Answer 4

You didn't go wrong anywhere. The function is still covered. If at some point the code base changes further so that it isn't covered anymore, then you'd have an issue, but then your coverage metrics would tell you so.

In fact, you did good, because as it was, your coverage metric was slightly misleading in that it counted basically the same code twice. Now that it has been reduced to a single occurrence, your metric is slightly closer to being functionally correct.