Stop changing code behaviour
Changing code’s behaviour is incompatible with Test Driven Development, here’s why and how you can still deliver changes without changing code behaviour.
Anyone who writes TDD knows how fast it can be to develop a new class. You are left with a neat set of tests and clean production code that match the requirements and you’re very confident in your robust new class.
Then over time as the requirements change, you modify both the code and tests to suit the new requirements. Modification of existing code and tests not only takes longer than writing tests and classes ‘greenfield’, but the tests start to decay:
Small things such as test names no longer match the scenario they are testing. This can introduce doubt; is the name wrong or is the test wrong?
Other text entities such as variable, method and class names, or other documentation can also start to diverge from the true functionality of the code.
When we initially TDD’d the class we got 100% coverage and we knew every line was added in response to a failing test. Now after all these changes, we might still have 100% coverage, but we can no longer be sure that all code is protected by a test.
Confidence in the test suite is diminished.
This article is for developers who struggle to maintain a meaningful TDD test suite in the face of changing requirements.
Example
In this scenario we are tasked with updating the sales tax rate in a shopping basket class.
Effective from Midnight December 31st, the sales tax rate will increase from 15% to 20%
A developer has hard coded the 15% tax rate in to the code. We’re not going to waste time judging that decision, maybe the tax rate hasn’t changed in many years, maybe it wasn’t specified in the requirements, maybe it was a concession from the client to keep costs down/include other features. Whatever happened, here we are.
So here’s our Basket class (on github v1.0):
public final class Basket {
private final List<Item> items = new LinkedList<>();
public void add(final Item item) {
items.add(item);
}
public BigDecimal subTotal() {
BigDecimal subTotal = BigDecimal.ZERO;
for (final Item item : items) {
subTotal = subTotal.add(item.getValue());
}
return subTotal;
}
public BigDecimal total() {
return subTotal()
.multiply(BigDecimal.valueOf(1.15))
.setScale(2, BigDecimal.ROUND_DOWN);
}
}And here are the tests:
public final class BasketTests {
@Test
public void canAddToBasketAndGetSubtotal() {
final Basket basket = new Basket();
basket.add(new Item(bd(1.23)));
assertEquals(bd(1.23), basket.subTotal());
}
@Test
public void canAddTwoItemsToBasketAndGetSubtotal() {
final Basket basket = new Basket();
final double val = 1.23;
basket.add(new Item(bd(val)));
basket.add(new Item(bd(2.34)));
assertEquals(bd(3.57), basket.subTotal());
}
@Test
public void canGetTotalWith15PercentTax() {
final Basket basket = new Basket();
basket.add(new Item(bd(10)));
assertEquals(bd(11.5), basket.total());
}
@Test
public void canGetTotalWith15PercentTaxRoundedDown() {
final Basket basket = new Basket();
basket.add(new Item(bd(1.10)));
assertEquals(bd(1.26), basket.total());
}
private BigDecimal bd(final double val) {
return BigDecimal
.valueOf(val)
.setScale(2, BigDecimal.ROUND_UNNECESSARY);
}
}The quick way
So, let’s do the obvious thing, let’s change the hard-coded value from 1.15 to 1.2, update the expected values in the tests and knock off early.
Our Basket class has the change:
public BigDecimal total() {
return subTotal()
.multiply(BigDecimal.valueOf(1.2))
.setScale(2, BigDecimal.ROUND_DOWN);
}We run the tests and the two tax tests fail, so we change their expected values:
@Test
public void canGetTotalWith15PercentTax() {
final Basket basket = new Basket();
basket.add(new Item(bd(10)));
assertEquals(bd(12), basket.total());
}
@Test
public void canGetTotalWith15PercentTaxRoundedDown() {
final Basket basket = new Basket();
basket.add(new Item(bd(1.10)));
assertEquals(bd(1.32), basket.total());
}Done?
No, the test names don’t match now. They refer to 15% and the test is actually testing for 20%. So we update them if we or a code reviewer even notice and if not, the decay has started and we will pay for it later. But in this case we notice and update the names:
@Test
public void canGetTotalWith20PercentTax() {
...
}
@Test
public void canGetTotalWith20PercentTaxRoundedDown() {
...
}Now are we done?
Take a close look at the test for rounding. 1.1 * 1.15 = 1.265, so the original test looking for 1.26 ensured we were rounding down correctly.
1.1 * 1.2 = 1.32, uh oh, the altered test no longer proves anything. Try it, we can mutate the code to ROUND_UP:
public BigDecimal total() {
return subTotal()
.multiply(BigDecimal.valueOf(1.2))
.setScale(2, BigDecimal.ROUND_UP);
}Run the tests and sadly they all pass (branch bad-example-updating-tests). So we had a TDD class, where everything was there for one reason; to make a test pass.
Now we still have 100% coverage, and with no fewer asserts, yet some code is completely unguarded by tests.
Whether we catch this or not, we still have the deployment to worry about. Midnight December 31st. Is someone going to wait to merge this change until closer to the date? Is someone going to have to press a button to deploy in the middle of the night, on new years eve?
So how can we avoid this?
Change nothing
Refactor, yes. Change, no.
We’re going to refactor the code to allow the Open Closed Principle — OCP.
We’re going to make the code Open for Extension but Closed for Modification.
Step one — identify an abstraction
The rate in the tax calculation is all we want to change so we could just abstract the TaxRate and pass into the Basket class’ constructor.
This is following the Dependency Inversion Principle — DIP; we are going to depend on an abstraction, not an implementation.
public final class Basket {
private final List<Item> items = new LinkedList<>();
private final TaxRate taxrate;
Basket(final TaxRate taxrate) {
this.taxrate = taxrate;
} ... public BigDecimal total() {
return subTotal()
.multiply(BigDecimal.valueOf(taxrate.getRate()))
.setScale(2, BigDecimal.ROUND_DOWN);
}
}
This would clearly work, but we should also consider the Single Responsibility Principle — SRP.
We are changing Basket because the tax rules are changing. Basket should have one responsibility, one reason for change, and that would be managing the list of items in the basket. It is not its responsibility to have any tax logic in it, either rates or rounding.
So let’s consider an abstraction to the whole tax logic.
public final class Basket {
private final List<Item> items = new LinkedList<>();
private final Tax tax;
Basket(final Tax tax) {
this.tax = tax;
}
...
public BigDecimal total() {
return tax.applyTo(subTotal());
}
}Basket now does not have the responsibility of taxation. Instead it has a dependency on a taxation abstraction.
Step 2 — refactor out the implementation
I will eventually make Tax an interface but for now I’m going to make it a class.
public final class Tax {
public BigDecimal applyTo(final BigDecimal subTotal) {
return subTotal
.multiply(BigDecimal.valueOf(1.15))
.setScale(2, BigDecimal.ROUND_DOWN);
}
}Now I need to refactor all places where I construct a Basket and inject an instance of Tax. So the tests look like this:
public final class BasketTests {
private Basket givenBasket() {
return new Basket(new Tax());
}
@Test
public void canAddToBasketAndGetSubtotal() {
final Basket basket = givenBasket();
basket.add(new Item(bd(1.23)));
assertEquals(bd(1.23), basket.subTotal());
}
@Test
public void canAddTwoItemsToBasketAndGetSubtotal() {
final Basket basket = givenBasket();
final double val = 1.23;
basket.add(new Item(bd(val)));
basket.add(new Item(bd(2.34)));
assertEquals(bd(3.57), basket.subTotal());
}
@Test
public void canGetTotalWith15PercentTax() {
final Basket basket = givenBasket();
basket.add(new Item(bd(10)));
assertEquals(bd(11.5), basket.total());
}
@Test
public void canGetTotalWith15PercentTaxRoundedDown() {
final Basket basket = givenBasket();
basket.add(new Item(bd(1.10)));
assertEquals(bd(1.26), basket.total());
}
private BigDecimal bd(final double val) {
return BigDecimal
.valueOf(val)
.setScale(2, BigDecimal.ROUND_UNNECESSARY);
}
}Notice how I refactored out a method called givenBasket. This is simply so I don’t have to repeat myself.
At this point, we have injected a dependency, but it’s not yet an abstraction.
Step 3— refactor out the abstraction
So now I make the abstraction, I’m going to make Tax an interface and provide an implementation for it.
public interface Tax {
BigDecimal applyTo(final BigDecimal subTotal);
}And here’s the implementation:
public final class Tax15PCRoundedDown implements Tax {
@Override
public BigDecimal applyTo(final BigDecimal subTotal) {
return subTotal
.multiply(BigDecimal.valueOf(1.15))
.setScale(2, BigDecimal.ROUND_DOWN);
}
}We update the method in the test class:
private Basket givenBasket() {
return new Basket(new Tax15PCRoundedDown());
}And all the tests pass. Note that in this step, we did not have to change Basket. We are beginning to see the power of the approach.
Up till now we have had no effect on the tests or code. We could (and maybe should) make a pull request now. These pure refactors are good, we’ve altered no behaviour. If this were a larger refactor, and we get these no-behaviour modification changes out to testers/production early we can identify issues before we’ve even finished the task. The earlier we catch issues, the fewer changes are involved, the easier it is to find the cause and rectify.
Step 4— Reimplement
Let’s assume we’ve taken the appropriate course with respect to deploying these refactors and continue with the actual change.
We’re going to TDD a brand new implementation of Tax.
New tests:
public final class TwentyPercentTaxTests {
private Tax givenTax() {
return new Tax20PCRoundedDown();
}
@Test
public void canApply20PercentTax() {
assertEquals(bd(1.2), givenTax().applyTo(bd(1)));
}
@Test
public void doesRoundDown() {
assertEquals(bd(12.01), givenTax().applyTo(bd(10.01)));
}
}Drives a new implementation:
public final class Tax20PCRoundedDown implements Tax {
@Override
public BigDecimal applyTo(final BigDecimal subTotal) {
return subTotal
.multiply(BigDecimal.valueOf(1.2))
.setScale(2, BigDecimal.ROUND_DOWN);
}
}Again, these changes do not affect production. They can be subject to a pull request and merged. It’s unreachable code in production, but it’s not deadcode as it’s fully test covered.
Note that you can refactor out base classes (e.g. TaxRoundedDownetc.) if you want. Or refactor a TaxMethod.Builder builder pattern that takes tax rate and rounding method and use them in both places. Refactoring with a full test suite is good and can be done without fear, but for the purpose of this article I’m not going to go overboard, I’ve done the bare minimum. There’s some duplication it’s true, but I actually like how Tax15PCRoundedDown and Tax20PCRoundedDown have no dependencies, for me, the refactoring here can wait.
Step 5— Refactor tests
Our original tests for Basket are now a mixture of unit tests and integration tests. I can keep the integration tests, or decide to create new tests along the lines of the TwentyPercentTaxTests.
I decide I want to keep the integration tests for now, but that I want to separate them from the BasketTests. Just as the Basket class has no business implementing tax rules, BasketTests has no business testing them. We move them out from BasketTests:
public final class BasketTests {
private Basket givenBasket() {
return new Basket(mock(Tax.class));
}
@Test
public void canAddToBasketAndGetSubtotal() {
final Basket basket = givenBasket();
basket.add(new Item(bd(1.23)));
assertEquals(bd(1.23), basket.subTotal());
}
@Test
public void canAddTwoItemsToBasketAndGetSubtotal() {
final Basket basket = givenBasket();
final double val = 1.23;
basket.add(new Item(bd(val)));
basket.add(new Item(bd(2.34)));
assertEquals(bd(3.57), basket.subTotal());
}
}And create a new test class of integration tests which comprise of the two original tests and a new one, ensuring we are using the Tax instance passed in to the constructor. You could assert that via the mock in BasketTests, but I feel it would be a little “whitebox”.
public final class BasketTaxIntegrationTests {
private Basket givenBasket(final Tax tax) {
return new Basket(tax);
}
private Basket givenBasketWith15PercentTax() {
return givenBasket(new Tax15PCRoundedDown());
}
private Basket givenBasketWith20PercentTax() {
return givenBasket(new Tax20PCRoundedDown());
}
@Test
public void canGetTotalWith15PercentTax() {
final Basket basket = givenBasketWith15PercentTax();
basket.add(new Item(bd(10)));
assertEquals(bd(11.5), basket.total());
}
@Test
public void canGetTotalWith15PercentTaxRoundedDown() {
final Basket basket = givenBasketWith15PercentTax();
basket.add(new Item(bd(1.10)));
assertEquals(bd(1.26), basket.total());
}
@Test
public void canGetTotalWith20PercentTax() {
final Basket basket = givenBasketWith20PercentTax();
basket.add(new Item(bd(10)));
assertEquals(bd(12), basket.total());
}
}Step 5 — deployment
Now we need a way to deploy this new implementation. Some implementations might vary based on a system configuration, or they may be able to make a decision at runtime. I’m going to give an example of the latter:
public final class DateTaxSwitch implements Tax {
private final Date switchDate;
private final Tax oldTax;
private final Tax newTax;
public DateTaxSwitch(
final Tax oldTax,
final Date switchDate,
final Tax newTax
) {
this.oldTax = oldTax;
this.newTax = newTax;
this.switchDate = switchDate;
}
@Override
public BigDecimal applyTo(final BigDecimal subTotal) {
return getActiveTaxRate().applyTo(subTotal);
}
private Tax getActiveTaxRate() {
return new Date().before(switchDate) ? oldTax : newTax;
}
}This implementation of Tax delegates to one other Tax instance before a date and another instance after a date, depending on the date at the time at applyTo call.
We can test drive this class too, I’ll leave the tests out of this article for brevity but they are in the github repo.
So now we can setup the Tax implementation like so at runtime:
final Tax tax = new DateTaxSwitch(
new Tax15PCRoundedDown(),
JANUARY_1st_2019,
new Tax20PCRoundedDown()
);Then we can inject tax to any new instance of Basket, maybe manually, or even using dependency injection.
Summary
I showed how modifying tests to meet changing requirements can easily degrade your tests’ effectiveness.
Alternative solutions such as mutation testing can help catch unguarded logic, but they cannot catch tests or other code which have become misnamed as a result of changes.
I showed how a branch-by-abstraction technique allows us to merge sooner and more frequently, blocking our team less. Remember the oath:
I will make frequent, small, releases so that I do not impede the progress of others. — The Programmers Oath
I showed how the result of a little refactoring effort was a system that was configurable at runtime. This allows binary deployment ahead of feature deployment. It also enables us to easily rollback a feature and easily provide even more implementations in future. Requirements that change once, will likely change again. Next time we will be ready.
So stop changing your code’s behaviour, refactor and write new code.
