Table of contents
TDD anti-patterns - episode 2 - the mockery, the inspector, the generous leftovers and the local hero - with code examples in javascript, kotlin and php
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-patterns 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 its own naming and categories that make it unique and at the same time tricky to use them correctly.
For example, (Fowler, 2022) and (Bob), 2022) classify the mocks family into 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” spread as it is easier to say “I will mock that” or “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 is important because we, as developers, have the habit of calling everything a mock, which can sometimes be misleading about what we mean. Spring.io goes even further, avoiding the debate about whether we should use mocks or not (referring to the Classical and the London school of TDD). I will do the same, as this is a discussion that deserves a blog post of its own. Martin Fowler also wrote about this very same subject (Fowler, 2022) 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 into two parts. The first is the excessive number of mocks needed to test a class, for example. This is also related to the Excessive Setup.
The second part is testing the mock instead of its interaction with the code under test. For example, if we have the following code that represents 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 experiencing the inspector is to start using too many tricks to achieve coverage of a specific part of the code with reflection. Luckily, a few years back, we had this question asked at StackOverflow. Is it bad to use reflection? Before proceeding to this question, let’s do a quick recap on what reflection is and why it is used in the first place.
Baeldung gives a few examples of why you might need reflection. The main usage of it is to gain understanding of a given part of the code, which methods, which properties, and to act on those. The following example is used on their 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 point to using reflection like that as a bad practice:
- Modifying production code for the 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 are part of the basics. Setting up fake data, listeners, authentication, or whatever it might be, we define them because they are crucial for the test. However, sometimes we forget to reset the state to how it was before the test. (Fowler, 2022) 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 item 3, such behavior becomes blurry when testing. For example, using persistent data is a must for end-to-end testing. On the other hand, item 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), it is common to forget to restore its 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 this is the lack of reset in the mock. The test fails saying that the mockFn was called twice. Getting the flow as it should be 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 these details can make a difference while writing tests.
The local hero
The TDD anti-patterns precede the container era, when it was common to have differences between the machine on which the developer was working and the server on which the application would actually run. Often, as they were not 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 cannot be enabled on 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 on another machine, it would fail, or fail on the Continuous Integration server.
Not only that, but environment variables can also get in the way. 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 component:
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 the code’s 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 are what matter 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 dummy 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 real files. This issue is also discussed in a Stack Overflow thread. The issue that comes with such a dependent test is that the test would run only on a Windows machine. Ideally, external dependencies should be avoided using test doubles.
Root causes
- Dependencies used for development do not take into account other places to run. This is also an issue for applications that were not built with cloud in mind
- Relying on the operating system dependencies
- Relying on the file system
References
- Fowler, M. (2022). TestDouble. https://martinfowler.com/bliki/TestDouble.html
- Bob), R. C. M. (U. (2022). The Little Mocker. https://blog.cleancoder.com/uncle-bob/2014/05/14/TheLittleMocker.html
- Fowler, M. (2022). Mocks Aren’t Stubs. https://martinfowler.com/articles/mocksArentStubs.html
Appendix
Edit Jan 21, 2021 - Codurance talk
Presenting the tdd anti-patterns at Codurance talk.