The curious case of the negative builder

Recently, one of the teams I’m coaching at my current client, asked me to help them with a problem, they were experiencing while using TDD to add and validate new mandatory query string parameters^[1]. This is a shortened version (validating fewer parameters than the original code) of the tests they were having problems with:

	<?php
	use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
	class DefaultControllerTest extends WebTestCase
	{
	/** @test */
	public function should_returns_200_when_all_mandatory_parameters_are_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("us")
	->withVertical(1)
	->withBrand("someBrand")->build()
	);
	$this->assertEquals(200, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_brand_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("us")
	->withVertical(1)->build()
	);
	$this->assertEquals(400, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_country_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET', $this->queryString
	->withBrand("someBrand")
	->withVertical(2)->build()
	);
	$this->assertEquals(400, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_vertical_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("es")
	->withBrand("someBrand")->build()
	);
	$this->assertEquals(400, $client->getResponse()->getStatusCode());
	}
	function queryString(): QueryStringBuilder
	{
	return new QueryStringBuilder();
	}
	}

view raw preRefactorControllerTest.php hosted with ❤ by GitHub

and this is the implementation of the QueryStringBuilder used in this test:

	<?php
	class QueryStringBuilder
	{
	const CONNECTOR = '&';
	const START = '/?';
	private $query;
	public function build(): string
	{
	return self::START . $this->query;
	}
	public function withBrand($brand): QueryStringBuilder
	{
	$this->query .= "brand={$brand}" . self::CONNECTOR;
	return $this;
	}
	public function withCountry($country): QueryStringBuilder
	{
	$this->query .= "country={$country}" . self::CONNECTOR;
	return $this;
	}
	public function withVertical($vertical): QueryStringBuilder
	{
	$this->query .= "vertical={$vertical}" . self::CONNECTOR;
	return $this;
	}
	}

view raw preRefactorQueryStringBuilder.php hosted with ❤ by GitHub

which is a builder with a fluid interface that follows to the letter a typical implementation of the pattern. There are even libraries that help you to automatically create this kind of builders^[2].

However, in this particular case, implementing the QueryStringBuilder following this typical recipe causes a lot of problems. Looking at the test code, you can see why.

To add a new mandatory parameter, for example sourceId, following the TDD cycle, you would first write a new test asserting that a query string lacking the parameter should not be valid.

	<?php
	///......
	/** @test */
	public function should_returns_400_when_source_id_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("us")
	->withVertical(1)
	->withBrand("someBrand")->build()
	);
	$this->assertEquals(200, $client->getResponse()->getStatusCode());
	}
	///......

view raw preRefactorNewParameterTestCase.php hosted with ❤ by GitHub

So far so good, the problem comes when you change the production code to make this test pass, in that momento you’ll see how the first test that was asserting that a query string with all the parameters was valid starts to fail (if you check the query string of that tests and the one in the new test, you’ll see how they are the same). Not only that, all the previous tests that were asserting that a query string was invalid because a given parameter was lacking won’t be “true” anymore because after this change they could fail for more than one reason.

So to carry on, you’d need to fix the first test and also change all the previous ones so that they fail again only for the reason described in the test name:

	<?php
	use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
	class DefaultControllerTest extends WebTestCase
	{
	/** @test */
	public function should_returns_200_when_all_mandatory_parameters_are_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("us")
	->withVertical(1)
	->withBrand("someBrand")
	->withSourceId(5)->build()
	);
	$this->assertEquals(200, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_brand_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("us")
	->withVertical(1)
	->withSourceId(5)->build()
	);
	$this->assertEquals(400, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_country_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withBrand("someBrand")
	->withVertical(2)
	->withSourceId(5)->build()
	);
	$this->assertEquals(400, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_vertical_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("es")
	->withBrand("someBrand")
	->withSourceId(5)->build()
	);
	$this->assertEquals(400, $client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_returns_400_when_source_id_is_not_present()
	{
	$client = static::createClient();
	$client->request(
	'GET',
	$this->queryString
	->withCountry("us")
	->withVertical(1)
	->withBrand("someBrand")->build()
	);
	$this->assertEquals(200, $client->getResponse()->getStatusCode());
	}
	function queryString(): QueryStringBuilder
	{
	return new QueryStringBuilder();
	}
	}

view raw preRefactorControllerTestWithNewParameter.php hosted with ❤ by GitHub

That’s a lot of rework on the tests only for adding a new parameter, and the team had to add many more. The typical implementation of a builder was not helping them.

The problem we’ve just explained can be avoided by chosing a default value that creates a valid query string and what I call “a negative builder”, a builder with methods that remove parts instead of adding them. So we refactored together the initial version of the tests and the builder, until we got to this new version of the tests:

	<?php
	use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
	class DefaultControllerTest extends WebTestCase
	{
	private $client;
	protected function setUp()
	{
	$this->client = static::createClient();
	parent::setUp();
	}
	/** @test */
	public function should_succeed_when_all_mandatory_parameters_are_present()
	{
	$this->client->request('GET', queryString()->build());
	$this->assertEquals(200, $this->client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_fail_when_brand_is_not_present()
	{
	$this->client->request(
	'GET',
	queryString()->withoutBrand()->build()
	);
	$this->assertEquals(400, $this->client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_fail_when_country_is_not_present()
	{
	$this->client->request(
	'GET',
	queryString()->withoutCountry()->build()
	);
	$this->assertEquals(400, $this->client->getResponse()->getStatusCode());
	}
	/** @test */
	public function should_fail_when_vertical_is_not_present()
	{
	$this->client->request(
	'GET',
	queryString()->withoutVertical()->build()
	);
	$this->assertEquals(400, $this->client->getResponse()->getStatusCode());
	}
	}
	function queryString(): QueryStringBuilder
	{
	return QueryStringBuilder::valid();
	}

view raw postRefactorControllerTest.php hosted with ❤ by GitHub

which used a “negative” QueryStringBuilder:

	<?php
	class QueryStringBuilder
	{
	const CONNECTOR = '&';
	const START = '/?';
	private $brand;
	private $vertical;
	private $country;
	public static function valid()
	{
	$builder = new QueryStringBuilder();
	$builder->brand = "1";
	$builder->vertical = "2";
	$builder->country = "es";
	return $builder;
	}
	public function build()
	{
	return self::START .
	$this->addParameter("brand", $this->brand) .
	$this->addParameter("vertical", $this->vertical) .
	$this->addParameter("country", $this->country);
	}
	public function withoutBrand()
	{
	$this->brand = null;
	return $this;
	}
	public function withoutCountry()
	{
	$this->country = null;
	return $this;
	}
	public function withoutVertical()
	{
	$this->vertical = null;
	return $this;
	}
	private function addParameter($name, $value): string
	{
	if (is_null($value)) {
	return "";
	}
	return "{$name}={$value}" . self::CONNECTOR;
	}
	}

view raw postRefactorQueryStringBuilder.php hosted with ❤ by GitHub

After this refactoring, to add the sourceId we wrote this test instead:

	<?php
	///......
	/** @test */
	public function should_returns_400_when_source_id_is_not_present()
	{
	$this->client->request(
	'GET',
	queryString()->withoutSourceId()->build()
	);
	$this->assertEquals(400, $this->client->getResponse()->getStatusCode());
	}
	///......

view raw postRefactorNewParameterTestCase.php hosted with ❤ by GitHub

which only carries with it updating the valid method in QueryStringBuilder and adding a method that removes the sourceId parameter from a valid query string.

Now when we changed the code to make this last test pass, no other test failed or started to have descriptions that were not true anymore.

Leaving behind the typical recipe and adapting the idea of the builder pattern to the context of the problem at hand, led us to a curious implementation, a “negative builder”, that made the tests easier to maintain and improved our TDD flow.

Acknowledgements.

Many thanks to my Codesai colleagues Antonio de la Torre and Fran Reyes, and to all the colleagues of the Prime Services Team at LIFULL Connect for all the mobs we enjoy together.

Footnotes:

[1] Currently, this validation is not done in the controller anymore. The code showed above belongs to a very early stage of an API we're developing.

[2] Have a look, for instance, at lombok's' @Builder annotation for Java.

Killing mutants to improve your tests

At my current client we’re working on having a frontend architecture for writing SPAs in JavaScript similar to re-frame’s one: an event-driven bus with effects and coeffects for state management^[1] (commands) and subscriptions using reselect’s selectors (queries).

One of the pieces we have developed to achieved that goal is reffects-store. Using this store, React components can be subscribed to given reselect’s selectors, so that they only render when the values in the application state tracked by the selectors change.

After we finished writing the code for the store, we decided to use mutation testing to evaluate the quality of our tests. Mutation testing is a technique in which, you introduce bugs, (mutations), into your production code, and then run your tests for each mutation. If your tests fail, it’s ok, the mutation was “killed”, that means that they were able to defend you against the regression caused by the mutation. If they don’t, it means your tests are not defending you against that regression. The higher the percentage of mutations killed, the more effective your tests are.

There are tools that do this automatically, stryker^[2] is one of them. When you run stryker, it will create many mutant versions of your production code, and run your tests for each mutant (that’s how mutations are called in stryker’s’ documentation) version of the code. If your tests fail then the mutant is killed. If your tests passed, the mutant survived. Let’s have a look at the the result of runnning stryker against reffects-store’s code:

	$ npm run test:mutants
	> reffects-store@0.1.0 test:mutants /home/trikitrok/todos-spike/reffects-store
	> stryker run
	21:38:12 (4489) INFO ConfigReader Using stryker.conf.js in the current working directory.
	21:38:12 (4489) INFO InputFileResolver Found 3 of 13 file(s) to be mutated.
	21:38:12 (4489) INFO InitialTestExecutor Starting initial test run. This may take a while.
	21:38:15 (4489) INFO InitialTestExecutor Initial test run succeeded. Ran 23 tests in 2 seconds (net 125 ms, overhead 2331 ms).
	21:38:15 (4489) INFO MutatorFacade 41 Mutant(s) generated
	21:38:15 (4489) INFO SandboxPool Creating 8 test runners (based on CPU count)
	Mutation testing [==================================================] 100% (ETC n/a) 41/41 tested (6 survived)
	15. [Survived] Block
	/home/trikitrok/todos-spike/reffects-store/src/store.js:38:63
	- listeners.forEach(function iterateListeners(currentListener) {
	- if (currentListener === listener) {
	- listener = null;
	- } else {
	- out.push(currentListener);
	- }
	- });
	+ listeners.forEach(function iterateListeners(currentListener) {});
	Ran all tests for this mutant.
	18. [Survived] IfStatement
	/home/trikitrok/todos-spike/reffects-store/src/store.js:39:8
	- if (currentListener === listener) {
	+ if (true) {
	Ran all tests for this mutant.
	19. [Survived] BinaryExpression
	/home/trikitrok/todos-spike/reffects-store/src/store.js:39:8
	- if (currentListener === listener) {
	+ if (currentListener !== listener) {
	Ran all tests for this mutant.
	20. [Survived] Block
	/home/trikitrok/todos-spike/reffects-store/src/store.js:39:38
	- if (currentListener === listener) {
	- listener = null;
	- } else {
	+ if (currentListener === listener) {} else {
	Ran all tests for this mutant.
	21. [Survived] Block
	/home/trikitrok/todos-spike/reffects-store/src/store.js:41:11
	- } else {
	- out.push(currentListener);
	- }
	+ } else {}
	Ran all tests for this mutant.
	22. [Survived] Block
	/home/trikitrok/todos-spike/reffects-store/src/store.js:53:33
	- export function unsubscribeAll() {
	- listeners = [];
	- }
	+ export function unsubscribeAll() {}
	Ran all tests for this mutant.
	Ran 15.27 tests per mutant on average.
	------------------|---------|----------|-----------|------------|----------|---------|
	File | % score | # killed | # timeout | # survived | # no cov | # error |
	------------------|---------|----------|-----------|------------|----------|---------|
	All files | 85.37 | 35 | 0 | 6 | 0 | 0 |
	store.js | 75.00 | 18 | 0 | 6 | 0 | 0 |
	storeUtils.js | 100.00 | 4 | 0 | 0 | 0 | 0 |
	subscriptions.js | 100.00 | 13 | 0 | 0 | 0 | 0 |
	------------------|---------|----------|-----------|------------|----------|---------|
	21:38:27 (4489) INFO HtmlReporter Your report can be found at: file:///home/trikitrok/todos-spike/reffects-store/reports/mutation/html/index.html
	21:38:27 (4489) INFO Stryker Done in 14 seconds.

view raw runningMutantsResultWithSurvivors.sh hosted with ❤ by GitHub

Notice how stryker shows the details of every mutation that survived our tests, and look at the summary the it produces at the end of the process.

All the surviving mutants were produced by mutations to the store.js file. Having a closer look to the mutations in stryker’s output we found that the functions with mutant code were unsubscribeAllListeners and unsubscribeListener. After a quick check of their tests, it was esay to find out why unsubscribeAllListeners was having surviving mutants. Since it was a function we used only in tests for cleaning the state after each test case was run, we had forgotten to test it.

However, finding out why unsubscribeListener mutants were surviving took us a bit more time and thinking. Let’s have a look at the tests that were exercising the code used to subscribe and unsubscribe listeners of state changes:

	describe('subscriptions to store changes', () => {
	test('subscribing to store changes', () => {
	const newValue = 'lolo';
	const path = ['koko'];
	store.initialize({ koko: 'loko' });
	store.subscribeListener(function(newState) {
	expect(newState).toEqual({ koko: newValue });
	});
	store.setState({ path, newValue });
	});
	test('unsubscribing from store changes', () => {
	const newValue = 'lolo';
	const path = ['koko'];
	store.initialize({ koko: 'loko' });
	function functionToUnsubscribe() {
	throw new Error('check is still suscribed!!');
	}
	store.subscribeListener(function(newState) {
	expect(newState).toEqual({ koko: newValue });
	});
	store.subscribeListener(functionToUnsubscribe);
	store.unsubscribeListener(functionToUnsubscribe);
	store.setState({ path, newValue });
	});
	});

view raw testsWithSurvivingMutants.js hosted with ❤ by GitHub

If we examine the mutations and the tests, we can see that the tests for unsubscribeListener are not good enough. They are throwing an exception from the subscribed function we unsubscribe, so that if the unsubscribeListener function doesn’t work and that function is called the test fails. Unfortunately, the test passes also if that function is never called for any reason. In fact, most of the surviving mutants that stryker found above have are variations on that idea.

A better way to test unsubscribeListener is using spies to verify that subscribed functions are called and unsubscribed functions are not (this version of the tests includes also a test for unsubscribeAllListeners):

	describe('subscriptions to store changes', () => {
	test('subscribing a listener to store changes', () => {
	const newValue = 'lolo';
	const path = ['koko'];
	store.initialize({ koko: 'loko' });
	const fn = jest.fn(newState =>
	expect(newState).toEqual({ koko: newValue })
	);
	store.subscribeListener(fn);
	store.setState({ path, newValue });
	expect(fn).toHaveBeenCalledTimes(1);
	});
	test('unsubscribing a listener from store changes', () => {
	const newValue = 'lolo';
	const path = ['koko'];
	store.initialize({ koko: 'loko' });
	const spyToBeUnsubscribed = jest.fn();
	const spyNotUnsubscribed = jest.fn();
	store.subscribeListener(spyNotUnsubscribed);
	store.subscribeListener(spyToBeUnsubscribed);
	store.unsubscribeListener(spyToBeUnsubscribed);
	store.setState({ path, newValue });
	expect(spyToBeUnsubscribed).toHaveBeenCalledTimes(0);
	expect(spyNotUnsubscribed).toHaveBeenCalledTimes(1);
	});
	test('unsubscribing all listeners from store changes', () => {
	const newValue = 'lolo';
	const path = ['koko'];
	store.initialize({ koko: 'loko' });
	const spyToBeUnsubscribed1 = jest.fn();
	const spyToBeUnsubscribed2 = jest.fn();
	store.subscribeListener(spyToBeUnsubscribed1);
	store.subscribeListener(spyToBeUnsubscribed2);
	store.unsubscribeAllListeners();
	store.setState({ path, newValue });
	expect(spyToBeUnsubscribed1).toHaveBeenCalledTimes(0);
	expect(spyToBeUnsubscribed2).toHaveBeenCalledTimes(0);
	});
	});

view raw testWithNoSurvivingMutant.js hosted with ❤ by GitHub

After this change, when we run stryker we got the following output:

	$ npm run test:mutants
	> reffects-store@0.1.4 test:mutants /home/trikitrok/todos-spike/reffects-store
	> stryker run
	21:49:18 (4981) INFO ConfigReader Using stryker.conf.js in the current working directory.
	21:49:18 (4981) INFO InputFileResolver Found 4 of 14 file(s) to be mutated.
	21:49:18 (4981) INFO InitialTestExecutor Starting initial test run. This may take a while.
	21:49:21 (4981) INFO InitialTestExecutor Initial test run succeeded. Ran 24 tests in 2 seconds (net 122 ms, overhead 2313 ms).
	21:49:21 (4981) INFO MutatorFacade 39 Mutant(s) generated
	21:49:21 (4981) INFO SandboxPool Creating 8 test runners (based on CPU count)
	Mutation testing [==================================================] 100% (ETC n/a) 39/39 tested (0 survived)
	Ran 15.74 tests per mutant on average.
	--------------|---------|----------|-----------|------------|----------|---------|
	File | % score | # killed | # timeout | # survived | # no cov | # error |
	--------------|---------|----------|-----------|------------|----------|---------|
	All files | 100.00 | 39 | 0 | 0 | 0 | 0 |
	subscription | 100.00 | 13 | 0 | 0 | 0 | 0 |
	index.js | 100.00 | 13 | 0 | 0 | 0 | 0 |
	store.js | 100.00 | 22 | 0 | 0 | 0 | 0 |
	utils.js | 100.00 | 4 | 0 | 0 | 0 | 0 |
	--------------|---------|----------|-----------|------------|----------|---------|
	21:49:32 (4981) INFO HtmlReporter Your report can be found at: file:///home/trikitrok/todos-spike/reffects-store/reports/mutation/html/index.html
	21:49:32 (4981) INFO Stryker Done in 13 seconds.

view raw runningMutantsResultWithNoSurvivors.sh hosted with ❤ by GitHub

No mutants survived!! This means this new version of the tests is more reliable and will protect us better from regressions than the initial version.

Mutation testing is a great tool to know if you can trust your tests. This is event more true when working with legacy code.

Acknowledgements.

Many thanks to Mario Sánchez and Alex Casajuana Martín for all the great time coding together, and thanks to Porapak Apichodilok for the photo used in this post and to Pexels.

Footnotes:

[1] See also reffects which is the synchronous event bus with effects and coeffects we wrote to manage the application state.

[2] The name of this tool comes from a fictional Marvel comics supervillain Willian Stryker who was obsessed with the eradication of all mutants.