Test Smell: Irrelevant information

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.

Often, you have some required data to set up your test fixture or as part of parameters for the subject under test, but most of the required data is not interesting for the test case at all.

You might write gibberish or give clues in the values like “any” or “blah blah, not important”, but these uninteresting values are still a distraction from the values that are actually relevant to the test.

Problem

Test or fixture code with lots of irrelevant information is hard to understand, especially if you want to treat your tests as documentation.

Possible solutions

Use the “extract method” refactoring to create methods which take the interesting values as parameters and hide everything else. These can be creation methods that return objects or custom assertions.

Take, for example, this HTTP client mock setup:

$httpClient->expects($this->once())
    ->method('send')
    ->willReturn(
        new Http\Psr7\Response(
            500,
            ['Content-type' => 'application/json'],
            \json_encode(
                [
                    'error' => true,
                    'message' => 'Houston, we have a problem',
                ]
            )
        );
    );

$httpClient->expects($this->once())

->method('send')

->willReturn(

new Http\Psr7\Response(

500,

['Content-type' => 'application/json'],

\json_encode(

[

'error' => true,

'message' => 'Houston, we have a problem',

]

)

);

We want to test what our application does when a certain HTTP request to a JSON API returns with a status code 500 (internal server error). Let’s say we are even interested in the error message returned in the body.

You might say the test is clear, there’s not much to improve. But let’s extract a creation method with a meaningful name for the response object and see how the test setup from above looks like now:

$httpClient->expects($this->once())
    ->method('send')
    ->willReturn(
        $this->internalServerError('Houston, we have a problem')
    );

$httpClient->expects($this->once())

->method('send')

->willReturn(

$this->internalServerError('Houston, we have a problem')

);

Much easier to read without having to think “is this important?” or “what’s this for?” every second line. If the reader is interested in the implementation details, how a 500 response looks like, they can check out the internalServerError() method. But most of the time, they will read the test to understand it on a higher level.

This was a small change in a small method with not much irrelevant information. Now imagine the effect on much more complicated setups.

Example from Magento

The following is part of a fixture for a customer with “reset password” token, used in a Magento 2 core integration test. The only values relevant to the test are the customer id (because the test loads the customer by id) and the reset password token. Everything else is distraction.

$customer->setWebsiteId(
    1
)->setId(
     1
)->setEntityTypeId(
    1
)->setAttributeSetId(
    1
)->setEmail(
    'customer@example.com'
)->setPassword(
    'password'
)->setGroupId(
    1
)->setStoreId(
    1
)->setIsActive(
    1
)->setFirstname(
    'Firstname'
)->setLastname(
    'Lastname'
)->setDefaultBilling(
    1
)->setDefaultShipping(
    1
)->setRpToken(
     '8ed8677e6c79e68b94e61658bd756ea5'
)->setRpTokenCreatedAt(
     date('Y-m-d H:i:s')
);

$customer->setWebsiteId(

)->setId(

)->setEntityTypeId(

)->setAttributeSetId(

)->setEmail(

'customer@example.com'

)->setPassword(

'password'

)->setGroupId(

)->setStoreId(

)->setIsActive(

)->setFirstname(

'Firstname'

)->setLastname(

'Lastname'

)->setDefaultBilling(

)->setDefaultShipping(

)->setRpToken(

'8ed8677e6c79e68b94e61658bd756ea5'

)->setRpTokenCreatedAt(

date('Y-m-d H:i:s')

);

With test data builders from our alternative fixture library at tddwizard/magento2-fixtures this could look as follows:

$customer = CustomerBuilder::aCustomer()
    ->withCustomAttributes(
        [
            'rp_token' => '8ed8677e6c79e68b94e61658bd756ea5',
            'rp_token_created_at' => date('Y-m-d H:i:s'),
        ]
    )
    ->build()

$customer = CustomerBuilder::aCustomer()

->withCustomAttributes(

[

'rp_token' => '8ed8677e6c79e68b94e61658bd756ea5',

'rp_token_created_at' => date('Y-m-d H:i:s'),

]

)

->build()

The customer builder creates customers with default values and for our test we only specify the relevant values. If you do not want to rely on a third party library or write general purpose builders as above, you could still improve such a fixture setup by extracting a method:

$token = '8ed8677e6c79e68b94e61658bd756ea5'
$date = date('Y-m-d H:i:s');
$customer = $this->createCustomerWithResetPasswordToken($token, $date);

$token = '8ed8677e6c79e68b94e61658bd756ea5'

$date = date('Y-m-d H:i:s');

$customer = $this->createCustomerWithResetPasswordToken($token, $date);

In Magento projects I highly recommend writing a fixture library for reusable default data in one way or another. But in the tests, specify all relevant data and do not use the default data in assertions (See: Test Smell: Mystery Guest). Thus, for lack of flexibility, amongst other reasons, the core fixture scripts are not a good solution.

Note that in this last example I also replaced the hard coded value, that will be used again in the verification step, with a variable. Hard coded values are also a test smell on which I will expand in another post.

Conclusion

Hide anything that’s not immediately relevant for the test as far away as possible. That can be low level private methods or external classes like the test data builders with default values. Of course, if it’s possible to leave out the irrelevant information completely, that’s the best solution.

Test Smell: Irrelevant information

Problem

Possible solutions

Example from Magento

Conclusion

Further reading

Author: Fabian Schmengler

Submit a Comment Cancel reply