Test Driven Wallpaper Delete - Part 1

In this video we are going to work towards a test-driven Wallpaper Delete process. Again, much like our test-drive approaches to Wallpaper Create, and Wallpaper Update, we will refer to our prototype implementation - our untested Wallpaper Delete process.

During our untested approach we ended up with two different listeners - a WallpaperUploadListener, and a WallpaperDeleteListener.

As we have discovered during our test-driven approach, a better name for our listener is simply WallpaperListener, and as a result, putting the different processes (create, update) inside this single listener has made sense. Taking this further, it also makes sense - to me, at least - to put the delete logic in here, too.

You needn't do this. You may prefer one listener for each event. I'm going with this approach as it is working fairly well so far, and would only add a more complicated implementation if our requirements changed.

Our first step is going to be to add a tag to our Symfony service definition for our wallpaper_listener:

# app/config/services.yml

services:

    app.event.listener.wallpaper_listener:
        class: AppBundle\Event\Listener\WallpaperListener
        arguments:
            - "@app.service.file_mover"
            - "@app.service.wallpaper_file_path_helper"
            - "@app.service.image_file_dimensions_helper"
        tags:
            - { name: doctrine.event_listener, event: prePersist }
            - { name: doctrine.event_listener, event: preUpdate }
            - { name: doctrine.event_listener, event: preRemove }

We have no direct test for this, so it's easy to forget to do this process :) Not for us, however, as we have done it up front.

As we know we will be listening for preRemove events, we should start writing some tests to cover this process.

Looking at our untested approach, our preRemove implementation is as follows:

// src/AppBundle/Event/Listener/WallpaperDeleteListener.php

    public function preRemove(LifecycleEventArgs $args)
    {
        /**
         * @var $entity Wallpaper
         */
        $entity = $args->getEntity();

        $this->fileDeleter->delete(
            $entity->getFilename()
        );

        $entity->setFile(null);
    }

Testing this should be fairly straightforward, given what we know at this point.

// spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

    function it_can_preRemove(LifecycleEventArgs $eventArgs, Wallpaper $wallpaper)
    {
        $wallpaper->getFilename()->willReturn('fake-filename.jpg');
        $eventArgs->getEntity()->willReturn($wallpaper);

        $this->preRemove($eventArgs);

        $this->fileDeleter->delete('fake-filename.jpg')->shouldHaveBeenCalled();

        // this won't work as expected
        // $wallpaper->setFile(null)->shouldHaveBeenCalled();
    }

Let's break this down to be absolutely clear what's happening.

Our test is called it_can_preRemove, because we expected to be able to call a method called preRemove.

This test will need two objects:

• LifecycleEventArgs $eventArgs
• Wallpaper $wallpaper

We will get PhpSpec to provide a mock for both.

Even though we don't own the implementation of LifecycleEventArgs, we haven't had any issues with mocking it so far, so I would persist with this approach until PhpSpec dictates otherwise. This is something we have covered throughout the videos in this series.

The first two lines of our test are setup. It helps to read them bottom-to-top.

From our implementation we know we will need to call getEntity on our instance of LifecycleEventArgs. We expect this to return a Wallpaper instance.

What if it doesn't?

We should write another test to cover this.

// spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

    function it_can_preRemove(LifecycleEventArgs $eventArgs, Wallpaper $wallpaper)
    {
        // * snip *
    }

    function it_will_not_continue_with_preRemove_if_not_a_Wallpaper_instance()
    {

    }

What's nice about this is that we have probably just identified a bug in our untested approach.

Remember in our prePersist and preUpdate methods we added a chunk of guard logic:

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

This is because, as we have covered previously, Doctrine is going to generate prePersist, preUpdate, and preRemove events whenever we create, update, or delete any entity.

We need to make sure our listener code reacts only to entities of type Wallpaper.

This is good stuff. Our tests are making us think at a deeper level, and stop us from laser focusing only on the happy path.

Ok, back to our test code.

    $wallpaper->getFilename()->willReturn('fake-filename.jpg');
    $eventArgs->getEntity()->willReturn($wallpaper);

When we call getEntity, we want to force PhpSpec to return our mocked $wallpaper object.

From our untested implementation we know that when we call the delete method of our fileDeleter service, we will need to pass in the Wallpaper's filename property. Therefore we fake what the return value of a call to getFilename will be.

Next, we call the method we are testing, passing in our now-setup $eventArgs object:

    $this->preRemove($eventArgs);

The implementation for preRemove is rather basic. We aren't doing any heavy lifting, but rather gathering together all the bits of data that we will need, and then using these bits of data to make the appropriate calls to underlying services which will do the heavy lifting:

$this->fileDeleter->delete('fake-filename.jpg')->shouldHaveBeenCalled();

We will use a service called fileDeleter to delete our given file by filename.

We faked the filename, so we should expect that fake filename to have been used as the only argument when invoking the delete method.

We don't have any implementation for this as yet, but our tests will guide us towards making this the reality.

Lastly in our untested implementation we null'ed out the file property on our Wallpaper class:

$entity->setFile(null);

It would be really nice if we could spy on our Wallpaper entity to see that a call to setFile was made with the argument of null:

$wallpaper->setFile(null)->shouldHaveBeenCalled();

We're going to hit upon an issue with this, but it's a potential unintuitive "gotcha" so we will cover this more in the next video.

An alternative approach here might be to use a real instance of Wallpaper, and then validate that a call to getFile returns null after our test has run.

We might need to alter our tests as we go through. We have the benefit of foresight given that we have the untested / prototype code to review. When writing tests for code that doesn't exist, typically I find the test process requests a few revisions to get to the final code.

Now let's run our new test:

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 143  - it can preRemove
      method AppBundle\Event\Listener\WallpaperListener::preRemove not found.

AppBundle/Event/Listener/WallpaperListener
 155  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                               71%                               14%      7
1 specs
7 examples (5 passed, 1 pending, 1 broken)
75ms

  Do you want me to create
  `AppBundle\Event\Listener\WallpaperListener::preRemove()` for you?
                                                                         [Y/n]
y

  Method AppBundle\Event\Listener\WallpaperListener::preRemove() has been created.

AppBundle/Event/Listener/WallpaperListener
 143  - it can preRemove
      property fileDeleter not found.

AppBundle/Event/Listener/WallpaperListener
 155  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                               71%                               14%      7
1 specs
7 examples (5 passed, 1 pending, 1 broken)
30ms

PhpSpec has helpfully generated us the method stub for preRemove inside WallpaperListener.

It's pointing out that we don't have any class property called fileDeleter, so we probably need to fix that before going much further.

A Basic File Deleter

We know from our untested / prototype approach that our FileDeleter service is going to be hardcoded towards our local filesystem. We've also said that in the future we might want to move our image files from local disk up to Amazon S3, or a similar service.

Thinking about this we probably will need multiple implementations of our FileDeleter. We will start with one for local disk, and in time add more as needed to delete from remote file systems.

By using an interface here, we can standardise our approach to deleting a file. Exactly how a file is deleted really is of no concern of the WallpaperListener.

In truth, we will probably need to rethink this approach in the future, but putting an interface here will still help us more than harm.

Let's get PhpSpec to help us through our test-driven implementation of a LocalFilesystemFileDeleter:

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

Specification for AppBundle\Service\LocalFilesystemFileDeleter created in /path/to/my/wallpaper/spec/AppBundle/Service/LocalFilesystemFileDeleterSpec.php.

That's the spec generated. Running this spec will get PhpSpec to do the dirty work of creating the expected file:

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

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

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

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

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

Immediately we can update our test code to reflect that we want this newly generated LocalFilesystemFileDeleter to implement our required interface:

<?php

// spec/AppBundle/Service/LocalFilesystemFileDeleterSpec.php

namespace spec\AppBundle\Service;

use AppBundle\Service\LocalFilesystemFileDeleter;
// don't forget this line:
use AppBundle\Service\FileDeleter;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class LocalFilesystemFileDeleterSpec extends ObjectBehavior
{
    function it_is_initializable()
    {
        $this->shouldHaveType(LocalFilesystemFileDeleter::class);
        $this->shouldImplement(FileDeleter::class);
    }
}

If we try to run this now, PhpSpec is understandably unhappy:

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

AppBundle/Service/LocalFilesystemFileDeleter
  12  - it is initializable
      expected an instance of AppBundle\Service\FileDeleter, but got
      [obj:AppBundle\Service\LocalFilesystemFileDeleter].

                                      100%                                       1
1 specs
1 example (1 failed)
25ms

We're basically just inventing things out of thin air here.

Let's create the expected interface:

<?php

// src/AppBundle/Service/FileDeleter.php

namespace AppBundle\Service;

interface FileDeleter
{
    public function delete(string $pathToFile);
}

Our interface is very straightforward.

It says that anything that implements this interface will need to have one public function called delete, which will take a single argument, a string of $pathToFile.

To make our latest PhpSpec test pass, all we need to do is ensure we implement that interface in LocalFilesystemFileDeleter and we should be good to go:

<?php

namespace AppBundle\Service;

class LocalFilesystemFileDeleter implements FileDeleter
{
    public function delete(string $pathToFile)
    {
        // TODO: Implement delete() method.
    }
}

Notice here that because FileDeleter is in the same directory as our LocalFilesystemFileDeleter, we don't need a use statement.

Because we now implements the FileDeleter interface we must have the expected method, even though currently it's just a stub that doesn't do anything.

Our test now happily passes:

php vendor/bin/phpspec run spec/AppBundle/Service/LocalFilesystemFileDeleterSpec.php
                                      100%                                       1
1 specs
1 example (1 passed)
28ms

Let's take a quick look at the implementation of FileDeleter that we have from our prototype:

<?php

// src/AppBundle/Service/FileDeleter.php

namespace AppBundle\Service;

use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\HttpFoundation\File\File;

class FileDeleter
{
    /**
     * @var Filesystem
     */
    private $filesystem;
    /**
     * @var string
     */
    private $filePath;

    public function __construct(Filesystem $filesystem, string $filePath)
    {
        $this->filesystem = $filesystem;
        $this->filePath = $filePath;
    }

    public function delete($pathToFile)
    {
        $this->filesystem->remove(
            $this->filePath . '/' . $pathToFile
        );
    }
}

Writing a test for this should be pretty easy at this point. There's nothing new here:

<?php

// spec/AppBundle/Service/LocalFilesystemFileDeleterSpec.php

namespace spec\AppBundle\Service;

use AppBundle\Service\LocalFilesystemFileDeleter;
use AppBundle\Service\FileDeleter;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use Symfony\Component\Filesystem\Filesystem;

class LocalFilesystemFileDeleterSpec extends ObjectBehavior
{
    function let(Filesystem $filesystem)
    {
        $this->beConstructedWith($filesystem, '/expected/base/path');
    }

    function it_is_initializable()
    {
        $this->shouldHaveType(LocalFilesystemFileDeleter::class);
        $this->shouldImplement(FileDeleter::class);
    }

    function it_can_delete()
    {
        $this->delete('to/some-file.jpg');

        $this
            ->filesystem
            ->remove('/expected/base/path/to/some-file.jpg')
            ->shouldHaveBeenCalled()
        ;
    }
}

We've covered it_is_initializable, so let's focus on the other two new functions:

    function let(Filesystem $filesystem)
    {
        $this->beConstructedWith($filesystem, '/expected/base/path');
    }

Every test /example that PhpSpec runs through will initially set up / __construct the underlying LocalFilesystemFileDeleter instance.

We use the let function to make sure that LocalFilesystemFileDeleter is configured with the expected properties.

In this case we mock Symfony's Filesystem. Again, we don't own this, but mocking is always my first attempt. I only change this up if PhpSpec is less than happy with my motives.

We pass this mock through to the __construct call:

    public function __construct(Filesystem $filesystem, string $filePath)
    {
        $this->filesystem = $filesystem;
        $this->filePath = $filePath;
    }

We also pass in a string: '/expected/base/path' as our second argument.

This should be enough to get the LocalFilesystemFileDeleter instance constructed properly.

The meat of our test is as follows:

    function it_can_delete()
    {
        $this->delete('to/some-file.jpg');

        $this
            ->filesystem
            ->remove('/expected/base/path/to/some-file.jpg')
            ->shouldHaveBeenCalled()
        ;
    }

Next we start by calling the delete method (as defined on our interface) with a path we just made up. It doesn't matter that this path doesn't exist in the real world, as we will never really try to delete a file in this test. All we care about is that the functions are being called with the right arguments.

As we mocked the filesystem, next we spy on what happened to that filesystem instance after a call to delete has been made.

Our assertion is that the filesystem's remove method should have been called with the path of:

'/expected/base/path/to/some-file.jpg'

This is the path we passed in to the constructor via our let method call, concatenated with the path we provided when calling delete.

Of course none of this works just yet.

Let's run our tests and get PhpSpec to help us out:

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

AppBundle/Service/LocalFilesystemFileDeleter
  18  - it is initializable
      method AppBundle\Service\LocalFilesystemFileDeleter::__construct not found.

AppBundle/Service/LocalFilesystemFileDeleter
  24  - it can delete
      method AppBundle\Service\LocalFilesystemFileDeleter::__construct not found.

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

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

AppBundle/Service/LocalFilesystemFileDeleter
  24  - it can delete
      property filesystem not found.

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

After running through our tests, PhpSpec will have created us a __construct function inside our LocalFilesystemFileDeleter implementation, but we must provide further logic to make the it_can_delete test into a passing state.

We have an implementation for this which should get us to a passing state. Let's copy / paste from our prototype and see where we get:

<?php

namespace AppBundle\Service;

use Symfony\Component\Filesystem\Filesystem;

class LocalFilesystemFileDeleter implements FileDeleter
{
    /**
     * @var Filesystem
     */
    private $filesystem;
    /**
     * @var string
     */
    private $filePath;

    public function __construct(Filesystem $filesystem, string $filePath)
    {
        $this->filesystem = $filesystem;
        $this->filePath = $filePath;
    }

    public function delete(string $pathToFile)
    {
        $this->filesystem->remove(
            $this->filePath . '/' . $pathToFile
        );
    }
}

If we run the tests now, they still fail:

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

AppBundle/Service/LocalFilesystemFileDeleter
  24  - it can delete
      property filesystem not found.

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

The error message:

property filesystem not found

Is what we need to focus on.

This is potentially confusing.

In our test file we have the following:

<?php

// spec/AppBundle/Service/LocalFilesystemFileDeleterSpec.php

class LocalFilesystemFileDeleterSpec extends ObjectBehavior
{
    // * snip *

    function it_can_delete()
    {
        $this->delete('to/some-file.jpg');

        $this
            ->filesystem
            ->remove('/expected/base/path/to/some-file.jpg')
            ->shouldHaveBeenCalled()
        ;
    }
}

The error message relates to our call to $this->filesystem.

We're trying to access $this->filesystem but we haven't defined it inside our test spec.

Fixing this is easy, but I wanted to highlight this as a potential problem area:

<?php

// spec/AppBundle/Service/LocalFilesystemFileDeleterSpec.php

class LocalFilesystemFileDeleterSpec extends ObjectBehavior
{
    private $filesystem;

    function let(Filesystem $filesystem)
    {
        $this->filesystem = $filesystem;

        $this->beConstructedWith($filesystem, '/expected/base/path');
    }

    // * snip *

    function it_can_delete()
    {
        $this->delete('to/some-file.jpg');

        $this
            ->filesystem
            ->remove('/expected/base/path/to/some-file.jpg')
            ->shouldHaveBeenCalled()
        ;
    }
}

Notice now that we have defined $filesystem as a property of the class. And now our tests pass:

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

Ok, so we are well on our way.

The last thing we need to here is to add in the service definition for our new LocalFilesystemFileDeleter:

# app/config/services.yml

    app.service.local_filesystem_file_deleter:
        class: AppBundle\Service\LocalFilesystemFileDeleter
        arguments:
            - "@filesystem"
            - "%kernel.root_dir%/../web/images"

The service definitions are starting to feel a little messy. We will come back to sort these out in a few videos time.