Pussy: Refactoring. Part two or addiction treatment

This translation is a continuation of a series of articles on refactoring from Matthias Noback.

The world is not so reliable to rely on it

During unit testing there is no need for the external environment to be involved in the testing process itself. Making real queries to the database, HTTP requests or writing to files, you slow down the tests, because these operations are unpredictable. For example, if the server to which you make inquiries during testing has fallen or has not responded in the best way - the unit test will fall even if everything else works correctly. This is bad, as unit tests should fall only when the code does something that it should not do.

As you can see in the last article, both classes (CachedCatApi and RealCatApi) depend on external factors. The first one writes files to the file system, the second one makes real HTTP requests, while these moments are rather low-level and the right tools are not used for them. Moreover, these classes do not take into account a large number of borderline cases.
')
Both classes may be deprived of such dependencies and for this it is enough that the new classes encapsulate all these low-level details. For example, we can easily remove the call to file_get_contents () in another class called FileGetContentsHttpClient.

class FileGetContentsHttpClient { public function get($url) { return @file_get_contents($url); } } 

Again inversion of dependencies

As in the previous article, you can’t just take and render some code to another class. For the new class, you need to enter the interface, because without it it will be difficult to write a normal test:

 interface HttpClient { /** * @return string|false Response body */ public function get($url); } 

Now you can pass HttpClient as an argument to the constructor of RealCatApi:

 class RealCatApi implements CatAPi { private $httpClient; public function __construct(HttpClient $httpClient) { $this->httpClient = $httpClient; } public function getRandomImage() { $responseXml = $this->httpClient->get('http://thecatapi.com/api/images/get?format=xml&type=jpg'); ... } } 

Real Unit Test

From now on, we will have a really cool unit test for RealCatApi. It is only necessary to replace (stand-in?) HttpClient, so that he returned a predefined XML-response:

 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>http://24.media.tumblr.com/tumblr_lgg3m9tRY41qgnva2o1_500.jpg</url> <id>bie</id> <source_url>http://thecatapi.com/?id=bie</source_url> </image> </images> </data> </response> EOD; $httpClient = $this->getMock('HttpClient'); $httpClient ->expect($this->once()) ->method('get') ->with('http://thecatapi.com/api/images/get?format=xml&type=jpg') ->will($this->returnValue($xmlResponse)); $catApi = new RealCatApi($httpClient); $url = $catApi->getRandomImage(); $this->assertSame( 'http://24.media.tumblr.com/tumblr_lgg3m9tRY41qgnva2o1_500.jpg', $url ); } } 

Now this is the correct test that verifies the following RealCatApi behavior: it must call HttpClient with a specific URL and return the field value from the XML response.

Separate API from file_get_contents ()

There remains one more thing to fix: the get () method of the HttpClient class still depends on the behavior of file_get_contents (), that is, returns false if the request was unsuccessful, or the response body as a string if the request is successful. We can easily hide this implementation detail by converting some return values ​​(like false, for example) into exceptions defined for them (custom action). Thus, we strictly limit the number of processed entities that pass through our objects. In our case, this is only a function argument, a returned string 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 { } 

It remains to slightly change RealCatApi so that it can catch exceptions instead of reacting to false:

 class RealCatApi implements CatAPi { public function getRandomImage() { try { $responseXml = $this->httpClient->get('http://thecatapi.com/api/images/get?format=xml&type=jpg'); ... } catch (HttpRequestFailed $exception) { return 'http://cdn.my-cool-website.com/default.jpg'; } ... } } 

You noticed that before we only had a unit test of the correct address? We tested only the successful file_get_contents () result with a valid XML response. It was not possible to test a dropped HTTP request, since it is not clear how you can forcefully block the HTTP request, except by pulling the network cable?

Now we have full control over the HttpClient and we can simulate a request drop - for this you just need to throw the 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('http://thecatapi.com/api/images/get?format=xml&type=jpg') ->will($this->throwException(new HttpRequestFailed()); $catApi = new RealCatApi($httpClient); $url = $catApi->getRandomImage(); $this->assertSame( 'http://cdn.my-cool-website.com/default.jpg', $url ); } } 

Getting rid of the file system

We can repeat the same steps for the CachedCatApi file system dependency:

 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 get rid of these terrible sleep () calls in CachedCatApiTest! And all this is due to the fact that we have a simple wrapper for Cache. I will leave this part as an independent exercise for the reader.

There were several problems:

1. I do not like the Cache interface API. The isNotFresh () method is hard to understand. It also does not correspond to already existing abstractions (for example, that from Doctrine), which makes it incomprehensible for people familiar with caching in PHP.
2. The cache path is still hardcoded in the FileCache class. This is bad for testing - there is no way to change it.

The first can be solved by renaming some methods and inverting some boolean logic. The second is solved by passing the required path as a constructor argument.

Conclusion

In this part, we have hidden out of sight a lot of low-level details related to the file system and HTTP requests. This allows you to write really the right unit tests.

Of course, the code in FileCache and FileGetContentsHttpClient still needs to be tested, the article ends, and the tests are still slow and fragile. But you can do this: refuse to test them in favor of using existing solutions for working with files or performing HTTP requests . The burden of testing such libraries lies entirely on their developers, but it allows you to focus on the important parts of your code and make your tests faster.

UPD : Pussies: Refactoring. Part three or brushing roughness