3

One of the main reasons for writing unit tests is to make sure that code still behaves the same way after refactoring. However, if interfaces change during refactoring, the tests need to be updated as well. Changing tests and production code at the same time introduces risk. How do you avoid it?

As an example, say you encounter a method that's too large, has too much conditional logic and parts of it can to be broken down into classes of their own. The method CreateNextNumber below is smelly. The switch statement violates the open-close principle, requiring the method to be 'opened up' to add a new operation type. Maybe I've been tasked with adding Division as an option? So I'm starting with refactoring this method.

class NumberCreator
{
    INumberRepository _numberRepository;

    public NumberCreator(INumberRepository numberRepository)
    {
        _numberRepository = numberRepository;
    }

    public void CreateNextNumber(OperationType operationType)
    {
        double firstNumber = _numberRepository.GetFirstNumber();
        double secondNumber = _numberRepository.GetSecondNumber();

        // In real life, this switch statement could be only a tiny part
        // of a much larger method
        double newValue;
        switch(operationType)
        {
            case OperationType.Addition:
                newValue = firstNumber + secondNumber;
                break;
            case OperationType.Subtraction:
                newValue = firstNumber - secondNumber;
                break;
            case OperationType.Multiplication:
                newValue = firstNumber * secondNumber;
                break;
            default:
                throw new ArgumentException("Unsupported operation", nameof(operationType));
        }

        _numberRepository.SaveNumber(newValue);
    }
}

Of course, before I start refactoring, I make sure it's covered by passing unit tests. Luckily, it is:

[TestFixture]
public class NumberCreatorTest
{
    NumberCreator _creator;
    INumberRepository _repository;

    [SetUp]
    public void SetUp()
    {
        _repository = Substitute.For<INumberRepository>();
        _repository.GetFirstNumber().Returns(2);
        _repository.GetSecondNumber().Returns(3);

        _creator = new NumberCreator(_repository);
    }

    [Test]
    public void Add()
    {
        _creator.CreateNextNumber(OperationType.Addition);

        _repository.Received().SaveNumber(5);
    }

    [Test]
    public void Subtraction()
    {
        _creator.CreateNextNumber(OperationType.Subtraction);

        _repository.Received().SaveNumber(-1);
    }

    [Test]
    public void Multiplication()
    {
        _creator.CreateNextNumber(OperationType.Multiplication);

        _repository.Received().SaveNumber(6);
    }

    [Test]
    public void Unsupported()
    {
        Assert.Throws<ArgumentException>(() => _creator.CreateNextNumber((OperationType)999));
    }
}

Step 1. I refactor the method not to take the enum value but an IOperation interface instead. In order to keep any existing code (and the tests) compiling, I keep the old signature of the method too but mark it as [Obsolete].

class NumberCreator
{
    INumberRepository _numberRepository;

    public NumberCreator(INumberRepository numberRepository)
    {
        _numberRepository = numberRepository;
    }

    [Obsolete]
    public void CreateNextNumber(OperationType operationType)
    {
        IOperation operation;
        switch(operationType)
        {
            case OperationType.Addition:
                operation = new Addition();
                break;
            case OperationType.Subtraction:
                operation = new Subtraction();
                break;
            case OperationType.Multiplication:
                operation = new Multiplication();
                break;
            default:
                throw new ArgumentException("Unsupported operation", nameof(operationType));
        }

        CreateNextNumber(operation);
    }

    public void CreateNextNumber(IOperation operation)
    {
        double firstNumber = _numberRepository.GetFirstNumber();
        double secondNumber = _numberRepository.GetSecondNumber();

        double newValue = operation.Perform(firstNumber, secondNumber);

        _numberRepository.SaveNumber(newValue);
    }
}

public interface IOperation
{
    double Perform(double left, double right);
}

public class Addition : IOperation
{
    public double Perform(double left, double right)
    {
        return left + right;
    }
}

public class Subtraction : IOperation
{
    public double Perform(double left, double right)
    {
        return left - right;
    }
}

public class Multiplication : IOperation
{
    public double Perform(double left, double right)
    {
        return left * right;
    }
}

I rerun the tests - they're still green! So my refactoring was successful. However, at this point the tests I wrote previously already test more classes than just the NumberCreator.

Step 2. I decide to get rid of the Obsolete method. However, this would break the tests. So before I do that, being as lazy as I am, I update the tests in the smallest possible way to keep them compiling and passing.

[TestFixture]
public class NumberCreatorTest
{
    NumberCreator _creator;
    INumberRepository _repository;

    [SetUp]
    public void SetUp()
    {
        _repository = Substitute.For<INumberRepository>();
        _repository.GetFirstNumber().Returns(2);
        _repository.GetSecondNumber().Returns(3);

        _creator = new NumberCreator(_repository);
    }

    [Test]
    public void Add()
    {
        _creator.CreateNextNumber(new Addition());

        _repository.Received().SaveNumber(5);
    }

    [Test]
    public void Subtraction()
    {
        _creator.CreateNextNumber(new Subtraction());

        _repository.Received().SaveNumber(-1);
    }

    [Test]
    public void Multiplication()
    {
        _creator.CreateNextNumber(new Multiplication());

        _repository.Received().SaveNumber(6);
    }
}

But now my tests for NumberCreator ended up testing implementations of IOperation more than anything else! Had I designed NumberCreator and IOperation like this from the beginning, each IOperation would have its own test suite and the test of NumberCreator would use a mock:

[TestFixture]
public class NumberCreatorTest
{
    NumberCreator _creator;
    INumberRepository _repository;

    [SetUp]
    public void SetUp()
    {
        _repository = Substitute.For<INumberRepository>();
        _repository.GetFirstNumber().Returns(2);
        _repository.GetSecondNumber().Returns(3);

        _creator = new NumberCreator(_repository);
    }

    [Test]
    public void CreateNextNumber()
    {
        var operation = Substitute.For<IOperation>();
        operation.Perform(2, 3).Returns(10);

        _creator.CreateNextNumber(operation);

        _repository.Received().SaveNumber(10);
    }
}

[TestFixture]
public class AdditionTest
{
    [Test]
    public void Perform()
    {
        var operation = new Addition();
        double result = operation.Perform(2, 3);
        Assert.AreEqual(5, result);
    }
}

[TestFixture]
public class SubtractionTest
{
    [Test]
    public void Perform()
    {
        var operation = new Subtraction();
        double result = operation.Perform(2, 3);
        Assert.AreEqual(-1, result);
    }
}

[TestFixture]
public class MultiplicationTest
{
    [Test]
    public void Perform()
    {
        var operation = new Multiplication();
        double result = operation.Perform(2, 3);
        Assert.AreEqual(6, result);
    }
}

Should I keep the tests I ended up with after Step 2 despite their ugliness?

Should I have discarded my old unit tests and written new ones, rather than trying to patch what I had? The issue here is that I would have been writing tests for code that was already written (I had already performed Step 1) so I would have had no guarantee that the tests wouldn't pass regardless of whether my code worked or not. I would never be in the 'red' state of the 'red-green-refactor' cycle.

Should I have written new unit tests before I performed step 1? But back then I felt I was doing the right thing - I leveraged the existing tests suite to make sure that my code worked after refactoring. That's what tests are for!

What is the correct way that adheres to the methods of TDD and has the least risk of invalidating the tests?

edit: The linked question is hardly the same as mine. I'm not concerned with updating tests when making functional changes to the code. The poster of the other question is concerned with the amount of work it takes him to maintain the tests - my concern is making sure the tests are well designed and keep testing the right thing. The other question is rather laconic and general whereas I would appreciate help with the specific example.

  • 1
    Possible duplicate of How to Avoid Fragile Unit Tests? – gnat Oct 16 '16 at 21:58
  • @kamilk you might want to have a look at this talk by Josh Kerievsky where he presents techniques to maximize time spent under the protective shield of green tests. – guillaume31 Oct 19 '16 at 13:12
4

A possible way to avoid changing both production code and test code at the same time is to begin your refactoring by making the changes internal to your object under test, keeping the external interface intact.

In your example, the production method CreateNextNumber receives the enum OperationType, a knowledge that is present in the tests as well. You can begin by creating a temporary second method within NumberCreator, e.g. NewCreateNextNumber, which expects an instance of IOperation instead of the enum. You can begin extracting the operation implementations from the original CreateNextNumber method safely, by making ChangeNextNumber convert the enum OperationType into the respective IOperation implementation, then delegate to NewCreateNextNumber.

While doing the above, the tests will still run against ChangeNextNumber, so you're still under their safety net. After completing the internal refactor of NumberCreator, you can move the ChangeNextNumber method, which now only delegates to the new one, to the test file. The conversion from the enum to each respective IOperation instance will then be part of the test logic, and you can then refactor this logic away completely, if you wish so, because now you can change the tests in isolation. Anyway, after moving the old signature to the test file, you can then rename NewCreateNextNumber to its correct name, and have your clients use it.

0

Why did you stop after only single refactoring? "Refactoring" means making lots of changes in small steps. So stopping after only single refactoring doesn't make sense. If you think your code is ugly, you continue refactoring the code until you think it is good enough.

What is stopping your to refactor your code so it gets turned into the code you think you would start with? Now that you know it, it should be easy to write it.

  • I didn't say there was anything else I wanted to improve in the production code. But as a result of refactoring I ended up with a class structure for which I would normally write very different unit tests. But I don't think can replace the whole test suite safely. – kamilk Oct 17 '16 at 6:39
  • @kamilk But I don't think can replace the whole test suite safely. How so? It doesn't matter if you change code or tests. If both coraborate, then everything is fine. – Euphoric Oct 17 '16 at 7:02
  • Maybe it is the best solution. But normally I would try to follow TDD in writing failing tests first, which validates they test the right thing. Here I wouldn't have this stage. I currently have tests likely written in this manner but I would be discarding them in favour of tests which would pass as soon as they are written (I've already done the refactoring). Though maybe I'm exaggerating the risk? – kamilk Oct 17 '16 at 8:30

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.