Introduction.
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();
}
}
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;
}
}
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].
The problem.
In this particular case, however, implementing the QueryStringBuilder following this typical recipe causes a lot of problems. Looking at the test code, you may 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());
}
///......
So far so good, the problem comes when you change the production code to make this test pass, in that moment 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();
}
}
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 "negative builder".
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();
}
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;
}
}
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());
}
///......
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.
Conclusions.
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. Thanks also to Markus Spiske for the photo used in this post and to Pexels.
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.
Top comments (0)