Unit Testing: Have I Been Doing It All Wrong?

I took part in a really interesting discussion late last week, and into the first part of this week regarding the usefulness of unit testing in the format most of us(?) practice.

To give some context: a problem was discovered whilst preparing a Live deployment. The problem itself was really small: an array being re-instantiated in a conditional, about 5 lines after it had originally been instantiated. That’s a very nerdy way of saying this was happening:

$myArray = ['a'];

if ($something) {
  $myArray = [];
}

This is the real world. Stuff happens. We deal with it, we (hopefully) find a way to mitigate it, and we move on.

My process of mitigation was to create a set of unit tests for the file in question. I used my typical approach:

  • Cover the default happy path – only mandatory arguments, unit testing a few variations if needed
  • Cover the alternative happy path – any optional arguments
  • Check for the obvious bad stuff, and assert the mitigation is as expected

I put the code in for review, and got some interesting feedback:

I don’t think Unit testing this class is the way to go… in an ideal world

I am paraphrasing somewhat, but stick with me.

Looking at this made me question everything I do around testing.

I deeply value and trust the opinion of this reviewer, and they are telling me that unit testing a class is not the way to go?

Am I doing unit testing all wrong?

There was a fair bit more to this piece of feedback on this particular PR. The reviewer had been kind enough to offer more detail on their thoughts for this issue.

This person’s preferred approach would be to test the interactions with this class, rather than the class itself.

To test the wider system behaviour, rather than the individual steps.

And this got me to thinking. I’d heard this advice before. I’ve read this advice before. But I started to question if it had sunk in.

Am I wrong to think unit tests add value here?

If unit tests haven’t already been created for this class, is it even worth adding them now?

At some point, can explicitly untested code ever be considered trusted?

I mulled over a bunch of questions like these all weekend.

My Perspective on Unit Testing

As a beginner to a project, my approach when unit testing is to work my way up.

I start with some problem to solve, and I follow that one tiny path from beginning to end, and see what I interact with along the way.

For any class I find, I look for a unit test.

If I find one, I read it.

If one doesn’t exist, I try to create one.

This isn’t always possible, particularly on legacy code.

In that case, one solution might be to hide implementations behind an interface. This way you can A/B any new code you do write, giving you options.

Once I have done this, I create a unit test for the new / revised / alternative implementation.

I keep doing this until I reach the end of the request>response life-cycle.

This causes me to write mostly one type of tests.

I write a lot of unit tests. When I don’t see them, I write them.

I believe this adds value. At the very least, this adds complimentary value.

Uncle Bob - a helpful mentor on unit testingAnother reviewer in the same thread, another very intelligent and smart person whose opinion I valued linked to some related reading. An Uncle Bob article, in particular.

I read that article twice, in full.

And I didn’t understand it.

Specifically, I didn’t understand this bit:

As the tests get more specific, the production code gets more generic.

This article takes the point of view that if you’re typically src  and test  files look something like:

  • MyImportantConcreteThing.php
  • MyImportantConcreteThingTest.php

That your tests are highly coupled to your production code. Which makes refactoring – true refactoring – inherently more difficult.

I am super guilty of calling any changes to my code refactoring. It sounds very official. Sorry, I can’t come to the pub, I’m refactoring.

Refactoring is defined as a sequence of small changes that keep the tests passing at all times

If the unit tests are tightly coupled to your implementation, it’s highly likely that small changes to your code break, comparatively, a lot of tests.

Keeping the test suite up to date becomes a chore, and is soon sacrificed when project managers push for constant changes. The rot sets in.

What I Learned

Look for behaviour. Then test that behaviour.

I agreed with this approach already.

My perspective of what constitutes behaviour is where I have been asking myself the most questions.

I feel I needed to understand the behaviour of that one class. As an outsider looking in, I found value in this approach.

I’ve learned to question the correct layer in which adding a test, or set of tests, gives the most benefit.

It may be that your problem is solved by an integration test suite. On larger projects, this test suite may not even be in the same language you’re working in. This presents different challenges.

Tools like Behat, and PHPSpec have led me down some paths that have been encouraging me to work like Uncle Bob, without even realising it.

I’ve also learned that I still have a lot to learn about unit testing. That’s a great thing. I have ordered Martin Fowler’s Refactoring book to better inform myself of what Refactoring is truly supposed to be about.

There are some interesting links I’d like to share with you this week around this subject:

And this talk:

I’d love to hear your opinions on this topic, too.

Video Update

This week saw three new videos added to the site.

https://codereviewvideos.com/course/live-stream-broken-link-checker/video/project-introduction-overview

This is something a little different. Members only. Enjoy.

https://codereviewvideos.com/course/everyday-linux/video/run-phpunit-tests-on-file-change

I like to run my unit test suite a lot whilst I’m developing. Tools like Facebook’s Jest have spoiled me. I want my unit tests to run automatically whenever I make a change to my code, or my tests.

If you’re like me, too lazy to keep hitting that damn up-arrow key, then this solution may be great for you.

There’s just one caveat: you need to be using Linux.

There is a chance this might work if using Windows Linux Subsystem (or whatever name it has). If you try it on Windows, please let me know if it works. Or you could always use Linux, the best OS.

https://codereviewvideos.com/course/beginners-guide-back-end-json-api-front-end-2018/video/handling-errors-api-platform

We wrapped up the API Platform portion of the Beginners Guide to Back End (JSON API) + Front End Development [2018] series.

This involved looking at the output when things go wrong. And capturing this data in our Behat tests.

Happy Days

Ok fans of Fonzie that I know you are, I’m going to wish you glad tidings for the weekend.

I’m looking forwards to next week, where I’m hoping to get 2 solid days of recording on the Live Stream project.

Oh by the way, I know it’s not a true / proper Live Stream. That’s coming, I just haven’t had time to figure out how to set it up.

Until next week, have a great weekend, and happy coding.

Chris

What happens to your design if you don’t use TDD?

In this week’s videos we are exploring TDD in PHP from an angle I’ve never seen covered before in other online tutorials.

To set the scene, we are adding File Upload to our EasyAdminBundle backend.

As this is a beginner friendly series, I am fully aware that adding tests – specifically PhpSpec – into the mix alongside all the other “stuff” we need to know to work effectively with Symfony can be a little overwhelming.

I said last week how I think testing is one of the most effective ways to improve yourself as a software developer. I stand by that.

However, there’s a hurdle with testing that’s seldom addressed:

How can I write code with tests if I don’t understand how to write the code without tests?

Ok, so to be absolutely clear here, I’m not talking about syntax. At this stage I am assuming you have a grasp of the fundamentals of PHP and are interested in expanding your knowledge into learning Symfony.

What I’m talking about here is how to write tests for everyday workflows inside your Symfony applications.

When starting with Symfony it can certainly feel overwhelming enough just to get a few forms up and running. Throw in saving your form data off to the database, and you might need a long lie down.

And yet now I’m saying hey, let’s learn how to write tests for all this jazz!

I’ll let you in on a little secret:

I was not planning on showing you any of this 🙂

When I started out writing up this series, I built out the prototype and was happy with the outcome.

When I started recording the videos I realised that I had bumped up the difficultly significantly about 50% of the way through the course. This was because of the inclusion of tests.

When coding up the prototype I had gone head down into “code mode”. I’d implemented a working, and tested upload system but the outcome would be really hard to explain without a lot of additional context.

At this point I had to make a decision. The outcome of that decision was to alter the course to show how I ended up with the test-driven outcome.

To get to this outcome it makes most sense to show how to write this code without tests.

In doing so, we will see one method of achieving out goal: a working Wallpaper file upload process.

However, back in the real world, doing this would fall quickly under the “technical debt” category.

The idea of this site is that it’s the real deal – a hobby site maybe, but a real, working website.

As administrators we would like to be able to log in to our site, add new wallpapers, see stats and stuff like that. These sorts of things keep our momentum going to add new features and keep improving the site. I find this is the best way I continually improve myself as a software developer.

Taking this further, it makes sense that as-and-when our wallpaper site becomes popular (hey, we can dream!) we will likely want to be adding new wallpaper content on a regular basis.

This means our Wallpaper file upload form is going to be getting battered.

Taking this from a different angle, you likely have code on other projects that is equally frequently used.

This kind of code absolutely needs to work, or you will be sat in the pub and your phone will ring and your boss will be angry as his / her client is angry. And that spoils your otherwise enjoyable life experiences.

but it was working when I left the office…

When I encounter code like this, I immediately reach for the test suite. It’s the insurance policy that keeps my phone from ringing, and ensures my pub visits are interruption free.

But stepping back into our application, we have our Symfony site as a whole, and then EasyAdminBundle where we need to start adding in this additional functionality.

The chances are, if we’ve never used Symfony, or never used EasyAdminBundle, or both, then we have no experience even doing this “the easy way”. That is to say, we have no idea how to do this without even worrying about writing tests.

It’s a big ask to correctly figure out all the requirements in advance to the point where we can write a bunch of failing tests that when passing would work as a file upload system. At least, it is in my opinion.

So here’s what I do in real life: I cheat.

I don’t bother with tests.

I create a new branch in git, and then I hack around until I have a solution that generally works. This can take minutes, hours, or sometimes days and weeks.

Going back to our wallpaper file upload, I would code up a solution that allows me to add, edit, and delete files. It very likely wouldn’t be pretty, but that doesn’t bother me.

What this does is it gives me the knowledge of how – roughly – this whole thing should work.

From here, I can switch back to master, create another new branch, and have another bite at the cherry – only this time, I have a starting point from which I can write some tests.

I will usually refer back to the untested implementation a bunch of times, and being able to switch branches and play about with some working code is often a massive help in defining assumptions and crafting test assertions.

And so, that is what we are doing in this – and next – week’s videos.

First, we are exploring how to simply get EasyAdminBundle file upload working.

Then next week we will re-do the implementation but using PhpSpec to guide us.

Until then, thanks for reading, have a great weekend and happy coding.

Chris

The Truth About Testing

In this week’s new videos we are diving head first into PhpSpec.

I am fully aware that this is a series that is aimed at beginners, but in my opinion it’s never too early to learn how to test.

Before we go further though, I do want to stress that PhpSpec is a highly opinionated approach to testing, and that opinion may not be one that you agree with. Or, more importantly, enjoy the feeling you get when you use it.

There are a couple of alternatives to PhpSpec that I have used and would recommend:

But this is somewhat misleading, as Codeception uses PhpUnit for unit testing.

Ok, so at this point, if you are someone new to testing, you may be feeling confused. A very valid question here is:

What the heck is unit testing?

My definition of unit testing is testing individual functions / methods.

There’s major benefits to doing this, which I am hoping to explore with you in these next few videos.

Let’s quickly cover unit testing by way of an example. Imagine we have a function that adds two numbers together:

function add($x, $y)
{
    return $x + $y;
}

We could manually test this:

echo add(1,2);

And we should expect to see 3.

One of the first things we do when writing code is find a way to output what we’ve done – to check if what we are doing is working. In PHP we can do this in a variety of ways: die , var_dump , echo , print_r , the list goes on.

This is a form of testing.

If all we are testing here is the output of a single function then heck, this is manual unit testing.

The problem is: we generally don’t have just one function. And if we do, that’s a different kind of problem in itself 🙂

Instead, what we will likely do is start using this add function in a variety of situations throughout our code base.

We might manually test those functions as well. In doing so, we indirectly test the other functions that function depends on.

And that’s all good.

Whilst all this logic is fresh in your head – whilst you’re deep into the systems internals – it’s all there as clear as day. But then another problem arises and you get sidetracked.

And when you return to this code even just a short time later, somehow the fog has begun to set in. You’re not quite sure how that method works anymore. You aren’t able to interpret that 70 line method exactly in your head.

At this point our untested code immediately becomes legacy code. At least, that’s my opinion.

We want to start refactoring it, to fix up all the confusion and restore that prior clarity.

But we can’t because the more we meddle, the more stuff breaks. Or may break. Coding is so stressful!!!!

zen-monk

Or, you can swap all this for zen.

You know your code works because the tests pass. Those tests didn’t pass to begin with. You’ve had to write code to prove how your system works. And now when you change your code, if your tests still pass then your system still works exactly as you intended.

And it’s surprising how, when doing this, you will break so many unexpected things when changing seemingly unrelated bits and pieces.

This takes your skill level further. You start to learn about better design as a result.

I’ve found PhpSpec will lead you towards a certain design.

That design is the distilled guidance from very clever people. Why not leverage their knowledge to help your project, but also you – to make your life easier, and less stressful?

Back to our function.

What if we had created our function using unit tests for guidance?

Well, we could take a look at using a unit test suite to write tests for this function.

However, the reality of it is, if you’re working with Symfony, you’re going to be writing code in a certain way.

Why not learn how to write unit tests in an environment just like your real world projects?

I think this is a better way to learn. It’s a little more effort upfront, but you’re learning how it can really and immediately help you become better on the type of code you have to write in your day job.

And so that’s what we are doing in these three videos:

https://codereviewvideos.com/course/let-s-build-a-wallpaper-website-in-symfony-3/video/testing-with-phpspec-to-guide-our-implementation

In case you haven’t been following along, in the past three video’s we’ve been learning how we can use EasyAdminBundle as a quick way to add a really nice UX to our admin area.

We can manage all our existing wallpapers from the back end, but it would be super useful to us if we could handle new wallpaper file uploads from the admin area, too.

EasyAdminBundle comes with a documented way to integrate with VichUploaderBundle.

We could have chosen to go that route, again leveraging the wisdom of the collective.

Instead we are doing some DIY. Our design decisions are not about sharing this code. We’re just exploring some concepts. We want to learn about file uploads, and hopefully improve ourselves as developers a little in the process.

The thing is, handling uploads is code that will be super important to the site and, if it all works, it will be used a lot.

This is the sort of problem that if you don’t handle up front, you’re going to be getting frantic calls from bosses and clients to fix as a matter of up-most urgency :/

No one wants to deal with that. Not you. Not your boss. Not the client.

So, when I hit on code like that, I reach for the unit tests.

https://codereviewvideos.com/course/let-s-build-a-wallpaper-website-in-symfony-3/video/using-phpspec-to-test-our-filemover

In the previous video we covered a little of how the design decisions came about, and started our test routine.

Now we’re going to write some specifications of how we expect this system to behave, if it is performing as expected.

🙂

This is the insurance policy that allows us to refactor our confusing code later on.

If the implementation we write now makes the tests pass, we can know for sure if our code changes don’t lead to failing tests, we have altered the logic without altering the outcome.

If your project survives, this makes it’s lifetime more enjoyable for you as a developer.

You’ll inevitably get to work on more features, not constant and stressful bug fixing.

https://codereviewvideos.com/course/let-s-build-a-wallpaper-website-in-symfony-3/video/symfony-dependency-testing-with-phpspec

Finally we get into how all this integrates with Symfony.

Understand these three pieces and the process essentially repeats over and over for whatever you want or need to test.

The truth about testing is that it’s a pain to get started.

But once you have tested one thing, you can use that one single thing as a reference and it becomes A LOT easier to add tests to an already tested system.

Ok, so that’s why I love testing.

Hopefully if you can invest just 15 minutes into your PHP skills this weekend, it will be these minutes you choose 🙂

I’d love to hear your thoughts on this series so far.

I’m also under way with the big back end refresh once again. I’m going to be deploying this new site as the topic of the upcoming Docker series, which I think is quite a cool way to cover it. It will at least show you how it all works behinds the scenes, if nothing else.

If you haven’t done so recently, why not check out what else is on the site?

Thanks for reading, and have a great weekend,

Chris

 

Checking An Edge Case With Symfony Voters

I love Symfony Voters. The Access Control List is, in the vast majority of cases, totally overkill for my security needs. Symfony’s Security Voters are a very quick and easy alternative.

Yesterday I got caught out by an edge case I had not been testing for.

A little background on this – I have been creating a RESTful API using Symfony 3, Behat 3, PHPSpec, and a few other interesting technologies. If you are interested in knowing a little more about this then be sure to check out the video tutorial series here on CodeReviewVideos where I have been documenting as I go.

The video tutorial series covers three different entities – Users (FOSUserBundle), Accounts, and Files (with FlySystem).

I want to ensure the currently logged in User has access to the resources they are requesting. I’ve been doing this with Voters.

It’s likely easier to follow along with these words if I give them more context. Here’s the code for the File Voter:

<?php

namespace spec\AppBundle\Security\Authorization\Voter;

use AppBundle\Model\AccountInterface;
use AppBundle\Model\FileInterface;
use AppBundle\Model\UserInterface;
use AppBundle\Security\Authorization\Voter\FileVoter;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;

class FileVoterSpec extends ObjectBehavior
{
    function it_is_initializable()
    {
        $this->shouldHaveType('AppBundle\Security\Authorization\Voter\FileVoter');
        $this->shouldImplement('Symfony\Component\Security\Core\Authorization\Voter\VoterInterface');
    }

    function it_supports_expected_attributes()
    {
        $this->supportsAttribute('view')->shouldReturn(true);
    }

    function it_doesnt_support_unexpected_attributes()
    {
        $this->supportsAttribute('1')->shouldReturn(false);
    }

    function it_supports_expected_class(FileInterface $file)
    {
        $this->supportsClass($file)->shouldReturn(true);
    }

    function it_doesnt_support_unexpected_class()
    {
        $this->supportsClass(new \StdClass())->shouldReturn(false);
    }

    function it_abstains_if_doesnt_support_attribute(TokenInterface $token, FileInterface $file)
    {
        $this->vote($token, $file, ['unsupported'])->shouldReturn(VoterInterface::ACCESS_ABSTAIN);
    }

    function it_abstains_if_doesnt_support_this_class(TokenInterface $token)
    {
        $this->vote($token, new \StdClass(), [FileVoter::VIEW])->shouldReturn(VoterInterface::ACCESS_ABSTAIN);
    }

    function it_denies_if_not_matching(
        TokenInterface $token,
        UserInterface $user,
        FileInterface $file
    )
    {
        $token->getUser()->willReturn($user);

        $file->belongsToUser($user)->willReturn(false);

        $this->vote($token, $file, [FileVoter::VIEW])
            ->shouldReturn(VoterInterface::ACCESS_DENIED);
    }

    function it_grants_if_matching(
        TokenInterface $token,
        UserInterface $user,
        FileInterface $file
    )
    {
        $token->getUser()->willReturn($user);

        $file->belongsToUser($user)->willReturn(true);

        $this->vote($token, $file, [FileVoter::VIEW])
            ->shouldReturn(VoterInterface::ACCESS_GRANTED);
    }
}

And the implementation:

<?php

namespace AppBundle\Security\Authorization\Voter;

use AppBundle\Model\AccountInterface;
use AppBundle\Model\FileInterface;
use AppBundle\Model\UserInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;

class FileVoter implements VoterInterface
{
    const VIEW = 'view';

    /**
     * @param string $attribute
     * @return bool
     */
    public function supportsAttribute($attribute)
    {
        return in_array($attribute, array(
            self::VIEW,
        ));
    }

    /**
     * @param string $class
     * @return bool
     */
    public function supportsClass($class)
    {
        $supportedClass = 'AppBundle\Model\FileInterface';

        return $supportedClass === $class || is_subclass_of($class, $supportedClass);
    }

    /**
     * @param TokenInterface            $token
     * @param null|FileInterface        $requestedFile
     * @param array                     $attributes
     * @return int
     */
    public function vote(TokenInterface $token, $requestedFile, array $attributes)
    {
        // check if class of this object is supported by this voter
        if (!$this->supportsClass(get_class($requestedFile))) {
            return VoterInterface::ACCESS_ABSTAIN;
        }

        // set the attribute to check against
        $attribute = $attributes[0];

        // check if the given attribute is covered by this voter
        if (!$this->supportsAttribute($attribute)) {
            return VoterInterface::ACCESS_ABSTAIN;
        }

        // get current logged in user
        $loggedInUser = $token->getUser(); /** @var $loggedInUser UserInterface */

        if ($requestedFile->belongsToUser($loggedInUser)) {
            return VoterInterface::ACCESS_GRANTED;
        }

        return VoterInterface::ACCESS_DENIED;
    }

}

This all works and is passing.

However, there is an edge case I hadn’t considered.

What happens if $requestedFile->belongsToUser($loggedInUser) fails, or returns null? Well, bad times as it happens.

Why might it fail? Well, let’s look at that interface, and the real method implementation:

<?php

namespace AppBundle\Security\Authorization;

use AppBundle\Model\UserInterface;

/**
 * Interface UserOwnableInterface
 * @package AppBundle\Security\Authorization
 */
interface UserOwnableInterface
{
    /**
     * @param UserInterface $user
     * @return bool
     */
    public function belongsToUser(UserInterface $user);
}

And the implementation:

    /**
     * @param UserInterface $user
     * @return bool
     */
    public function belongsToUser(UserInterface $user)
    {
        return $this
            ->getAccounts()
            ->filter(function ($account) use ($user) { /** @var $account AccountInterface */
                return $account->isManagedBy($user);
            })
            ->count() > 0
        ;
    }

Due to a mistake in my fixtures, the File wasn’t correctly being added to the Account I expected, so the call of getAccounts() was returning null.

In this case, I first fixed my fixtures as that situation should never really happen.

But what if it does? I don’t want to be throwing errors to the user. It’s easier to deny them access, log the fault (using monolog, and/or to ELK), and fix it in a slightly more controlled manner.

I added the following two specs to my FileVoterSpec test file:

    function it_denies_if_belongsToUser_throws(
        TokenInterface $token,
        UserInterface $user,
        FileInterface $file
    )
    {
        $token->getUser()->willReturn($user);

        $file->belongsToUser($user)->willThrow(new \Exception());

        $this->vote($token, $file, [FileVoter::VIEW])
            ->shouldReturn(VoterInterface::ACCESS_DENIED);
    }

    function it_denies_if_belongsToUser_returns_null(
        TokenInterface $token,
        UserInterface $user,
        FileInterface $file
    )
    {
        $token->getUser()->willReturn($user);

        $file->belongsToUser($user)->willReturn(null);

        $this->vote($token, $file, [FileVoter::VIEW])
            ->shouldReturn(VoterInterface::ACCESS_DENIED);
    }

And then changed up the implementation (uglify!):

    /**
     * @param TokenInterface            $token
     * @param null|FileInterface        $requestedFile
     * @param array                     $attributes
     * @return int
     */
    public function vote(TokenInterface $token, $requestedFile, array $attributes)
    {
        // check if class of this object is supported by this voter
        if (!$this->supportsClass(get_class($requestedFile))) {
            return VoterInterface::ACCESS_ABSTAIN;
        }

        // set the attribute to check against
        $attribute = $attributes[0];

        // check if the given attribute is covered by this voter
        if (!$this->supportsAttribute($attribute)) {
            return VoterInterface::ACCESS_ABSTAIN;
        }

        // get current logged in user
        $loggedInUser = $token->getUser(); /** @var $loggedInUser UserInterface */

        try {
            if ($requestedFile->belongsToUser($loggedInUser)) {

                return VoterInterface::ACCESS_GRANTED;
            }
        } catch (\Exception $e) {

            return VoterInterface::ACCESS_DENIED;
        }

        return VoterInterface::ACCESS_DENIED;
    }

Note the try/catch block. It’s definitely made the code uglier, but it ‘fixes’ a potential problem I hadn’t considered.

I say ‘fixes’ as this should never happen. But if I have learned anything over the last 15-odd years, it’s that those things that should never happen… they happen.

Mocking Collections in PHPSpec

phpspec-logoI’m a huge fan of PHPSpec, and its close cousin, Behat. I find when writing code in conjunction with PHPSpec, I am able to enter a rhythm that I have never found with any other tool.

I particularly enjoy the code generation functionality – describe some action, do a bin/phpspec run and have your methods created for you as you go. It really is quite a joy to use.

However, as with any tool, there is a learning curve.

I found the basics – the stuff described in the manual – to be straightforward enough that even when stuck, I could relatively quickly find my way through and get back on track.

Then, recently, I decided to build an application involving third party / social media providers for authentication using HWIOAuthBundle.

Along the way, I added the concept of a User object having a Collection (Doctrine\Common\Collections\Collection) of Account objects. Fairly common stuff, particularly if you have ever used Symfony at all.

The very basic idea would be something like this:

<?php

namespace AppBundle\OAuth\Connect;

use HWI\Bundle\OAuthBundle\OAuth\Response\UserResponseInterface;
use HWI\Bundle\OAuthBundle\Security\Core\User\FOSUBUserProvider as BaseClass;
use FOS\UserBundle\Model\UserManagerInterface;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Security\Core\User\UserInterface;

class ProfileConnector extends BaseClass
{
    /**
     * @var RequestStack
     */
    private $requestStack;

    public function injectRequestStack(RequestStack $requestStack)
    {
        $this->requestStack = $requestStack;
    }

    public function connect(UserInterface $user, UserResponseInterface $response)
    {
        $req = $this->requestStack->getMasterRequest();

        if ( ! $req->request->has('profiles')) {
            return false;
        }

        $selectedProfiles = $req->request->get('profiles');

        /** @var $user \AppBundle\Entity\User */
        /** @var $profile \AppBundle\Model\ProfileInterface */
        foreach ($user->getProfiles() as $profile) {
            if ( ! array_key_exists($profile->getId(), $selectedProfiles)) {
                continue;
            }
        }

        // something else here
        return true;
    }
}

 

This is a work in progress, so if it looks a little rough… hey, that’s why I do TDD. The refactoring will come in time.

Seeing the tests would likely also help:

<?php

namespace spec\AppBundle\OAuth\Connect;

use Doctrine\Common\Collections\ArrayCollection;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use HWI\Bundle\OAuthBundle\OAuth\Response\UserResponseInterface;
use FOS\UserBundle\Model\UserManagerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use AppBundle\Model\ProfileInterface;
use AppBundle\Entity\Profile;
use AppBundle\Entity\User;
use AppBundle\Entity\SocialMediaAccount;

class ProfileConnectorSpec extends ObjectBehavior
{
    private $user;
    private $response;

    function let(UserManagerInterface $userManager, User $user, UserResponseInterface $userResponse)
    {
        $this->user = $user;
        $this->response = $userResponse;

        $this->beConstructedWith($userManager, []);
    }

    function it_is_initializable()
    {
        $this->shouldHaveType('AppBundle\OAuth\Connect\ProfileConnector');
        $this->beAnInstanceOf('HWI\Bundle\OAuthBundle\Security\Core\User\FOSUBUserProvider');
    }

    function it_can_inject_the_request_stack()
    {
        $this->injectRequestStack(new RequestStack());
    }

    function it_can_handle_no_profile_accounts_being_selected()
    {
        $requestStack = new RequestStack();
        $requestStack->push(new Request());

        $this->injectRequestStack($requestStack);

        $this->connect($this->user, $this->response)->shouldReturn(false);
    }

    function it_can_connect_one_account_to_a_social_media_service(User $user, Profile $profile)
    {
        $profile->getId()->willReturn(16);
        $profiles = new ArrayCollection([$profile->getWrappedObject()]);

        $user->getProfiles()->willReturn($profiles);

        $requestStack = new RequestStack();
        $requestStack->push(new Request([], ['profiles'=>[16=>'on']]));

        $this->injectRequestStack($requestStack);

        $this->connect($user, $this->response)->shouldReturn(true);
    }

    function it_can_connect_multiple_accounts_to_a_social_media_service()
    {

    }
}

This is absolutely a work in progress. I even left in the last test, yet to be started.

As a quick side note, for the first time I have decided to declare commonly required objects by way of the let() method. I am not absolutely sure whether or not this is good practice, but it does seem to work as I intended it too. If you know differently, do let me know by way of leaving a comment – thanks !

Currently the tests are passing – with the exception of the pending example still to write.

What may be less obvious is how much effort I went through to get the it_can_connect_one_account_to_a_social_media_service() example to play ball.

I’ve spent a good few hours these past few evenings trying to figure out this error:

AppBundle/OAuth/Connect/ProfileConnector
  51  - it can connect one account to a social media service
      warning: array_key_exists(): The first argument should be either a string or an integer in
      /var/www/myproj/src/AppBundle/OAuth/Connect/ProfileConnector.php line 46

When I was seeing this error, the problematic test actually looked like this:

function it_can_connect_one_account_to_a_social_media_service(User $user, Profile $profile)
{
    $profile->getId()->willReturn(16);
    $user->addProfile($profile);

    $requestStack = new RequestStack();
    $requestStack->push(new Request([], ['profiles'=>[16=>'on']]));

    $this->injectRequestStack($requestStack);

    $this->connect($user, $this->response)->shouldReturn(true);
}

A bit of Google-foo told me I was likely going about this entirely the wrong way:

https://codereviewvideos.com/blog/wp-content/uploads/2015/10/everzet-stop-mocking-collections.png

When the creator of PHPSpec tells you (indirectly) that you are wrong, then… you are wrong.

I could actually see the problem – the implementation seemed correct, but PHPSpec sees my call $profile->getId() as returning an Object, and then throwing an error something along the lines of :

AppBundle/OAuth/Connect/ProfileConnector
  51  - it can connect one account to a social media service
      error: Object of class Prophecy\Prophecy\MethodProphecy could not be converted to string in
      /var/www/myproj/src/AppBundle/OAuth/Connect/ProfileConnector.php line 44

At first I figured… ok, well I won’t mock the User object – I can new that up – but when trying to add a mock Profile to the real collection, it all went a bit wrong:

AppBundle/OAuth/Connect/ProfileConnector
  50  - it can connect one account to a social media service
      error: Argument 1 passed to AppBundle\Entity\User::addProfile() must implement interface
      AppBundle\Model\ProfileInterface, instance of PhpSpec\Wrapper\Collaborator given, called in
      /var/www/myproj/spec/AppBundle/OAuth/Connect/ProfileConnectorSpec.php on line 54 and defined in
      /var/www/myproj/src/AppBundle/Entity/User.php line 44

That’s fine, I thought, I will use a real Profile object as well, because why not?

Well, I’ll tell you why not.

The test expects my Profile object to have an id of 16. I’m not about to add in a setId() method, that would be bonkers.

At heart, I knew as soon as I started adding in real objects that I was heading down the wrong path.

Generally I find that when PHPSpec is making your life hard it is because you are trying to do the wrong thing. Sooner or later you must stop resisting.

Star Trek: The Next Generation 365 (Star Trek 365)
Picard lost most of his hair due to frustrating late night bug fixing in ten forward. It’s all explained in the Season 3 episode Bynar2Hex.

Anyway, after quite a bit a lot of further hackery, I managed to find a working solution (as per the earlier sample):

    function it_can_connect_one_account_to_a_social_media_service(User $user, Profile $profile)
    {
        $profile->getId()->willReturn(16);
        $profiles = new ArrayCollection([$profile->getWrappedObject()]);

        $user->getProfiles()->willReturn($profiles);

        $requestStack = new RequestStack();
        $requestStack->push(new Request([], ['profiles'=>[16=>'on']]));

        $this->injectRequestStack($requestStack);

        $this->connect($user, $this->response)->shouldReturn(true);
    }

When I think about it now, it does make sense. This is similar to what I was trying to do, only this way conforms with the way PHPSpec expects me to behave 🙂

Both the User and Profile objects are properly mocked, but as per Everzet’s comment, we are returning a real ArrayCollection, which by way of $profile->getWrappedObject() will return the underlying Profile objects, rather than the PHPSpec wrapped objects / Object Prophecies.

This is exactly the sort of problem that my brother – an aspiring coder – would think I “just knew” how to fix. And that he should also be expected to know how to solve instinctively also.

Of course, the next time this comes up, I will know exactly how to solve it. It’s just if you were sat watching over my shoulder, you wouldn’t imagine the hours of my life that lesson took to learn 😉