Testing The Wallpaper File Path Helper

Towards the end of the previous video we had an almost working Wallpaper / file upload implementation. We could browse to our admin panel, enter the "Wallpaper" section, click the "Add Wallpaper" button, and choose our wallpaper file to upload.

We ventured into the browser to take a very visual inspection of our progress. We know we aren't quite there just yet. It would also be a good time to check back in with our test suite:

php vendor/bin/phpspec run

AppBundle/Event/Listener/WallpaperUploadListener
  45  - it can prePersist
      method `Double\FileInterface\P5::getExistingFilePath()` is not defined.

                                 85%                                     14%     7
3 specs
7 examples (6 passed, 1 broken)
30ms

  Would you like me to generate a method signature
  `AppBundle\Model\FileInterface::getExistingFilePath()` for you?
                                                                         [Y/n]

Important - We do not want PhpSpec to generate this method signature for us - so hit N, or ctrl+c.

Mostly our tests are all passing, bar one.

We've changed out interface without letting PhpSpec know about us doing so. This highlights a number of changes we need to make to our design.

The problem that PhpSpec has picked up on is that in our WallpaperUploadListener, we have the following invalid code:

// /src/AppBundle/Event/Listener/WallpaperUploadListener.php

        // got here
        $this->fileMover->move(
            $file->getExistingFilePath(),
            $file->getNewFilePath()
        );

After changing our FileInterface, neither of these methods are valid.

As soon as we start trying to fix this problem, another problem will become evident. Well, it will if you use an IDE. PhpStorm won't give us auto completion for our $file variable.

The reasoning for this is that our type hints are incorrect on the Wallpaper entity.

Our Wallpaper::$file property expects a string. We need to update this to take a FileInterface.

// /src/AppBundle/Entity/Wallpaper.php

use AppBundle\Model\FileInterface;

class Wallpaper
{
    // ...

    /**
     * @var FileInterface
     */
    private $file;

    // ...

    /**
     * @return FileInterface
     */
    public function getFile()
    {
        return $this->file;
    }

    /**
     * @param FileInterface $file
     * @return Wallpaper
     */
    public function setFile(FileInterface $file)
    {
        $this->file = $file;

        return $this;
    }

This resolves our auto-completion issue.

But still, we hit on another issue.

I like to think of these issues as a guiding hand in what to do next. This is a good thing, not a bad thing!

We know from our prototype that in order to work effectively with our FileMover, we are going to need to pass in a pre-constructed $newFileLocation:

        $newFileLocation = $this->wallpaperFilePathHelper->getNewFilePath(
            $file->getClientOriginalName()
        );

This wallpaperFilePathHelper service was a concept we came up with in our prototype which does not yet exist in our test-driven approach.

Let's get PhpSpec to help us again here:

php vendor/bin/phpspec desc AppBundle/Service/WallpaperFilePathHelper

Specification for AppBundle\Service\WallpaperFilePathHelper created in /path/to/my/wallpaper/spec/AppBundle/Service/WallpaperFilePathHelperSpec.php.

If we run this spec, PhpSpec will go ahead and prompt us to allow it to create this new file for us. Ahhh, the lazy man's approach to development :)

php vendor/bin/phpspec run spec/AppBundle/Service/WallpaperFilePathHelperSpec.php

AppBundle/Service/WallpaperFilePathHelper
  11  - it is initializable
      class AppBundle\Service\WallpaperFilePathHelper does not exist.

                                      100%                                       1
1 specs
1 example (1 broken)
22ms

  Do you want me to create `AppBundle\Service\WallpaperFilePathHelper` for
  you?
                                                                         [Y/n]
y
Class AppBundle\Service\WallpaperFilePathHelper created in /path/to/my/wallpaper/src/AppBundle/Service/WallpaperFilePathHelper.php.

                                      100%                                       1
1 specs
1 example (1 passed)
11ms

Sweet.

Let's not over think this. We've already created an implementation that works as part of our prototype:

<?php

// /src/AppBundle/Service/WallpaperFilePathHelper.php

namespace AppBundle\Service;

class WallpaperFilePathHelper
{
    /**
     * @var string
     */
    private $wallpaperFileDirectory;

    public function __construct(string $wallpaperFileDirectory)
    {
        $this->wallpaperFileDirectory = $wallpaperFileDirectory;
    }

    public function getNewFilePath(string $newFileName)
    {
        return $this->wallpaperFileDirectory . $newFileName;
    }
}

Writing tests for this implementation should be a small, enjoyable exercise:

<?php

// /spec/AppBundle/Service/WallpaperFilePathHelper.php

namespace spec\AppBundle\Service;

use AppBundle\Service\WallpaperFilePathHelper;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class WallpaperFilePathHelperSpec extends ObjectBehavior
{
    function let()
    {
        $this->beConstructedWith('/new/path/to/');
    }

    function it_is_initializable()
    {
        $this->shouldHaveType(WallpaperFilePathHelper::class);
    }

    function it_can_get_a_new_file_path_when_given_a_filename()
    {
        $this
            ->getNewFilePath('some/file.name')
            ->shouldReturn('/new/path/to/some/file.name')
        ;
    }
}

We need to use let to ensure each time the object is constructed, it has the expected argument - in this case, a string. The object will be constructed for both tests.

Running this should prompt us to set up a constructor, and both add and implement the expected method:

php vendor/bin/phpspec run spec/AppBundle/Service/WallpaperFilePathHelperSpec.php

AppBundle/Service/WallpaperFilePathHelper
  16  - it is initializable
      method AppBundle\Service\WallpaperFilePathHelper::__construct not found.

AppBundle/Service/WallpaperFilePathHelper
  21  - it can get a new file path
      method AppBundle\Service\WallpaperFilePathHelper::__construct not found.

                                      100%                                       2
1 specs
2 examples (2 broken)
11ms

  Do you want me to create
  `AppBundle\Service\WallpaperFilePathHelper::__construct()` for you?
                                                                         [Y/n]
y
  Method AppBundle\Service\WallpaperFilePathHelper::__construct() has been created.

AppBundle/Service/WallpaperFilePathHelper
  21  - it can get a new file path
      method AppBundle\Service\WallpaperFilePathHelper::getNewFilePath not found.

                  50%                                     50%                    2
1 specs
2 examples (1 passed, 1 broken)
10ms

  Do you want me to create
  `AppBundle\Service\WallpaperFilePathHelper::getNewFilePath()` for you?
                                                                         [Y/n]
y
  Method AppBundle\Service\WallpaperFilePathHelper::getNewFilePath() has been created.

AppBundle/Service/WallpaperFilePathHelper
  21  - it can get a new file path
      expected "/new/path/to/some/file.na...", but got null.

                  50%                                     50%                    2
1 specs
2 examples (1 passed, 1 failed)
9ms

Adding in the implementation - as already covered above - should yield two passing tests:

php vendor/bin/phpspec run spec/AppBundle/Service/WallpaperFilePathHelperSpec.php
                                      100%                                       2
1 specs
2 examples (2 passed)
7ms

A question you may have at this point is: Why am I not using an interface for the WallpaperFilePathHelper?

Simply put: I don't see a need. I am currently not expecting to have more than one implementation to help with file paths. If at some point in the future this changes, I would look to standardise to an interface. My personal preference is to keep things simple, until they need to be more complicated.

Can we do better here?

Yes. We could test a variety of circumstances.

For example, what happens if the $wallpaperFileDirectory we construct with does not end with a trailing slash?

What about if it does end with a trailing slash, and the $newFileName we pass in starts with a leading slash?

What happens if we don't pass in an argument to our constructor? (hint: kaboom!)

Each of these situations could be tested for, and the appropriate defensive action implemented to mitigate against. This is an example of how working with tests leads to a more robust system. We are more likely to consider 'edge' cases, and as our code is more likely to be isolated, validating our code's behaviour is both easier, and more likely to happen.

<?php

namespace spec\AppBundle\Service;

use AppBundle\Service\WallpaperFilePathHelper;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class WallpaperFilePathHelperSpec extends ObjectBehavior
{
    function let()
    {
        $this->beConstructedWith('/new/path/to/');
    }

    function it_is_initializable()
    {
        $this->shouldHaveType(WallpaperFilePathHelper::class);
    }

    function it_can_get_a_new_file_path()
    {
        $this
            ->getNewFilePath('some/file.name')
            ->shouldReturn('/new/path/to/some/file.name')
        ;
    }

    function it_gracefully_handles_no_trailing_slash_in_the_constructor_arg()
    {
        $this
            ->beConstructedWith('/whoops/no/trailing/slash')
        ;

        $this
            ->getNewFilePath('some/file.name')
            ->shouldReturn('/whoops/no/trailing/slash/some/file.name')
        ;
    }

    function it_removes_leading_slash_in_new_file_path_arg()
    {
        $this
            ->getNewFilePath('/another/file.name')
            ->shouldReturn('/new/path/to/another/file.name')
        ;
    }

    function it_throws_if_not_constructed_properly()
    {
        // reset the constructor arguments
        $this->beConstructedWith();

        $this
            ->shouldThrow()
            ->duringInstantiation()
        ;
    }
}

And a possible implementation:

<?php

namespace AppBundle\Service;

class WallpaperFilePathHelper
{
    /**
     * @var string
     */
    private $wallpaperFileDirectory;

    public function __construct(string $wallpaperFileDirectory)
    {
        $this->wallpaperFileDirectory = $this
            ->ensureHasTrailingSlash(
                $wallpaperFileDirectory
            )
        ;
    }

    public function getNewFilePath(string $newFilePath) : string
    {
        $newFilePath = $this->ensureHasNoLeadingSlash($newFilePath);

        return $this->wallpaperFileDirectory . $newFilePath;
    }

    private function ensureHasTrailingSlash(string $path)
    {
        if (substr($path, -1) === '/') {
            return $path;
        }

        return $path. '/';
    }

    private function ensureHasNoLeadingSlash(string $path)
    {
        if (substr($path, 0, 1) === '/') {
            return substr($path, 1);
        }

        return $path;
    }
}

Could this be cleaner? Sure, there are likely more concise ways to implement this. Ternaries could be used in the ensure functions - but for me, they reduce the readability here, rather than improve it.

What's super nice about taking this approach is that I've found myself more likely to try new things. If you have any interest in functional programming, exploring the use of array_map, array_reduce, and array_filter becomes much more approachable when you can try these functions out in a controlled manner.

One thing that PhpSpec won't remind us to do, yet which is a very important step, is to define this new service in our configuration:

# /app/config/services.yml

    app.service.wallpaper_file_path_helper:
        class: AppBundle\Service\WallpaperFilePathHelper
        arguments:
            - "%kernel.root_dir%/../web/images/"

Updating The WallpaperUploadedListener

With the creation of the WallpaperFilePathHelper we can "fill in the blanks" around how we end up with the correct file path, as asserted in our tests.

To make use of this new WallpaperFilePathHelper we're going to need to inject it as a constructor argument into our WallpaperUploadListener.

Let's add that into our spec:

<?php

namespace spec\AppBundle\Event\Listener;

use AppBundle\Service\WallpaperFilePathHelper;
// other deps removed for brevity

class WallpaperUploadListenerSpec extends ObjectBehavior
{
    private $fileMover;
    private $wallpaperFilePathHelper;

    function let(FileMover $fileMover, WallpaperFilePathHelper $wallpaperFilePathHelper)
    {
        $this->beConstructedWith($fileMover, $wallpaperFilePathHelper);

        $this->fileMover = $fileMover;
        $this->wallpaperFilePathHelper = $wallpaperFilePathHelper;
    }

And there's some changes to the it_can_prePersist test function that need to be made also:

    function it_can_prePersist(
        LifecycleEventArgs $eventArgs,
        FileInterface $file
    )
    {
        $fakeTempPath = '/tmp/some.file';
        $fakeFilename = 'some.file';

        $file->getPathname()->willReturn($fakeTempPath);
        $file->getFilename()->willReturn($fakeFilename);

        $wallpaper = new Wallpaper();
        $wallpaper->setFile($file->getWrappedObject());

        $eventArgs->getEntity()->willReturn($wallpaper);

        $fakeNewFilePath = '/some/new/fake/' . $fakeFilename;
        $this->wallpaperFilePathHelper->getNewFilePath($fakeFilename)->willReturn($fakeNewFilePath);

        $this->prePersist($eventArgs)->shouldReturn(true);

        $this->fileMover->move($fakeTempPath, $fakeNewFilePath)->shouldHaveBeenCalled();
    }

The most interesting line here:

$this->wallpaperFilePathHelper->getNewFilePath($fakeFilename)->willReturn($fakeNewFilePath);

Through this we can not only fake our return value, but also ensure that getNewFilePath was called with the expected argument. That's really powerful, and didn't take much effort at all on our part.

To make this work, we will need to update the implementation accordingly:

<?php

namespace AppBundle\Event\Listener;

use AppBundle\Entity\Wallpaper;
use AppBundle\Service\FileMover;
// this is new
use AppBundle\Service\WallpaperFilePathHelper;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;

class WallpaperUploadListener
{
    /**
     * @var FileMover
     */
    private $fileMover;
    /**
     * @var WallpaperFilePathHelper
     */
    private $wallpaperFilePathHelper;

    public function __construct(FileMover $fileMover, WallpaperFilePathHelper $wallpaperFilePathHelper)
    {
        $this->fileMover = $fileMover;
        // this is new
        $this->wallpaperFilePathHelper = $wallpaperFilePathHelper;
    }

    public function prePersist(LifecycleEventArgs $eventArgs)
    {
        $entity = $eventArgs->getEntity();

        // if not Wallpaper entity, return false
        if (false === $entity instanceof Wallpaper) {
            return false;
        }

        /**
         * @var $entity Wallpaper
         */

        $file = $entity->getFile();

        // this is new
        $newFilePath = $this->wallpaperFilePathHelper->getNewFilePath($file->getFilename());

        // got here
        $this->fileMover->move(
            $file->getPathname(),
            $newFilePath
        );

        return true;
    }

    public function preUpdate(PreUpdateEventArgs $eventArgs)
    {
    }
}

And with that, we should have a passing suite of tests. Hurrah.