Testing a Pivotal API request client using lots of mocking

Question

I have a class that is all about doing HTTP requests, and logging (in file system & database). It's only using 3 dependencies to do these things, so I'm fine with the code so far.

Here it is for convenience :

<?php

namespace App\Services\Pivotal\Request;

use App\Core\Request\HttpRequest;
use App\Core\Request\Log\RequestsLog;
use App\Services\Pivotal\Token\Factory\TokenFactory;
use Config;

class PivotalRequest
{
    private $httpRequest;
    private $tokenFactory;
    private $requestLog;
    private $serviceId;
    private $baseUri = 'https://api.pivotal.io/vnz';
    private $version = 'v1';

    public function __construct(HttpRequest $httpRequest, TokenFactory $tokenFactory, RequestsLog $requestLog)
    {
        $this->serviceId    = 287;
        $this->httpRequest  = $httpRequest;
        $this->tokenFactory = $tokenFactory;

        $this->httpRequest->setHeaders(
            [
                "Content-Type"  => "application/json",
                "Authorization" => "Bearer {$this->tokenFactory->getTokenId()}"
            ]
        );

        $this->httpRequest->setLogName('pivotal');
        $this->requestLog = $requestLog;
    }

    public function get($uri, $datas = [])
    {
        return $this->request($uri, $datas, 'get');
    }

    public function post($uri, $datas = [])
    {
        return $this->request($uri, $datas, 'post');
    }

    public function useVersionNumber($versionNumber)
    {
        $this->checkVersionNumber($versionNumber);

        $this->version = "v{$versionNumber}";
    }

    protected function request($uri, $datas, $method)
    {
        $this->prepareRequest($uri, $datas);

        return $this->getResponse($method);
    }

    protected function prepareRequest($uri, $datas)
    {
        $fullUri = $this->getFullUri($uri);

        $this->httpRequest->setRequestLog($this->requestLog->fromConnection($this->serviceId, $fullUri));
        $this->httpRequest->setUri($fullUri);
        $this->httpRequest->setDatas($datas);
    }

    protected function getResponse($method)
    {
        if ($method === 'get') {
            $response = $this->httpRequest->get();
        }
        else if ($method === 'post') {
            $response = $this->httpRequest->post();
        }

        return $response;
    }

    protected function checkVersionNumber($versionNumber)
    {
        if (!is_int($versionNumber)) {
            throw new \InvalidArgumentException('Version number should be a positive int.');
        }
    }

    private function getFullUri($uri)
    {
        $this->checkUri($uri);

        return "{$this->baseUri}/{$this->version}/$uri";
    }

    private function checkUri($uri)
    {
        if (empty($uri)) {
            throw new \InvalidArgumentException('Uri should not be empty.');
        }
    }
}

Now I'd like to write tests for this class. It seems I can't do anything else but mock, mock, and mock even more.

So far, this is what I have :

<?php

use App\Core\Request\HttpRequest;
use App\Core\Request\Log\RequestsLog;
use App\Services\Pivotal\Request\PivotalRequest;
use App\Services\Pivotal\Token\Factory\TokenFactory;

class PivotalRequestTest extends TestCase
{
    /**
     * @test
     */
    public function full_passing_request_test()
    {
        $tokenId          = 1;
        $tokenFactoryMock = Mockery::mock(TokenFactory::class);
        $tokenFactoryMock->shouldReceive('getTokenId')->once()->withNoArgs()->andReturn($tokenId);

        $fullUri         = 'https://api.pivotal.io/vnz/v1/livecheck';
        $httpRequestMock = Mockery::mock(HttpRequest::class);
        $httpRequestMock->shouldReceive('setHeaders')->once()->with([
            'Content-Type'  => 'application/json',
            'Authorization' => 'Bearer 1'
        ]);
        $httpRequestMock->shouldReceive('setLogName')->with('pivotal');
        $httpRequestMock->shouldReceive('setRequestLog')->once();
        $httpRequestMock->shouldReceive('setUri')->once()->with($fullUri);
        $httpRequestMock->shouldReceive('setDatas')->once()->with([]);
        $httpRequestMock->shouldReceive('get')->once()->withNoArgs();

        $requestLog                    = new RequestsLog();
        $requestLog->srv_name          = 'srv_name';
        $requestLog->request_name      = $fullUri;
        $requestLog->ip_src            = 'ip_src';
        $requestLog->service_id        = 287;
        $requestLog->request_status    = 'prepared';
        $requestLog->webservice_status = 'prepared';

        $requestLogMock = Mockery::mock(RequestsLog::class);
        $requestLogMock->shouldReceive('fromConnection')
                       ->once()
                       ->with(287, $fullUri)
                       ->andReturn($requestLog);

        $pivotalRequest = new PivotalRequest($httpRequestMock, $tokenFactoryMock, $requestLogMock);
        $pivotalRequest->get('livecheck', []);
    }
}

This looks really smelly to me, because I feel like I'm only describing the inners of the class, so instead of describing what the class should do, I describe how it does what it does (which should be of no interest, right ?).

Is that common to have so many mocks (basically almost only mocks) to test a class ? Is that a good practice ? If not, how can I change this ?

Answer 1

First of all, Bravo for writing unit tests! Secondly, Bravo for doing it in PHP!

Seriously though, I don't see enough people doing this type of testing.

What you're running into is referred to as test pain. Test Pain describes the amount of effort and setup required to sufficiently isolate code dependencies in order to perform unit testing.

In your case, the problem is the HttpRequest class. The class has a deep object graph that your PivotalRequest class knows too many details about.

From here, you have a couple of options:

1. Test Helpers

One approach here would be to create some helper code in your Unit tests that can easily create default values for mocked HTTP data (like request, cookies, query string, etc...) to help reduce test pain when dealing with HttpRequest.

The reason you have almost ONLY mocks as you said, is because your class is actually pretty compact and simple (which are both good). It is only the sheer size of mocking an HTTP Request compared to the amount of your actual code that makes it look this way.

As far as testing the inner-workings of your class vs. testing what it does. I would suggest renaming your test to clear this up. The convention I usually follow is something like this:

ObjectName_MethodName_GivenArguments_ShouldBehaveAsFollows

Or, for a more concrete example:

PivotalRequest_get_WithValidRequest_ShouldReturnValidResponse

By being that explicit, it becomes clear that you are really only interested in input and output and what happens in-between is less important (unless the test is focused on what is in-between).

2. Create a lightweight abstraction

If the HttpRequest class is too complicated, it could be that the code smell here is that class does too much and should be split / broken down into smaller chunks.

Also, you could consider creating a wrapper class that is simpler to Mock such as:

class CompactHttpRequest
{
    private $_httpRequest;

    function __construct(HttpRequest $httpRequest)
    {
        $this->_httpRequest = $httpRequest;
    }

    public function setHttpRequestDefaults($queryString, $headers, $formData)
    {
        // TODO: map incoming arguments to $this->_httpRequest
    }
}

Then, use the CompactHttpRequest class in the constructor of your PivotalRequest class instead of HttpRequest. In doing so, you'll only ever need to mock one method. In addition, the only aspect of that mock that you care about is that the setHttpRequestDefaults method is called. The specifics of what is passed becomes irrelevant for the purposes of the test.

3. Be less specific

In this test, the actual content of the headers being set seems like extra data that isn't necessary to specify if we're not ACTUALLY going to make the HTTP request.

4. Refactor object relationships

This being my preferred approach as I believe it is more in the spirit of OO programming.

It seems to me like the PivotalRequest doesn't do much as you also note in your question. That being the case, it seems to me to make more sense to have a Class that can process different types of requests and make your PivotalRequest class inherit from the HttpRequest class and override the specifics like the URL being posted to, the HTTP headers, etc...

Here is an illustration I made to demonstrate what I mean:

In this way, your system becomes more flexible and all of your core logic is in one spot in the RequestProcessor class.

Overall, the best way to know what will work for your application and what won't is to decide what type of testing you will do.

I am more of a mockist tester as defined by Martin Fowler, so I am interested in having slightly stronger coupling between my tests and my implementation because it forces me to think more about what I'm doing and although it makes tests a bit more fragile, it also finds more bugs.

For more on this, check out his article here:

http://martinfowler.com/articles/mocksArentStubs.html