34

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?

  • 26
    Refactoring is, by definition, changing the structure of the code while retaining existing functionality, behind some set of interfaces that you are not changing. All your (regression) test cases should be expressed in terms of those interfaces, so that when you get to the refactoring step, you are (1) not changing the tests, and (2) all your code is already covered (often enough indirectly) by existing tests. That's the whole point; the test provide a "safety net" of sorts that gives you confidence that you haven't broken anything during refactoring. 1/2 – Filip Milovanović Nov 12 at 4:18
  • 15
    If you have to change the tests because you have to change the "surface level" code that the tests are written in terms of (the outer interfaces and other types that serve to encapsulate the internals), then you are not doing refactoring, but some other kind of restructuring (and that's sometimes necessary). In that case, in a certain sense, you are writing new code and new tests for it. The tests are essentially a stand in for client code, and should not depend on the details of the implementation. 2/2 – Filip Milovanović Nov 12 at 4:18
  • 3
    @FilipMilovanović: it is debatable if using the term "refactoring" requires necessarily "some set of interfaces that you are not changing" or just unchanged external behaviour. Personally, I would not to insist using a different term for the restructuring shown in this example. – Doc Brown Nov 12 at 4:55
  • 2
    @DocBrown: I use the term "interface" in a general sense - public types & data strutures, APIs, contracts, conventions - whatever the client code is written in terms of; if you change that, the client code will break, so you can't test for unchanged external behavior without adjusting the client, but then you are risking introducing bugs there (the notion of "tests check the code, and the code checks the tests" relies on changing one at a time). Now, I agree, that is not explicitly in the definition of the term, but is of practical importance. – Filip Milovanović Nov 12 at 5:15
  • 10
    The biggest problem with your question is that you make Clamper public. A small detail, yet it has huge consequences. By making it public, you change the public surface (API, interface or whatever else you may choose to call it). As such, this isn't then a refactor; it's introducing new public functionality. So you should write a test for that functionality before introducing that public class. But it shouldn't be public in the first place. Mark it internal and it just becomes an implementation detail, ie a refactor and no new test is needed. – David Arno Nov 12 at 11:25

10 Answers 10

49

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.

  • I think this link is relevant to the direction of your answer: javacodegeeks.com/2019/01/test-driven-development-wrong.html – Albuquerque Nov 12 at 9:25
  • 4
    For clarification, I agree that separating the Clamping class and method is not a good refactor. I wanted to give an example where making a new class was part of the refactor solution and I came out with that one. Perhaps it hindered my question? – Albuquerque Nov 12 at 9:27
  • 3
    @Albuquerque: Off-topic, that link is ad-spam taken to an infuriating degree. Fixed header ad, fixed footer ad, left ad, two fixed right ads, and then a full-screen modal ad. The content of the article seems alright at first sight though I get the impression that it lacks punch at the end and is written more as an ad-carrier than an actual piece with a proper conclusion. – Flater Nov 12 at 9:28
  • 1
    It's just using the tools of TDD a strategy to cope with a difficult problem as an individual developer (so you're not wearing the "consumer hat" in this case, so to speak). For example, if there's a tricky internal substep that is difficult to get right (but the code is covered by higher-level tests), you just write a couple of finer-grained tests so that you can have quick feedback on your progress, and then throw them away when you're satisfied. (I think I heard Kent Beck talk about the idea in a podcast somewhere, but I'm not sure, so don't quote me on that.) – Filip Milovanović Nov 12 at 11:10
  • 1
    Refactoring means unchanged behavior so if you never make any mistake in refactoring, then the behavior will not change. But if you never make any mistake, I wonder why you shold make any test at all – edc65 Nov 14 at 15:21
20

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.

  • 5
    Extracting logic to another class without changing behaviour of original - is refactoring ;) – Fabio Nov 12 at 10:14
  • @AntP: see my edit. Better now? – Doc Brown Nov 12 at 11:42
  • 4
    My apologies, I didn't read you answer properly and jumped to conclusions. Having read it properly - especially with the edit - it actually agrees with what I said in a comment. So it's now a +1 (and a red face) from me. – David Arno Nov 12 at 11:51
  • 4
    Comparing this answer to the current accepted one, I feel that this one tackles the "so you want to add a public helper class Clamper using TTD" in a very appropriate way while the current accepted elaborates on "what is refactoring and how TTD should focus on behavior not implementation details". I believe both answers compose, together, a great answer to the question. – Albuquerque Nov 12 at 15:48
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.

  • I believe that @Filip Milovanović comments on the original question really add to this answer to make it more complete. – Albuquerque Nov 12 at 9:23
2

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.

2

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.

1

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.

0

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.

  • 1
    But the problem is that if your tests are too low-level, then refactoring breaks the tests even though the external functionality is perfectly OK. – Michael Kay Nov 13 at 18:15
  • @MichaelKay that's always a tradeoff in unit testing... Doesn't remove from the fact that you should have comprehensive tests – jwenting Nov 14 at 7:02
  • Yes, but "should" and "comprehensive" are subjective terms. For example, I'm happy to live with having unreachable and untested code in the product if I think it's worth retaining because it might be useful in the future: these things are value judgements. – Michael Kay Nov 14 at 12:26
0

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.

0

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.

-2

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.

  • 1
    Without specifying when the test should be written you aren't answering the question sufficiently. – Caleb Nov 13 at 7:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.