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.
