Test Smell: Copy and Paste

This is a series of short blog posts on the topic of test architecture. Most topics evolved from my talk “Dealing with Testing Fatigue” which highlights typical anti-patterns in test code, why they are problematic and how to improve them.

This test smell seems like a no-brainer: Code with lots of Copy&Paste duplication is bad, we all know this, right? So how is this specific to tests?

First, it is not natural to apply the same rules for production code to tests as well. Many developers really care about clean code in production code, but write a mess in their tests (I know because I’ve made this mistake too).

Second, in tests you often need the same or similar execution several times, to test the same subject with different input. So the urge to just copy&paste is more present.

Problem

So I have to stress it: readable and maintainable tests are just as important, if not even more important, than readable and maintainable production code.

Tests with duplicated logic are not affording changes, you will likely have to edit many places for one changed aspect.

Tests should help to document and understand the intended usage of the production code, so they should be readable even if they are not going to change.

Possible solutions

Luckily, the solutions are basically the same as in regular code: Use the extract method or extract class refactoring technique.

And this is how do these typically look in tests:

Extract method/class in setup phase

Extract creation method to create fixtures. E.g.

PHP

$customer = $this->createCustomer();

1

$customer = $this->createCustomer();
Extract finder method to access shared fixture. E.g.

PHP

$customer = $this->getCustomer();

1

$customer = $this->getCustomer();
Extract test data builder class to create complex or reusable fixtures. See box “Test Data Builder” below for details. E.g.

PHP

$customer = CustomerBuilder::aCustomer()->create();

1

$customer = CustomerBuilder::aCustomer()->create();

Test Data Builder

A builder is a class that is responsible to create complex objects and can be configured with parameters before actually creating the object, or object hierarchy. Builders for test data often come with default configurations, so that you only have to specify the parameters that are relevant for the current test. E.g.

$customer = CustomerBuilder::aCustomer()->withLastname('Costello')->create();

1	$customer = CustomerBuilder::aCustomer()->withLastname('Costello')->create();

Extract method/class in verification phase

Extract custom assertion method to combine assertions. E.g.

PHP

$this->assertCustomerIsLoggedIn($customer)

1

$this->assertCustomerIsLoggedIn($customer)
Extract custom constraint class to build reuse assertions across tests. See box “Constraints” below for details. E.g.

PHP

$this->assertThat($customer, new CustomerIsLoggedIn())

1

$this->assertThat($customer, new CustomerIsLoggedIn())

Constraints

Under the hood, all assertions in PHPUnit evaluate constraints:

$this->assertEquals($expected, $actual, $message)

1	$this->assertEquals($expected, $actual, $message)

is the same as

$this->assertThat($actual, $this->equals($expected), $message);
                           ^----------------------^
                                 constraint

$this->assertThat($actual, $this->equals($expected), $message);

^----------------------^

constraint

With assertThat() you can evaluate any constraint. Constraints also can be combined with AND, OR, NOT, using composite logical constraints:

$this->assertThat(0.5,
    $this->logicalAnd(
        $this->greaterThan(0),
        $this->lessThan(1)
    ),
    '0.5 should be between 0 and 1'
);

$this->assertThat(0.5,

$this->logicalAnd(

$this->greaterThan(0),

$this->lessThan(1)

'0.5 should be between 0 and 1'

);

To build your own constraints, extend the Constraint base class and implement two methods:

matches() receives the tested value and returns true if the constraint is met, i.e. the assertion should pass, false otherwise.
failureDescription() also receives the tested value and returns a failure message that is shown if the constraint is not met, i.e. the assertion should fail.

Here’s a basic example:

class IsMeaningOfLife extends \PHPUnit\Framework\Constraint
{
    protected function matches($other)
    {
        return $other === 42;
    }
    protected function failureDescription($other)
    {
        return sprintf(
            "%s is not the meaning of life, the universe and everything", $other
        );
    }
}

class IsMeaningOfLife extends \PHPUnit\Framework\Constraint

{

protected function matches($other)

{

return $other === 42;

}

protected function failureDescription($other)

{

return sprintf(

"%s is not the meaning of life, the universe and everything", $other

);

}

Usually you would parameterize the constraint and pass the parameter(s) as constructor argument, but obviously there is only one true answer to the meaning of life, the universe and everything.

When extracting, just as in production code, beware of premature abstraction! If code looks similar, it does not automatically mean it is duplicated logic. For example, if you find yourself extracting methods and adding “flags” as arguments to let the method do slightly different things, stop what you are doing and think again: Either the method can be divided into smaller composable parts, or you are better off having some duplicated code instead of a costly abstraction.

Data providers

Another solution, specific to tests, are data providers. With them you can feed the same test method with different arguments. Usually those arguments will be input and expected output for the test execution. Details can be found in the PHPUnit documentation. The same warning as above applies here: do not use the arguments as “flags”. I’ll cover that topic with another post, Test Smell: Conditional Test Logic.

Example from Magento

My favorite copy&paste example is from the integration test fixture scripts: Take a look at these file names and imagine the worst:

dev/tests/integration/testsuite/Magento/Customer/_files/customer.php
dev/tests/integration/testsuite/Magento/Customer/_files/two_customers.php
dev/tests/integration/testsuite/Magento/Customer/_files/three_customers.php
dev/tests/integration/testsuite/Magento/Customer/_files/twenty_one_customers.php

dev/tests/integration/testsuite/Magento/Customer/_files/customer.php

dev/tests/integration/testsuite/Magento/Customer/_files/two_customers.php

dev/tests/integration/testsuite/Magento/Customer/_files/three_customers.php

dev/tests/integration/testsuite/Magento/Customer/_files/twenty_one_customers.php

It’s exactly like that: the first file contains a bunch of spaghetti code and the last file contains the same spaghetti code plus twenty copies of it with slight variations. I mean, come on! Nobody would do this in production code, if they are not a total newbie. There is nothing wrong with being a newbie, but Magento is architected, written and reviewed by experienced developers, that for some reason thought that in tests they can throw away all rules of sane programming.

An improvement without changing the structure of fixture scripts would be to extract a function with arguments that creates a new customer:

$createCustomer = function($email, $firstname, $lastname) {
   // here the original code, but only once
};
for ($i = 1; $i <= 21; ++$i) {
  $createcustomer("customer{$i}@example.com", "Firstname{$i}", "Lastname{$i}");
}

$createCustomer = function($email, $firstname, $lastname) {

// here the original code, but only once

};

for ($i = 1; $i <= 21; ++$i) {

$createcustomer("customer{$i}@example.com", "Firstname{$i}", "Lastname{$i}");

}

But I do not recommend to use these scripts and rather set up your fixtures in the test case itself, or in a shared class, where you will be able to add this function as reusable piece of code. You can read more about the reasoning in the previous post Test Smell: Mystery Guest and about alternative solutions in Improved Fixtures for Magento 2 Integration Tests.

Conclusion

Readable and maintainable tests are just as important, if not even more important, than readable and maintainable production code. Use known refactoring techniques to remove duplicated logic and data providers to run tests with different inputs. But do not abstract too eagerly, avoid the conditional test logic smell.