TDD anti-patterns - episode 2 - the mockery, the inspector, the generous leftovers and the local hero - with code examples in javascript, kotlin and php

Last updated Aug 20, 2022 Published Jan 6, 2022

The content here is under the Attribution 4.0 International (CC BY 4.0) license

This is a follow up on a series of posts around TDD anti patterns. The first of this series covered: the liar, excessive setup, the giant and slow poke, those for are part of 22 more, in which is listed in the James Carr post.

Do you prefer this content in video format? The video is available on demand in livestorm.

In this blog post we are going to focus on four more of them, named: The mockery, The inspector, The generous leftovers and The local hero. Each one of them focus on a specific aspect of code that makes testing harder.

The mockery

The mockery is one of the most popular anti pattern across developers, as it seems that everyone has had some experience mocking code to test. The first idea, of mocking is simple: avoid external code to focus on what you want to test. Therefore, mocking has it’s own naming and categories that makes it unique and at the same time tricky to use them correctly.

For example, [1] and [2], classifies the mocks family in different types: dummies, spies, fakes and stubs (I also wrote a list based on his blog post). Uncle Bob refers to “True mocks” whereas Martin Fowler refers to “Test double”. Uncle Bob explains that “mocks” got spread as it is easier to say: “I will mock that”, “can you mock this”?

If you like to know a bit of the history behind that, this paper can help out on how mocks were conceived and also this blog post.

The difference between those are important, because we, as developers, have the habit to call everything a mock, thus misleading sometimes what we want to mean, spring.io goes even further avoiding the debate about if we should use mocks or not (referring to the Classical and the London school of TDD) - I will do the same, this is a discussion that deserves a blog post itself. Martin Fowler also wrote about this very same subject [3] and agrees that for a while he also thought about mocks and stubs being the same.

On the other hand, The mockery refers to the same definition as Uncle Bob, referring to all mocks. To make things clear, let’s split the anti pattern in two parts, the first, is the excessive number of mocks needed to test a class, for example - it’s also related to the Excessive Setup.

The second part is, testing the mock instead of its interaction with the code under testing. For example, if we have the following code, that is an interaction with a payment gateway.

/**
 * Two constructor dependencies, both need to be
 * mocked in order to test the process method.
 */
class PaymentService(
    private val userRepository: UserRepository,
    private val paymentGateway: PaymentGateway
) {

    fun process(
      user: User,
      paymentDetails: PaymentDetails
    ): Boolean {
        if (userRepository.exists(user)) {
            return paymentGateway.pay(paymentDetails)
        }

        return false
    }
}

With the given test:

class TestPaymentService {
    private val userRepository: UserRepository = mockk()
    private val paymentGateway: PaymentGateway = mockk()
    private val paymentService = PaymentService(
      userRepository,
      paymentGateway
    )

    @Test
    fun paymentServiceProcessPaymentForUser() {
        val user: User = User() 
        every { userRepository.exists(any()) } returns true
        every { paymentGateway.pay(any()) } returns true          // setting up the return for the mock

        assertTrue(paymentService.process(user, PaymentDetails())) // asserting the mock
    }
}

Root causes

Historic reasons / TDD styles?
As simple as the code is, the easiest is to overuse it
Also, no experience in TDD

The inspector

Often, the purpose of testing is mixed with “inspection”, this time I will focus on the idea of inspecting instead of checking for behavior (the output). For example, some times while testing, it gets a bit tricker as we would like to verify if given an input X we will receive the outcome Y, in the following snippet there is the method getFilesToWriteRelease and setFilesToWriteRelease, if we think about the context of the class why such methods exists?

<?php

class Assembly
{
    /* skipped code */

    public function __construct(
        FindVersion $findVersion,
        FileRepository $fileRepository,
        string $branchName,
        FilesToReleaseRepository $filesToReleaseRepository
    ) {
        $this->findVersion = $findVersion;
        $this->fileRepository = $fileRepository;
        $this->branchName = $branchName;
        $this->filesToReleaseRepository = $filesToReleaseRepository;
    }

    public function getFilesToWriteRelease(): array
    {
        return $this->filesToWriteRelease;
    }

    public function setFilesToWriteRelease(array $filesToWriteRelease)
    {
        $this->filesToWriteRelease = $filesToWriteRelease;
        return $this;
    }

    public function packVersion(): Release
    {
        $filesToRelease = $this->getFilesToWriteRelease();

        if (count($filesToRelease) === 0) {
            throw new NoFilesToRelease();
        }

        $files = [];

        /** @var File $file */
        foreach ($filesToRelease as $file) {
            $files[] = $this->fileRepository->findFile(
                $this->findVersion->getProjectId(),
                sprintf('%s%s', $file->getPath(), $file->getName()),
                $this->branchName
            );
        }

        $versionToRelease = $this->findVersion->versionToRelease();

        $release = new Release();
        $release->setProjectId($this->findVersion->getProjectId());
        $release->setBranch($this->branchName);
        $release->setVersion($versionToRelease);

        $fileUpdater = new FilesUpdater($files, $release, $this->filesToReleaseRepository);
        $filesToRelease = $fileUpdater->makeRelease();

        $release->setFiles($filesToRelease);

        return $release;
    }
}

In this sense, if we think about Object oriented programming, we talk about invoking methods in objects. Such invocations allow us to interact and receive the result, but, the get/set is breaking the encapsulation just for the sake of testing.

Another possible reason for experience the inspector is to start tricking too much to achieve a specific part of the code with reflection. Luckily, a few years back, we had this question asked at stackoverflow. Is that bad having to use reflection? Before proceed to this question, let’s do a quick recap on what is reflection and why is it used in the first place?

Baeldung gives a few examples on why you might need reflection, the main usage of it, is to get understanding of a given part of the code, which methods, which properties and act on those. The following example is used on his site:

public class Employee {
    private Integer id;
    private String name;
}

@Test
public void whenNonPublicField_thenReflectionTestUtilsSetField() {
    Employee employee = new Employee();
    ReflectionTestUtils.setField(employee, "id", 1);
 
    assertTrue(employee.getId().equals(1));
}

Some assumptions in the answers that points using reflection like that as a bad practice:

Modifying production code for sake of testing is a smell for something wrong in the design

The Generous leftovers

While practicing TDD, tasks such as setting up the state in which the test will run is part of the basics. Settings fake data, listeners, authentication, whatever it might be, we define them, because they are crucial for the test, but sometimes we forget to reset the state to be how it was before the test - [3] also mentions the xUnit phase in which such preparation happens: “setup, exercise, verify, teardown”.

It can cause different issues, and the first one is failing the next test, that was supposed to run without problems, the following list, tries to depict a few scenarios in which it might happen.

Setting up listeners and forget to remove them, it might also cause memory leaks
Populating data without removing them - things like files, database, or even cache
Last but not least, depending on one testing creating the data needed and using in another test
Cleaning up mocks

If we think about 3, such behavior would be something that gets blurry when testing, for example, using persistent data is a must for end-2-end testing. On the other hand, 4 is a common source of errors while testing guided by tests. Often, as the mock is usually used to collect calls in the object (and verify it later), is common to forget to restore it’s state.

Codingwithhugo depicts this behavior in a code snippet using jest, giving the following test case (and assume they are in the same scope):

const mockFn = jest.fn(); // setting up the mock

function fnUnderTest(args1) {
  mockFn(args1);
}

test('Testing once', () => {
  fnUnderTest('first-call');
  expect(mockFn).toHaveBeenCalledWith('first-call');
  expect(mockFn).toHaveBeenCalledTimes(1);
});

test('Testing twice', () => {
  fnUnderTest('second-call');
  expect(mockFn).toHaveBeenCalledWith('second-call');
  expect(mockFn).toHaveBeenCalledTimes(1);
});

The first test that calls the function under test will pass, but the second, will fail. The reason behind is the lack of reset in the mock. The test fails saying that the mockFn was called twice. Getting the flow as it should is as easy as:

test('Testing twice', () => {
  mockFn.mockClear();              // clears the previous execution

  fnUnderTest('second-call');
  expect(mockFn).toHaveBeenCalledWith('second-call');
  expect(mockFn).toHaveBeenCalledTimes(1);
});

Such behavior is not easily faced by javascript developers, as the javascript functional scope prevents that out of the box. Anyways, being mindful of this details can make difference while writing tests.

The local hero

The TDD anti-patterns precedes the container era, which was common to have differences between the machine in which the developer was working on and the server in which the application actually would run - often, as they weren’t the same, configuration specific to the developer machine got in the way during the deployment process.

PHP for example, relies heavily on extensions that can or can’t be enabled in the server. Extensions such as threads, drivers for connecting to a database and many more.

In this case, if the developer relied on a specific version for a given extension, the test would actually run successfully, but as soon as we try to run the suite in another machine it would fail - or in the Continuous Integration server.

Not only that, environment variables can get into the ways as well, for example, the following code depicts a component that needs a url to load the survey (some of the code has been removed/modified intentionally and adapted to fit the example - for more info, follow the github link):

import { Component } from 'react';
import Button from '../../buttons/primary/Primary';

import '../../../../scss/shake-horizontal.scss';
import './survey.scss';

const config = {
  surveyUrl: process.env.REACT_APP_SURVEY_URL || '',
}

const survey = config.surveyUrl; 

const mapStateToProps = state => ({
  user: state.userReducer.user,
});

export class Survey extends Component {
  /* skipped code */

  componentDidMount = () => { /* skipped code */}

  onSurveyLoaded = () => { /* skipped code */}

  skipSurvey = () => { /* skipped code */}

  render() {
    if (this.props.user.uid && survey) {
      return (
        <div className={`w-full ${this.props.className}`}>
          {
            this.state.loading &&
            <div className="flex justify-center items-center text-white">
              <h1>Carregando questionário...</h1>
            </div>
          }

          <iframe
            src={this.state.surveyUrl}
            title="survey form"
            onLoad={this.onSurveyLoaded}
          />

          {
            !this.state.loading && this.props.skip &&
            <Button
              className="block mt-5 m-auto"
              description={this.state.buttonDescription}
              onClick={this.skipSurvey}
            />
          }
        </div>
      );
    }

    return (
      <div className="flex justify-center items-center text-white">
        <h1 className="shake-horizontal">Ocorreu um erro ao carregar o questionário</h1>
      </div>
    );
  }
}

/* skipped code */

And here goes the test case for this components:

import { mount } from 'enzyme';
import { Survey } from './Survey';
import { auth } from '../../../../pages/login/Auth';
import Button from '../../buttons/primary/Primary';

describe('Survey page', () => {

  test('should show up message when survey url is not defined',() => {
    const wrapper = mount(<Survey user= />);
    const text = wrapper.find('h1').text();
    expect(text).toEqual('Carregando questionário...');
  });

  test('should not load survey when user id is missing', () => {
    const wrapper = mount(<Survey user= />);
    const text = wrapper.find('h1').text();
    expect(text).toEqual('Ocorreu um erro ao carregar o questionário');
  });

  test('load survey passing user id as a parameter in the query string', () => {
    const user = { uid: 'uhiuqwqw-k-woqk-wq--qw' };

    const wrapper = mount(<Survey user={user} />);
    const url = wrapper.find('iframe').prop('src');
    expect(url.includes(auth.user.uid)).toBe(true);
  });

  test('should not up button when it is loading', () => {
    const user = { uid: 'uhiuqwqw-k-woqk-wq--qw' };

    const wrapper = mount(<Survey user={user} />);
    expect(wrapper.find(Button).length).toBe(0);
  });

  test('should not up button when skip prop is not set', () => {
    const user = { uid: 'uhiuqwqw-k-woqk-wq--qw' };

    const wrapper = mount(<Survey user={user} />);
    expect(wrapper.find(Button).length).toBe(0);
  });

  test('show up button when loading is done and skip prop is true', () => {
    const user = { uid: 'uhiuqwqw-k-woqk-wq--qw' };

    const wrapper = mount(<Survey user={user} skip={true} />);
    wrapper.setState({
      loading: false
    });
    expect(wrapper.find(Button).length).toBe(1);
  });
});

Despite of the code age (long time class components in react), it seems to do the job pretty well. Deriving the behavior from the test cases, we can understand that some loading is going on based on the survey URL and the user id. Unfortunately, the details of the implementation is what matters the most - if we run the test case for the current implementation, it will fail.

Test Suites: 1 failed, 62 passed, 63 total
Tests:       3 failed, 593 passed, 596 total

And the fix for such a run, is to export an environment variable named REACT_APP_SURVEY_URL. Well, the easy fix would be to use the env variable. The long term fix, would be to avoid depending on the external definition and assume some defaults, here are some ideas that come to my mind to fix that properly:

Assume a dummie variable as default
Do not use any URL at all and build the tests around having it or not - if not, just skip the execution

Another example, would be relying on the real files, this issue is also discussed in a stack overflow thread. The issue that comes in such dependent test, is: the test would run only in a windows machine. Ideally external dependencies should be avoided using test doubles.

Root causes

Dependencies used for development does not take into account other places to run - this is also an issue for applications that weren’t build with cloud in mind
Relying on the operating system dependencies
Relying on the file system

References

[1]M. Fowler, “TestDouble,” 2022 [Online]. Available at: https://martinfowler.com/bliki/TestDouble.html. [Accessed: 17-Jan-2006]
[2]R. C. M. (U. Bob), “The Little Mocker,” 2022 [Online]. Available at: https://blog.cleancoder.com/uncle-bob/2014/05/14/TheLittleMocker.html. [Accessed: 14-May-2014]
[3]M. Fowler, “Mocks Aren’t Stubs,” 2022 [Online]. Available at: https://martinfowler.com/articles/mocksArentStubs.html. [Accessed: 02-Jan-2007]