TDD – Should Unit Tests Be Added to Refactored Code

Architecturedesignrefactoringtddunit testing

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?

Best Answer

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.

Related Topic