In TDD, should I add unit tests to refactored code?

Question

While refactoring my code using Test Driven Development (TDD), should I keep making new test cases for the new refactored code I am writing?

This question is bases on the following TDD steps:

1. Write just enough of a test for code to fail
2. Write just enough code for the test to pass
3. Refactor

My doubt is in the refactor step. Should new unit test cases be written for refactored code?

To illustrate that, I will give a simplified example:

---

Suppose I am making a RPG and I am making a HPContainer system that should do the following:

• Allow the player to lose HP.
• HP should not go below zero.

To answer that, I write the following tests:

[Test]
public void LoseHP_LosesHP_DecreasesCurrentHPByThatAmount()
{
    int initialHP = 100;
    HPContainer hpContainer= new HPContainer(initialHP);
    hpContainer.Lose(5)
    int currentHP = hpContainer.Current();
    Assert.AreEqual(95, currentHP);
}

[Test]
public void LoseHP_LosesMoreThanCurrentHP_CurrentHPIsZero()
{
    int initialHP = 100;
    HPContainer hpContainer= new HPContainer(initialHP);
    hpContainer.Lose(200)
    int currentHP = hpContainer.Current();
    Assert.AreEqual(0, currentHP);
}

To satisfy the requirements, I implement the following code:

public class HPContainer
{
    private int currentHP = 0;

    public void HPContainer(int initialHP)
    {
        this.currentHP = initialHP; 
    }

    public int Current()
    {
        return this.currentHP;
    }

    public void Lose(int value)
    {
        this.currentHP -= value;
        if (this.currentHP < 0)
            this.currentHP = 0;
    }
}

Good!

The tests are passing.

We did our job!

---

Now let's say the code grows and I want to refactor that code, and I decide that adding a Clamper class as following is a good solution.

public static class Clamper
{
    public static int ClampToNonNegative(int value)
    {
        if(value < 0)
            return 0;
        return value;
    }
}

And as a result, changing the HPContainer class:

public class HPContainer
{
    private int currentHP = 0;

    public void HPContainer(int initialHP)
    {
        this.currentHP = initialHP; 
    }

    public int Current()
    {
        return this.currentHP;
    }

    public void Lose(int value)
    {
        this.currentHP = Clamper.ClampToNonNegative(this.currentHP - value);
    }
}

The tests still pass, so we are sure we did not introduce a regression in our code.

But my question is:

Should unit tests be added to the class Clamper?

---

I see two opposing arguments:

1. Yes, tests should be added because we need to cover Clamper from regression. It will ensure that if Clamper ever needs to be changed that we can do that safely with test coverage.

2. No, Clamper is not part of the business logic, and is already covered by the test cases of HPContainer. Adding tests to it will only make unnecessary clutter and slow future refactoring.

What is the correct reasoning, following the TDD principles and good practices?

Answer 1

Testing before and after

In TDD, should I add unit tests to refactored code?

"refactored code" implies you are adding the tests after you've refactored. This is missing the point of testing your changes. TDD very much relies on testing before and after implementing/refactoring/fixing code.

• If you can prove that the unit test outcomes are the same before and after your refactoring, you've proven that the refactoring did not change the behavior.
• If your tests went from failing (before) to passing (after), you've proven that your implementations/fixes have solved the issue at hand.

You shouldn't be adding your unit tests after you refactor, but rather before (assuming these tests are warranted of course).

---

Refactoring means unchanged behavior

Should new unit test cases be written for refactored code?

The very definition of refactoring is to change the code without changing its behavior.

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior.

As unit tests are written specifically to test the behavior, it doesn't make sense for you to require additional unit tests after refactoring.

• If these new tests are relevant, then they were already relevant before the refactoring.
• If these new tests are not relevant, then they are obviously not needed.
• If these new tests were not relevant, but are now, then your refactoring must invariably have changed the behavior, which means you've done more than just refactoring.

Refactoring can inherently never lead to needing additional unit tests that were not needed before.

---

Adding tests needs to happen sometimes

That being said, if there were tests that you should have had from the beginning but you had forgotten it until now, of course you can add them. Don't take my answer to mean that you can't add tests just because you had forgotten to write them before.

Similarly, sometimes you forget to cover a case and it only becomes apparent after you've encountered a bug. It's good practice to then write a new test that now checks for this problem case.

---

Unit testing other things

Should unit tests be added to the class Clamper?

It seems to me that Clamper should be an internal class, as it is a hidden dependency of your HPContainer. The consumer of your HPContainer class doesn't know that Clamper exists, and doesn't need to know that.

Unit tests only focus on external (public) behavior to consumers. As Clamper should be internal, it requires no unit tests.

If Clamper is in another assembly altogether, then it does need unit testing as it is public. But your question makes it unclear if this is relevant.

Sidenote
I'm not going to go into a whole IoC sermon here. Some hidden dependencies are acceptable when they are pure (i.e. stateless) and don't need to be mocked - e.g. no one is really enforcing that .NET's Math class be injected, and your Clamper is functionally no different from Math.
I'm sure that others will disagree and take the "inject everything" approach. I'm not disagreeing that it can be done, but it's not the focus of this answer as it's not pertinent to the posted question, in my opinion.

---

Clamping?

I don't think the clamping method is all that necessary to begin with.

public static int ClampToNonNegative(int value)
{
    if(value < 0)
        return 0;
    return value;
}

What you've written here is a more limited version of the existing Math.Max() method. Every usage:

this.currentHP = Clamper.ClampToNonNegative(this.currentHP - value);

can be replaced by Math.Max:

this.currentHP = Math.Max(this.currentHP - value, 0);

If your method is nothing but a wrapper around a single existing method, it becomes pointless to have it.

Answer 2

This could be seen as two steps:

• first you are going to create a new public class Clamper (without changing HPContainer). This is actually not a refactoring, and when applying TDD strictly, literally following the nano-cycles of TDD, you would not even be allowed to write the first line of code for this class before you write at least one unit test for it.

• then you start refactoring the HPContainer by using the Clamper class. Assuming that the existing unit tests for this class already provide sufficient coverage, there is no need to add any more unit tests during this step.

So yes, if you create a reusable component with the intention of using it for a refactoring in the near future, you should add unit tests for the component. And no, during the refactoring you usually don't add more unit tests.

A different case is when Clamper is still kept private / internal, not intended for reuse. Then the whole extraction can be seen as one refactoring step, and adding new unit tests does not necessarily bring any benefit. However, for these cases, I would also take into consideration how complex the components are - if the two components are so complex that the root cause for a failing test which tests both may be hard to spot, then it may be a good idea to provide individual unit tests for both: one set of tests which tests Clamper on its own, and one testing HPContainer with an injected mock for Clamper.

Answer 3

Clamper is its own unit - and units should be tested with Unit tests - as units can be used elsewhere. Which is great if Clamper is also helping you to implement ManaContainer, FoodContainer, DamageCalculator, etc...

If Clamper were an implementation detail only, then it cannot be directly tested. This is because we cannot access it as a unit in order to test it.

Your first example treats the check as an implementation detail - which is why you didn't write a test checking that the if statement works in isolation. As an implementation detail, the only way to test it is to test the observable behaviour of the unit of which it is an implementation detail (in this case the behaviour of HPContainer centered around Lose(...)).

To keep the refactoring, but leave it an implementation detail:

public class HPContainer
{
    private int currentHP = 0;

    public void HPContainer(int initialHP)
    {
        this.currentHP = initialHP; 
    }

    public int Current()
    {
        return this.currentHP;
    }

    public void Lose(int value)
    {
        this.currentHP = ClampToNonNegative(this.currentHP - value);
    }

    private static int ClampToNonNegative(int value)
    {
        if(value < 0)
            return 0;
        return value;
    }
}

Gives you the expressiveness, but leaves the decision to introduce a new unit to later. Hopefully when you've got several instances of duplication from which you can reasonably generalise a reusable solution. Right now (your second example) is presuming it will be needed.

Answer 4

Should unit tests be added to the class Clamper?

Not yet.

The goal is clean code that works. Rituals that don't contribute toward this goal are waste.

I get paid for code that works, not for tests, so my philosophy is to test as little as possible to reach a given level of confidence -- Kent Beck, 2008

Your refactoring is an implementation detail; the external behavior of the system under test hasn't changed at all. Writing a new collection of tests for this implementation detail is not going to improve your confidence at all.

Moving implementation into a new function, or a new class, or a new file -- we do these things for a number of reasons unrelated to the behavior of the code. We don't need to introduce a new suite of tests yet. These are changes in structure, not behavior

Programmer tests should be sensitive to behavior changes and insensitive to structure changes. -- Kent Beck, 2019

The point where we start to think about change is when we are interested in changing the behavior of Clamper, and the extra ceremony of creating an HPContainer starts to get in the way.

You wanted a banana but what you got was a gorilla holding the banana and the entire jungle. -- Joe Armstrong

We're trying to avoid the situation where our tests (which serve as documentation of the expected behavior of some module in our solution) are polluted with a bunch of irrelevant details. You've probably seen examples of tests that create some test subject with a bunch of null objects because real implementations aren't necessary for the current use case but you can't invoke the code without them.

For purely structural refactorings, though, no you don't need to start introducing new tests.

Answer 5

No, don't write tests for Clamper class,
because it is already tested through tests for HPContainer class.

If you write simplest and quickest possible solution to make tests pass you end up with one big class/function which do everything.

When you start refactoring, because now you can see whole picture of the implementation, you will be able to recognize duplications or some patterns in the logic.
During refactoring you remove duplication by extracting duplications to dedicated methods or classes.

If you decide to pass newly introduced classes via constructor, you will need to change only one place in the tests where you setup class under the test to pass new dependencies. This should be only change of test code "allowed" during refactoring.

If you write tests for the classes introduced during refactoring you will end up in "infinite" loop.
You will not be able to "play" with different implementations, because you "forced" to write tests for new classes, which is silly, because this classes are tested already through tests for main class.

In most of the cases refactoring is extracting some duplicated or complicated logic in more readable and structured way.

Answer 6

Personally, I'm a great believer in testing only against stable interfaces (whether external or internal) that aren't likely to be affected by refactoring. I don't like creating tests that will inhibit refactoring (I've seen cases where people couldn't implement a refactoring because it would break too many tests). If a component or subsystem has a contract with other components or subsystems that it will deliver a particular interface, then test that interface; if an interface is purely internal, then don't test it, or throw your tests away once they've done their job.

Answer 7

Unit tests are what gives you some assurance that your refactoring effort didn't introduce bugs.

So you write unit tests and make sure they pass without changing the existing code.

Then you refactor, ensuring that your unit tests don't fail while doing so.

That is how you have some level of certainty that your refactoring didn't break things. Of course that's only true if your unit tests are correct and cover all possible code paths in the original code. If you miss something in the tests, you still run the risk of your refactoring breaking things.

Answer 8

This is how I generally like to structure and think about my tests and code. Code should be organised into folders, folders may have subfolders further subdividing it, and folders that are leaves (doesn't have subfolders) is called a file. The tests should also be organised into a corresponding hierarchy that mirrors the main code's hierarchy.

In languages where folders doesn't make sense, you can replace it with packages/modules/etc or other similar hierarchic structures in your language. It doesn't matter what the hierarchical element is in your project, the important point here is to organise your tests and main code with matching hierarchies.

The tests for a folder within the hierarchy should completely cover every code under the corresponding folder of the main codebase. A test that indirectly tests code from a different parts of the hierarchy are accidentals, and doesn't count towards coverage of that other folder. Ideally, there shouldn't be any code that's only called and tested by tests from a different parts of the hierarchy.

I don't recommend subdividing test hierarchy to the class/function level. It is usually too fine grained and it doesn't give you much benefit to subdivide things in that detail. If a main code file is big enough that it'd warrant multiple test files, then it usually indicates that the file is doing too much and should've been broken down.

Under this organisation structure, then if your new class/function lives under the same leaf folder as all the code that's using it, then it doesn't need its own tests as long as the tests for that file already covers it. If, on the other hand, you consider the new class/method big enough or independent enough to warrant its own file/folder in the hierarchy, then you should also make the corresponding test file/folder.

Generally speaking, a file should be around the size that you can fit the rough outline into your head and where you can write a paragraph to explain what the content of the files to describe what brings them together. As a rule of thumb, this is usually about a screenful to me (a folder shouldn't have more than a screenful of subfolders, a file shouldn't have more than a screenful of top level classes/functions, a function shouldn't have more than a screenful of lines). If imagining the outline of the file feels hard, then the file is probably too big.

Answer 9

As other answers have noted, what you're describing doesn't sound like refactoring. Applying TDD to refactoring would look like this:

1. Identify your API surface. By definition, refactoring will not change your API surface. If the code was written without a clearly designed API surface, and consumers depend on implementation details, then you have bigger problems that cannot be addressed by refactoring. This is where you either define an API surface, lock down everything else, and bump the major version number to signify that the new version is not backward compatible, or throw out the entire project and rewrite it from scratch.

2. Write tests against the API surface. Think of the API in terms of guarantees, e.g., method Foo returns a meaningful result when given a parameter that meets specified conditions, and throws a specific exception otherwise. Write tests for every guarantee you can identify. Think in terms of what the API is supposed to do, not what it actually does. If there was an original specification or documentation, study it. If there wasn't, write some. Code without documentation is neither right nor wrong. Do not write tests against anything that is not in the API specification.

3. Start modifying the code, running your tests frequently to ensure that you haven't broken any guarantees of the API.

There's a disconnect in many organizations between developers and testers. Developers who don't practice TDD, at least informally, are often unaware of the characteristics that make code testable. If all developers wrote testable code, there would be no need for mocking frameworks. Code that isn't designed for testability creates a chicken and egg problem. You can't refactor without tests, and you can't write tests until you've fixed the code. The costs of not practicing TDD from the start are enormous. Changes are likely to cost more than the original project. Again, this is where you resign yourself to either making breaking changes, or throwing the whole thing out.

Answer 10

A refactor is generally just changing existing code to still obtain the same objectives. If the code's purpose has not changed, it should still pass the existing unit tests.

If the code's purpose has changed, new unit tests should be written. If the structure of the code has fundamentally changed, then new unit tests should be written.

Write a unit test for Clamper.