Don't Mock What You Don't Own

Towards the end of the previous video we had learned one way to implement a File Upload process inside our EasyAdminBundle admin backend.

To get an understanding of how this process could work, we went with an untested approach. For the sake of this next section, we shall consider this untested approach to be our prototype. At this stage it's really easy to convince ourselves that we are done - we have a working solution - and that we can move on.

Now, if you have no desire to learn about testing then by all means feel free to skip these next few videos and move on to implementing the 'Edit' Wallpaper form functionality. However, if you would like to learn about testing, whether to learn testing as a concept, or to learn about how testing can improve your approach to software development in general, then I hope you will enjoy this and the next few videos in particular.

I'm going to give you a heads up: testing this process will be harder than you might think.

Even though we have a prototype to work from and refer back too, we have done something in our prototype that I believe is a really common practice when concentrating on code, rather than thinking of the bigger picture:

We have tied ourselves to a very specific implementation of a file: Symfony's UploadedFile.

As we covered in an earlier video, Symfony's UploadedFile extends Symfony's File, which itself extends \SplFileInfo.

Because we are working with a Symfony project, this may not seem like such a bad thing - and in many ways I personally think it's fine to couple to your framework of choice. The often touted "decouple from your framework" stuff is really rather unnecessary in my experience. There has never been a time when I've wanted to swap frameworks, and if I did, it would highly unlikely be to another PHP-based framework. But that's beside the point.

Even so, we are now tied to working with Symfony's UploadedFile. We don't control this implementation - and it doesn't have an interface we can rely on.

Let's see how this seemingly small issue impacts our testing work flow, and overall system design.

Oh, you can't get out backwards. You've got to go forwards to go back, better press on.

To start with, we need to revert our codebase back to as it was in Video 18.

To do this, I'm going to glog (or git log) to see my commit history, and then git checkout the tagged code as it was at video 18:

git log --oneline --decorate --color --graph

* 8a26598 (HEAD -> master, tag: vid-20, github/master) vid-20
* 7e6c5bc (tag: vid-19) wall-19
* cf52be8 (tag: vid-18) vid-18
* 668773d (tag: vid-17) vid 17
...

From here I can see a list of git commit hashes (e.g. 7e6c5bc, cf52be8, etc) and the tag name I gave each hash.

What I want to do is revert to the code as it was at the end of video 18, which corresponds to tag vid-18.

I will use git to checkout that commit, then create a new branch from that commit which I will call vid-21:

git checkout cf52be8
Note: checking out 'cf52be8'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at cf52be8... vid-18

Cool, our code is now as it was at the end of video 18. Now to create a new branch from this point, I need to simply checkout a new named branch:

git checkout -b vid-21

As we made changes to our database during videos 19 and 20, we will need to revert those changes too. The easiest way to do this is to drop the database and recreate, apply all our migrations, then populate using our fixtures.

First we must drop and re-create our database, and then migrate up to where we were at:

php bin/console doctrine:database:drop --force
Dropped database for connection named `wallpaper_site`

php bin/console doctrine:database:create
Created database `wallpaper_site` for connection named default

php bin/console doctrine:migrations:migrate

* snip *

  ------------------------

  ++ finished in 0.16s
  ++ 4 migrations executed
  ++ 6 sql queries

We are now back to an empty database with the schema in-line with how it was before we started prototyping.

We also need to re-populate our database with our fixture data.

There is one thing that we learned during our prototyping phase that we need to immediately implement: we don't need an @ORM annotation on our Wallpaper::$file property. Let's remove that:

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

    /**
     * @var string
     *
     * // remove the @ORM\Column(name="file", type="string") annotation
     */
    private $file;

With that done, we now need to generate a new migration file:

php bin/console doctrine:migrations:diff

Generated new migration class to "/path/to/my/wallpaper/app/DoctrineMigrations/Version20170701084502.php" from schema differences.
<?php

namespace Application\Migrations;

use Doctrine\DBAL\Migrations\AbstractMigration;
use Doctrine\DBAL\Schema\Schema;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
class Version20170701084502 extends AbstractMigration
{
    /**
     * @param Schema $schema
     */
    public function up(Schema $schema)
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE wallpaper DROP file');
    }

    /**
     * @param Schema $schema
     */
    public function down(Schema $schema)
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE wallpaper ADD file VARCHAR(255) NOT NULL COLLATE utf8_unicode_ci');
    }
}

And then we can apply the migration:

php bin/console doctrine:migrations:migrate

                    Application Migrations

WARNING! You are about to execute a database migration that could result in schema changes and data lost. Are you sure you wish to continue? (y/n)y
Migrating up to 20170701084502 from 20170612135209

  ++ migrating 20170701084502

     -> ALTER TABLE wallpaper DROP file

  ++ migrated (0.1s)

  ------------------------

  ++ finished in 0.1s
  ++ 1 migrations executed
  ++ 1 sql queries

Cool, now we can safely load our fixtures:

php bin/console doctrine:fixtures:load

Careful, database will be purged. Do you want to continue y/N ?y
  > purging database
  > loading [100] AppBundle\DataFixtures\ORM\LoadCategoryData
  > loading [200] AppBundle\DataFixtures\ORM\LoadWallpaperData

With all the setup out of the way, let's get back to writing some tests.

Test Driving Donuts

We have already made a start on testing the WallpaperUploadListener. Here's what we have currently:

<?php

// /spec/AppBundle/Event/Listener/WallpaperUploadListenerSpec.php

namespace spec\AppBundle\Event\Listener;

use AppBundle\Entity\Category;
use AppBundle\Event\Listener\WallpaperUploadListener;
use AppBundle\Service\FileMover;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class WallpaperUploadListenerSpec extends ObjectBehavior
{
    private $fileMover;

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

        $this->fileMover = $fileMover;
    }

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

    function it_returns_early_if_prePersist_LifecycleEventArgs_entity_is_not_a_Wallpaper_instance(
        LifecycleEventArgs $eventArgs
    )
    {
        $eventArgs->getEntity()->willReturn(new Category());

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

        $this->fileMover->move(
            Argument::any(),
            Argument::any()
        )->shouldNotHaveBeenCalled();
    }

    function it_can_prePersist(LifecycleEventArgs $eventArgs)
    {
        $fakeTempPath = '/tmp/some.file';
        $fakeRealPath = '/path/to/my/project/some.file';

        $this->prePersist($eventArgs);

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

    function it_can_preUpdate(PreUpdateEventArgs $eventArgs)
    {
        $this->preUpdate($eventArgs);
    }
}

We've previously discussed how our prePersist and preUpdate methods are going to be doing very similar things, so are concentrating primarily on prePersist before expanding to preUpdate later.

The most noticable difference is that prePersist relates to creating new Wallpapers, and preUpdate will relate to updating existing Wallpapers. We haven't gotten to the 'Edit' portion of our implementation just yet, so aren't overly concerned with that at present.

Right now when running our tests we still have the same failure as before our prototype excursion:

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

AppBundle/Event/Listener/WallpaperUploadListener
  43  - it can prePersist
      no calls have been made that match:
        Double\AppBundle\Service\FileMover\P4->move(exact("/tmp/some.file"), exact("/path/to/my/project/some.file"))
      but expected at least one.

                            75%                                     25%          4
1 specs
4 examples (3 passed, 1 failed)
75ms

Let's push on with the it_can_prePersist test to start making use of the knowledge we gained from our prototype phase.

We can 'cheat' a little here. Unlike with true TDD, we already know the approximate outcome of some code that works. We've removed a chunk of the guessing phase - the bit that I personally find tricky with true TDD. Let's quickly refer to our working implementation of the WallpaperUploadListener, paying particular attention to prePersist:

<?php

// working implementation from our prototype, as of Video 20
// /src/AppBundle/Event/Listener/WallpaperUploadListener.php

namespace AppBundle\Event\Listener;

use AppBundle\Entity\Wallpaper;
use AppBundle\Service\FileMover;
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->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
         */

        // get access to the file
        $file = $entity->getFile();

        $temporaryLocation = $file->getPathname();

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

        // todo:
        //   - move the uploaded file
        $this->fileMover->move($temporaryLocation, $newFileLocation);

        //   - update the entity with additional info
        [
            0 => $width,
            1 => $height,
        ] = getimagesize($newFileLocation);

        $entity
            ->setFilename(
                $file->getClientOriginalName()
            )
            ->setHeight($height)
            ->setWidth($width)
        ;

        return true;
    }

    public function preUpdate(PreUpdateEventArgs $eventArgs)
    {
    }
}

Let's strip this down to the most basic 'stuff' that initially matters. Remember we already have a test for the guard clause (where we check if this is an instanceof Wallpaper):

// get access to the file
$file = $entity->getFile();

$temporaryLocation = $file->getPathname();

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

$this->fileMover->move($temporaryLocation, $newFileLocation);

The entity that's been populated as a result of submitting the Wallpaper form type (thanks to EasyAdminBundle) has a method called getFile.

Calling getFile, we know, should return an instance of Symfony's UploadedFile. This is because we have the following snippet of config in our EasyAdminBundle:

# /app/config/config/easy_admin_bundle.yml

easy_admin:
    entities:
        Wallpaper:
            class: AppBundle\Entity\Wallpaper
            form:
                fields:
                    - { property: "file", type: "file", label: "File" }
                    - "slug"

I've removed some superfluous config from that snippet.

The important part is that we are using one of EasyAdminBundle's form field helpers to define our Wallpaper::file property should store the outcome of a file upload. When EasyAdminBundle renders this form, Symfony's FileType form field type is used and rendered into our form as a file upload input.

This is how we get to accessing that UploadedFile instance when calling getFile. Symfony's file upload form will always return us an instance of UploadedFile.

Let's add this knowledge into our test setup:

// /spec/AppBundle/Event/Listener/WallpaperUploadListenerSpec.php

use AppBundle\Entity\Wallpaper;
use Symfony\Component\HttpFoundation\File\UploadedFile;

    function it_can_prePersist(
        LifecycleEventArgs $eventArgs,
        UploadedFile $uploadedFile // new line
    )
    {
        $fakeTempPath = '/tmp/some.file';
        $fakeRealPath =  '/path/to/my/project/some.file';

        $uploadedFile->getPathname()->willReturn($fakeTempPath);

        $wallpaper = new Wallpaper();
        $wallpaper->setFile($uploadedFile);

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

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

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

Ok, let's break this down.

use Symfony\Component\HttpFoundation\File\UploadedFile;

    function it_can_prePersist(
        LifecycleEventArgs $eventArgs,
        UploadedFile $uploadedFile // new line
    )

Just like we mocked LifecycleEventArgs, we are also going to mock Symfony's UploadedFile.

These are not classes that we 'own'.

Very important to note: These are classes - as in concrete implementations - not interfaces.

Will this work for us? Let's carry on and find out.

$fakeTempPath = '/tmp/some.file';
$fakeRealPath =  '/path/to/my/project/some.file';

We need some paths to work with. These are just fake paths we've invented that we will expect to pass in, and if all goes to plan, we can use later to check the arguments used by other methods that rely on these paths.

$uploadedFile->getPathname()->willReturn($fakeTempPath);

Our $uploadedFile variable is a PhpSpec Collaborator object, as injected for us via the function arguments.

As $uploadedFile is a collaborator we can tell it how to behave. In this case, when the getPathname() method is called, we want it to always return the contents of our $fakeTempPath variable.

$wallpaper = new Wallpaper();
$wallpaper->setFile($uploadedFile);

Wallpaper is a class that we control. It is part of our "business domain".

However, we aren't mocking Wallpaper. We are using a real new instance.

Onto this particular instance we are setting the file property (via setFile) to be our $uploadedFile collaborator. In other words, when $wallpaper->getFile() is called in the real implementation, we want it to return our PhpSpec collaborator, which should behave as we have previously defined.

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

Likewise, $eventArgs is also a collaborator. Here we are telling it to return our real $wallpaper instance whenever getEntity() is called inside the real implementation we are testing.

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

This is our primary assertion.

In other words, all the 'stuff' that came before was a bunch of setup logic helping us get to a point where we can check an outcome is as we expect.

When we call prePersist and pass in the $eventArgs collaborator, the real implementation will run through and - hopefully - we should get back a true boolean outcome.

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

Our secondary assertion is that our fileMover->move method should have been called with our expected arguments.

All of this seems like good stuff. But does it work?

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

AppBundle/Event/Listener/WallpaperUploadListener
  45  - it can prePersist
      exception [exc:Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException("The file "" does not exist")] has been thrown.

                            75%                                     25%          4
1 specs
4 examples (3 passed, 1 broken)
71ms

Heck yes it ... doesn't?!

At this point you may be wondering why on Earth I am showing you stuff that doesn't work.

Stick with me :)

The exception is confusing:

FileNotFoundException("The file "" does not exist")

A file named... or with the path "" doesn't exist?

Our test code doesn't cover how a file (presumably an UploadedFile?) instance arrives. We've used a collaborator, so why isn't that working? Is it a bug with PhpSpec?

As it happens we have run afoul of a way PhpSpec expects us to work:

Don't mock what you don't own.

A bunch of very clever people have espoused this knowledge, but in my mind I attribute this quote (possibly slightly paraphrased) from Everzet.

We are mocking two things we don't own here:

• LifecycleEventArgs
• UploadedFile

Up until now, we've largely gotten away with it for the purpose of LifecycleEventArgs. We could use a new LifecycleEventArgs instance in our code, and then we would need to create a collaborator to mock the dependencies of that object (an instance of ObjectManager). For me, that seems overkill, so I'm going to remain with LifecycleEventArgs as a collaborator here to avoid making the code harder to read, for my tastes. Feel free to disagree, I'm always open to improving.

But if we can't use a collaborator for UploadedFile then let's try creating a new UploadedFile instance in our test code, and see where that gets us:

// /spec/AppBundle/Event/Listener/WallpaperUploadListenerSpec.php

use AppBundle\Entity\Wallpaper;
use Symfony\Component\HttpFoundation\File\UploadedFile;

// ...

    function it_can_prePersist(
        LifecycleEventArgs $eventArgs
    )
    {
        $fakeTempPath = '/tmp/some.file';
        $fakeRealPath = '/path/to/my/project/some.file';

        $path = $fakeTempPath;
        $originalName = 'some.file';

        $uploadedFile = new UploadedFile($path, $originalName)

        $wallpaper = new Wallpaper();
        $wallpaper->setFile($uploadedFile);

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

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

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

An UploadedFile takes two arguments:

• path
• originalName

We have set these as known values.

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

AppBundle/Event/Listener/WallpaperUploadListener
  45  - it can prePersist
      exception [exc:Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException("The file "/tmp/some.file" does not exist")] has been thrown.

                            75%                                     25%          4
1 specs
4 examples (3 passed, 1 broken)
62ms

But we can't just fake these values.

We're confusing PHP here. We've just created this new UploadedFile instance, which by way of its inheritance chain, PHP thinks is an instance of \SplFileInfo. Yet, when PHP does some rudimentary checks - such as if the actual file exists on disk at the path we are giving - then everything falls to pieces.

No matter which way we try to go here - using a mock, or using a real implementation - we seem to have snookered ourselves.

Even though this may seem like a bad situation, we are now experiencing one of the major benefits of PhpSpec in my opinion. Whenever things get hard, it's usually an indication of a bad design decision in our system.

Before moving on to the next video, take a few minutes and think about how you may address this problem. By way of a clue, remember the quote from Everzet above:

Don't mock what you don't own.

See you in the next video :)