Refactoring towards testability

There are multiple issues with this code:

• Unpredictable methods are used (such as sys_get_temp_dir and temp_name).
• We use a global variable.
• The global variable can be changed by the CanWriteFiles::verify method which could make the function calls after that unpredictable.

By wrapping the static/unpredictable code in services, injecting these through the constructor and by passing the global user id to the method, we can easily extract all global state out of the method and make it testable. A better iteration for this class could look like this:

canWriteFiles = $canWriteFiles;
        $this->temporaryFiles = $temporaryFiles;
    }

    public function writeToTemporaryFile(string $data, int $userId): string
    {
        $this->canWriteFiles->verify($userId);

        return $this->temporaryFiles->write($userId, $data);
    }
}

We can now perfectly test the logic in this class using a test:

canWriteFiles = $this->createMock(CanWriteFiles::class);
        $this->temporaryFiles = $this->createMock(TemporaryFiles::class);
        $this->fileManager = new FileManager(
            $this->canWriteFiles,
            $this->temporaryFiles
        );
    }

    /**
     * @test
     */
    public function itDoesNotWriteAFileIfUserHasNotEnoughRights()
    {
        $this->givenUserCannotWriteFiles(1);

        $this->temporaryFiles->expects($this->never())
            ->method('write');

        $this->expectException(CannotWriteFiles::class);
        $this->fileManager->writeToTemporaryFile('data', 1);
    }

    /**
     * @test
     */
    public function itDoesReturnAFilenameIfUserCanWriteFiles()
    {
        $this->givenuserCanWriteFiles(1);

        $this->temporaryFiles->expects($this->once())
            ->method('write')
            ->with('data', 1)
            ->willReturn('/tmp/filename');

        $fileName = $this->fileManager->writeToTemporaryFile('data', 1);

        $this->assertEquals('/tmp/filename', $fileName);
    }

    private function givenUserCannotWriteFiles(int $userId)
    {
        $this->canWriteFiles->method('verify')
            ->with($userId)
            ->willThrowException(CannotWriteFiles::withUserId($userId));
    }

    private function givenUserCanWriteFiles(int $userId)
    {
        $this->canWriteFiles->method('verify')
            ->with($userId)
            ->willReturn(true);
    }
}

Avoid instantiating services in code

Whenever you see the new keyword pop up in your code and it is used to instantiate anything but a value object or an entity, you’re likely creating services here that you would prefer to replace by another implementation (e.g. a mock) in your tests.

A good example of this is a piece of code that has to call a fictional microservice, in order to see if a user is subscribed:

 'http://api-base-url.example.com/'
        ]);

        $response = $client->get($endpoint, $requestData);

        if ($response->getStatusCode() != 200) {
            throw new RequestFailedException('could not call ' . $endpoint);
        }

        return json_decode((string) $response->getData(), true);
    }
}

This code is untestable without doing real requests to the production version of a microservice that contains real application data. As a result, such code is prone to changes. When we pull out the client and pass it to the class using dependency injection, we can easily make the class testable. So we can also easily configure the client differently in other environments (e.g. a staging and a local microservice). The result could look like this:

createMock(HttpClientInterface::class);
        $caller = new MicroServiceCaller($client);
        $endpoint = '/isUserSubscribed';
        $requestData = ['user_id' => 123];

        $this->client->expects($this->once())
            ->method('get')
            ->with($endpoint, $requestData)
            ->willReturn(
                Response::withData(
                    '{"data":{"marketing_mails": false, "notifications": true}}'
                )
            );

        $response = $caller->call($endpoint, $requestData);

        $this->assertEquals(
            [
                'marketing_mails' => false,
                'notifications' => true,
            ],
            $response
        );
    }

    /** @test*/
    public function itThrowsWhenTheMicroServiceIsNotReachable()
    {
        $client = $this->createMock(HttpClientInterface::class);
        $caller = new MicroServiceCaller($client);

        $this->client->expects($this->once())
            ->method('get')
            ->willReturn(Response::withErrorCode(400));

        $this->expectException(RequestFailedException::class);
        $caller->call('/isUserSubscribed', ['user_id' => 123]);
    }
}

client = $client;
    }

    public function get(string $endpoint, array $requestData): array
    {
        $response = $this->client->get($endpoint, $requestData);

        if ($response->getStatusCode() != 200) {
            throw new RequestFailedException('could not call ' . $endpoint);
        }

        $responseData = json_decode((string) $response->getData(), true);

        return $responseData['data'];
    }
}

No more statics

The main issue with testing static methods is that it basically is procedural code. Since static methods never have state, they can be entirely unpredictable and can call other code that has side effects which you don’t want to (or even cannot) execute while running your tests. It is also extremely hard to mock code that is static. In PHP, the ‘patchwork’ framework lets you do exactly that. But trust me, you shouldn’t use it! This framework will slow down your tests enormously, because it hooks into all internal calls and checks to see if it has to monkey patch execution paths on the fly. So let’s avoid having to do that!

An example of untestable code is this controller, which uses static methods to see which user it has to show the detail page:

userRepository = $userRepository;
        $this->templating = $templating;
    }

    public function detail(): ResponseInterface
    {
        $userId = RequestData::get('id');

        if (!$userId) {
            $userId = Authentication::getCurrentUserId();
        }

        try {
            $user = $this->userRepository->find($userId);
        } catch (UserNotFound $exception) {
            return new NotFoundResponse();
        }

        return $this->templating->render('user-detail', compact($user));
    }
}

Both static calls might do a lot in the background. Maybe the Request::get method depends on $_POST variables to be available, while the Authentication::getCurrentUserId might depend on a session being started and having user data in there. We can do better! By using a request object that is instantiatable and can be passed to the controller, we can easily mock the request or change it with a different implementation that does not have any dependencies. This would result in something like this:

userRepository = $userRepository;
        $this->templating = $templating;
    }

    public function detail(AuthenticatedRequest $request): ResponseInterface
    {
        $userId = $request->getParameter('id');

        if (!$userId) {
            $userId = $request->getAuthenticatedUserId();
        }

        try {
            $user = $this->userRepository->find($userId);
        } catch (UserNotFound $exception) {
            return new NotFoundResponse();
        }

        return $this->templating->render('user-detail', compact($user));
    }
}

userRepository = $this->createMock(UserRepository::class);
        $this->userController = new UserController(
            $this->userRepository,
            new JsonTemplating() // could be a mock, but let's use a dummy implementation for once
        );
    }

    /** @test */
    public function itUsesTheUserIdProvidedInTheRequest()
    {
        $requestedUser = new User(12, 'John', 'Doe');
        $authenticatedUser = new User(17, 'Jane', 'Roe');
        $this->userRepository->expects($this->once())
            ->method('find')
            ->with($requestedUser->getId())
            ->willReturn();

        $response = $this->userController->detail(new AuthenticatedRequest(
            $authenticatedUser,
            ['id' => $requestedUser->getId()]
        ));

        $this->assertEquals(
            new JsonResponse(json_encode($requestedUser)),
            $response
        );
    }

    /** @test */
    public function itFallsBackToTheAuthenticatedUserIfNoIdIsProvided()
    {
        $authenticatedUser = new User(17, 'Jane', 'Roe');
        $this->userRepository->expects($this->once())
            ->method('find')
            ->with($authenticatedUser->getId())
            ->willReturn();

        $response = $this->userController->detail(new AuthenticatedRequest(
            new AuthenticatedUser(17)
        ));

        $this->assertEquals(
            new JsonResponse(json_encode($authenticatedUser)),
            $response
        );
    }

    /** @test */
    public function itReturnsANotFoundWhenNoUserCouldBeFound()
    {
        $this->userRepository->expects($this->once())
            ->method('find')
            ->willThrowException(UserNotFound::class);

        $response = $this->userController->detail(new AuthenticatedRequest(
            new AuthenticatedUser(17)
        ));

        $this->assertEquals(new NotFoundResponse(), $response);
    }
}

No die/exit statements

When your code contains statements that stop code execution, this will also stop execution of your tests when it reaches that point. Consequently your testsuite won’t be able to output the information it collected by running the testsuite and – even worse – it won’t be able to run all tests coming after your exit or die statement. This is clearly a no-go for testability.

A simplified example of a piece of code that does exactly that could be this untestable service:

mailer = $mailer;
    }

    public function notify(User $user): void
    {
        if (!$user->hasEmail()) {
            die('please add an email to the user');
        }

        $this->mailer->send(
            'Welcome to our platform',
            'Thanks for registering, please activate using ...',
            $user->getEmail()
        );
    }
}

There are two common strategies to get rid of die/exit statements in code:

1. Use a return instead of a die. This is commonly done to do redirects, instead of echoing headers and then doing an exit. You could return a RedirectResponse object instead: this object then gets converted to the right headers in the outer layer of your code.
2. Throw an exception instead of exiting immediately. This way, the execution of your code is still stopped, but you can choose to still do something with your exception by catching it wherever you want. This could for example allow you to create a global error handler in your application wich logs the error and shows a user friendly message to the user.

By using the second strategy, we can refactor the code and get to this result:

mailer = $mailer;
    }

    public function notify(User $user): void
    {
        if (!$user->hasEmail()) {
            throw CanNotSendEmail::withoutRecipients();
        }

        $this->mailer->send(
            'Welcome to our platform',
            'Thanks for registering, please activate using ...',
            $user->getEmail()
        );
    }
}

createMock(Mailer::class));
        $userWithoutEmail = new User('name');

        $this->expectException(CanNotSendEmail::class);
        $newUserNotifier->notify($userWithoutEmail);
    }

    /** @test */
    public function itEmailsNewUsersThatHaveAnEmailAddress()
    {
        $mailer = $this->createMock(Mailer::class);
        $newUserNotifier = new NewUserNotifier($mailer);

        $userWithEmail = new User('name', '[email protected]');

        $mailer->expects($this->once())
            ->method('send')
            ->with(
                $this->assertInternalType('string'),
                $this->assertInternalType('string'),
                '[email protected]'
            );
        $newUserNotifier->notify($userWithEmail);
    }
}