Refactoring the Cat API client - Part II
Matthias Noback
The world is not a safe thing to depend upon
When you’re running unit tests, you don’t want the world itself to be involved. Executing actual database queries, making actual HTTP requests, writing actual files, none of this is desirable: it will make your tests very slow as well as unpredictable. If the server you’re sending requests to is down, or responds in unexpected ways, your unit test will fail for the wrong reasons. A unit test should only fail if your code doesn’t do what it’s supposed to do.
As we saw in the previous part of this series, both the CachedCatApi and the RealCatApi classes are in some way dependent on the world. The first writes actual files to the filesystem, the second makes actual HTTP requests. Besides, creating files and making HTTP requests is pretty low-level stuff, for which these classes currently don’t use the best tools available. Also, a lot of edge-cases have not been taken care of.
Both classes can be freed from their dependence on the world, simply by letting some new classes encapsulate the low-level details. For example, we can easily move the call to file_get_contents() to another class, called
FileGetContentsHttpClient:
class FileGetContentsHttpClient
{
public function get($url)
{
return @file_get_contents($url);
}
}
Dependency inversion again
Just like in the previous article, extracting some code into a new class won’t suffice. We need to introduce an interface for the new class, or we won’t be able to easily create a stand in for it in our tests:
interface HttpClient
{
/**
* @return string|false Response body
*/
public function get($url);
}
Now we can provide an HttpClient as a constructor argument of RealCatApi:
class RealCatApi implements CatAPi
{
private $httpClient;
public function __construct(HttpClient $httpClient)
{
$this->httpClient = $httpClient;
}
public function getRandomImage()
{
$responseXml = $this->httpClient->get('https://thecatapi.com/api/images/get?format=xml&type=jpg');
...
}
}
A true unit test
For the first time we can now have a true unit test for RealCatApi. We only need to provide a stand-in for HttpClient which just returns a predefined XML response body:
class RealCatApiTest extends \PHPUnit_Framework_TestCase
{
/** @test */
public function it_fetches_a_random_url_of_a_cat_gif()
{
$xmlResponse = <<<EOD
<response>
<data>
<images>
<image>
<url>https://24.media.tumblr.com/tumblr_lgg3m9tRY41qgnva2o1_500.jpg</url>
<id>bie</id>
<source_url>https://thecatapi.com/?id=bie</source_url>
</image>
</images>
</data>
</response>
EOD;
$httpClient = $this->getMock('HttpClient');
$httpClient
->expect($this->once())
->method('get')
->with('https://thecatapi.com/api/images/get?format=xml&type=jpg')
->will($this->returnValue($xmlResponse));
$catApi = new RealCatApi($httpClient);
$url = $catApi->getRandomImage();
$this->assertSame(
'https://24.media.tumblr.com/tumblr_lgg3m9tRY41qgnva2o1_500.jpg',
$url
);
}
}
We have a proper test now, which verifies the correctness of RealCatApi’s behavior: it should call the HttpClient with a specific URL and return just the value of the <url> element from the XML response body.
Decoupling our API from file_get_contents()
One issue still remains to be fixed right now: the contract of HttpClient::get() still adheres to the contract of file_get_contents() in that it returns false when the request failed, or the response body (as a string) when it succeeded. We can nicely hide this implementation detail by converting any specific return value like false into a custom exception. This way, the only things that travel across the object boundary are a function argument, a string return value, or an exception:
class FileGetContentsHttpClient implements HttpClient
{
public function get($url)
{
$response = @file_get_contents($url);
if ($response === false) {
throw new HttpRequestFailed();
}
return $response;
}
}
interface HttpClient
{
/**
* @return string Response body
* @throws HttpRequestFailed
*/
public function get($url);
}
class HttpRequestFailed extends \RuntimeException
{
}
We only need to modify RealCatApi a bit to catch the exception, instead of responding to false:
class RealCatApi implements CatAPi
{
public function getRandomImage()
{
try {
$responseXml = $this->httpClient->get('https://thecatapi.com/api/images/get?format=xml&type=jpg');
...
} catch (HttpRequestFailed $exception) {
return 'https://cdn.my-cool-website.com/default.jpg';
}
...
}
}
Did you notice that previously we only had a unit test for the happy path? We only tested the result when file_get_contents() gave us a proper XML response body. We weren’t in the position to test for a failing HTTP request since, well, how would you “reliably” cause an HTTP request to fail, except by unplugging your network cable?
Since we now have full control over the HttpClient, we can simulate a failing HTTP request, simply by throwing an HttpRequestFailed exception:
class RealCatApiTest extends \PHPUnit_Framework_TestCase
{
...
/** @test */
public function it_returns_a_default_url_when_the_http_request_fails()
{
$httpClient = $this->getMock('HttpClient');
$httpClient
->expect($this->once())
->method('get')
->with('https://thecatapi.com/api/images/get?format=xml&type=jpg')
->will($this->throwException(new HttpRequestFailed());
$catApi = new RealCatApi($httpClient);
$url = $catApi->getRandomImage();
$this->assertSame(
'https://cdn.my-cool-website.com/default.jpg',
$url
);
}
}
Removing the filesystem dependency
We can repeat the same steps for CachedCatApi’s dependency on the filesystem:
interface Cache
{
public function isNotFresh($lifetime);
public function put($url);
public function get();
}
class FileCache implements Cache
{
private $cacheFilePath;
public function __construct()
{
$this->cacheFilePath = __DIR__ . '/../../cache/random';
}
public function isNotFresh($lifetime)
{
return !file_exists($this->cacheFilePath)
|| time() - filemtime($this->cacheFilePath) > $lifetime
}
public function put($url)
{
file_put_contents($this->cacheFilePath, $url);
}
public function get()
{
return file_get_contents($this->cacheFilePath);
}
}
class CachedCatApi implements CatApi
{
...
private $cache;
public function __construct(CatApi $realCatApi, Cache $cache)
{
...
$this->cache = $cache;
}
public function getRandomImage()
{
if ($this->cache->isNotFresh()) {
...
$this->cache->put($url);
return $url;
}
return $this->cache->get();
}
}
Finally, finally, we can remove those ugly sleep() statements from the CachedCatApiTest as well, since can use a simple stand-in for Cache. I leave this part of the exercise to the imagination of the reader.
There are some issues left here:
- I don’t like the API of
Cache.Cache::isNotFresh()is hard to wrap your head around. It also doesn’t correspond to existing cache abstractions like the one from Doctrine, which makes it harder to follow for people familiar with caching things in PHP. - The cache file path is still hard-coded in the
FileCacheclass. This is bad for testing since you can’t change it for testing purposes.
The first issue can be solved by renaming some methods and inverting some of the boolean logic. The second issue can be solved by simply injecting the path as a constructor argument.
Conclusion
In this part we’ve hidden a lot of the low-level details about file handling and HTTP requests from sight. This allowed us to turn our tests into proper unit tests.
Of course, the code in FileCache and FileGetContentsHttpClient still needs to be tested. So you will still end up with some slow and fragile tests. There’s one very helpful thing you can do: push these tests out of your own project by using existing solutions for doing filesystem manipulations and making HTTP requests. The burden of testing the correctness of that code is upon the shoulders of the maintainers of these libraries. This basically allows you to focus on the important parts of your code and will keep your unit test suite very fast.