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:

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

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:

And the implementation:

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:

And the implementation:

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:

And then changed up the implementation (uglify!):

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.

How to fix: You have requested a non-existent service “test.client” with Behat 3

So, it’s been a long day of development.

And the last 50 minutes have been particularly painful.

I’ve been bringing FOSUserBundle, Dunglas API Bundle, Lexik JWT Bundle, and Behat (amongst a good few others) together into one project for the first time.

Most of today has been productive. I’ve solved two of my big headaches, but as the hours have gone by, tiredness has set in.

And rather than call it a night, I did my usual “I’ll just see if I can…” once too many times, and buggered everything up.

If I had been behaving, I could have quickly done a little git bisect magic and found the source of my woes. Alas, I had not been behaving.

Anyway, the issue:

Run bin/behat , encounter error:

Wot do?

Well, there is a major shortage of Google help on this one.

The two most likely answers were not it.

Behat had been working just fine. I knew it was my mistake. But the project is growing and there’s just a ton of possible files that could have changed. When will I learn?

As it turned out, I had changed my  behat.yml file in my tired stupor:

I’d changed  env to a new Symfony Environment I had created so as not to keep messing up my local  dev database every time I re-ran behat.

In my case changing back to  env: "test" solved all my problems.

Ok, well not all of them… plenty of work still to do. But that’s it for this evening.

I hope that saves someone a headache in the future.

Time to commit and go to bed.

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:

 

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:

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:

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

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 :

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:

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):

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 😉

 

Behat 3 Tables and TableNode Examples

Behaviour Driven Development (BDD) with Behat 3 is a thing of beauty. When combined with PHPSpec you get something I am hugely excited about.

However, there are many ways in which BDD can be daunting not just for the new-comer, but for a new project in general.

Once you have a feature or two written up, copy / pasting and doing a little editing can yield quick results but writing the code that lives in the underlying Feature Context can be a little harder. For me, this was most evident when dealing with Scenarios that contained tabular data.

I made myself a little Behat TableNode cheat sheet to help – at a glance, and whilst in my IDE (PHPStorm btw) – figure out just what might be in my TableNode objects for the specific methods available on that class.

This is an example of the scenario I was working with:

Behat 3 TableNode var_dump

And a dump of the resulting TableNode:

var_dump($table);

var_dump($table->getRowsHash());

var_dump($table->getColumnsHash());

Hopefully this is as useful a reference to you as it has become for me. Being able to quickly ‘guess’ what is going to be in my TableNode objects and where has helped save me a good deal of time already.