"the rumours of this blog's demise... [etc]". Yeah. I'm still here. More about that in a separate article.
Here's a completely fictitious scenario which bears absolutely no relation to a code review discussion I had at work today. I don't even know why I mention work. This, after all, has nothing to do with my day job. Or my colleagues. I'm just making it all up.
Here's some code I just made up for the sake of discussion:
<?php
namespace me\adamcameron\someApp\service;
use \me\adamcameron\someApp\exception;
class SomeService {
public function getTheThingFromTheWebService($id){
// bunch of stuff elided
try {
$result = $this->connector->getThing($id);
// bunch of stuff elided
return $finalResult;
} catch (exception\NotFoundException $e) {
// bunch of stuff elided
throw new exception\ServiceException("The Thing with ID $id could not be retrieved");
}
}
}
Basically we're calling a web service, and the connector we use wraps up all the HTTP bumpf, and with stuff like 404 responses it throws a NotFoundException up to the service tier so that the service doesn't need to concern itself with the fact the connector is dealing with a REST web service. It could just as easily be connecting straight to a DB and the SELECT statement returned zero rows: that's still a NotFoundException situation. That's cool. We're all-good with this approach to things. For reasons that are outwith this discussion (TBH, I'm slightly contriving the situation in the code above, but maintaining the correct context) we don't want to let the NotFoundException bubble any further, we want to throw a different exception (one that other upstream code is waiting to catch), and provide a human-friendly message with said exception.
But for testing, I need to actually make sure that the Service handles this exception properly, and the two elements of this are that it chucks its own exception, and it includes that human-friendly message when it does.
So I've tested both of those.
Because I'm a well-behaved, team-playing dev, I TDD my code as I go, and
<?php
namespace me\adamcameron\someApp\test\service;
use \namespace me\adamcameron\someApp\service\SomeService;
/** @coversDefaultClass \me\adamcameron\someApp\service\SomeService */
class SomeServiceTest extends PHPUnit_Framework_TestCase {
public function setup() {
$this->setMockedConnector();
$this->testService = new SomeService($this->mockedConnector);
}
/**
@covers ::getTheThingFromTheWebService
@expectedException \me\adamcameron\someApp\exception\ServiceException
@expectedExceptionMessage The Thing with ID 1 could not be retrieved
*/
public function testGetTheThingsFromTheWebServiceWillThrowServiceExceptionWhenIdNotFound() {
$testId = 1;
$this->mockedConnector
->method('getThing')
->with($testId)
->will($this->throwException('\me\adamcameron\someApp\exception\NotFoundException'));
$this->testService->getTheThingFromTheWebService($testId);
}
private function setMockedConnector(){
$this->mockedConnector = $this->getMockBuilder('\me\adamcameron\connector\MyConnector')
->disableOriginalConstructor()
->setMethods(['getThing'])
->getMock();
}
}
I've left a bunch of contextual and mocking code in there so you can better see what's going one. But the key parts of this test are the annotations regarding the exception.
(we're using an older version of PHPUnit, so we need to do this via annotations, not expectations, unfortunately. But anyway).
This went into code review, and I got some feedback (this is a paraphrase. Of, obviously, a completely fictitious conversation. Remember this is not really work code I'm discussing here):
- Not too sure about the merits of testing the whole message string here. For the purposes of testing, we don't care about text, we just care about the $id being correct. Perhaps use @expectedExceptionMessageRegExp instead, with like "/.*1.*/" as the pattern.
- not sure 1 is a great ID to mock, TBH
Well I can't fault the latter bit. If one is gonna be actually checking for values, then use values that are really unlikely to accidentally occur as side-effects of something else going on. Basically 0 and 1 are dumb test values. There's always gonna be a risk they'll end up bubbling up via an accident rather than on purpose. I usually use random prime numbers (often taken from this list of the first 1000 primes). They're just not likely to coincidentally show up somewhere else in one's results at random.
But the first bit is interesting. My first reaction was "well what's the important bit here? The number or the error message?" We wanna check the error message cos given that suggested regex pattern the exception could simply return 1, which is not help to anyone. And yet that'd pass the test. Surely the important thing is the guidance: "The Thing with ID [whatever] could not be retrieved".
But that's woolly thinking on my part. I'm not doing tests for the benefit of humans here. And no code is every gonna rely on the specifics of that message. TDD tests (and unit tests in general) are about testing logic, not "results". And they don't give a shit about the Human Interface. We've got QA for that.
There's no need to test PHP here. If we have this code:
throw new exception\ServiceException("The Thing with ID $id could not be retrieved");
We don't need to test:
- that PHP knows what a string is
- that PHP knows how to interpolate a variable
- that one can pass a string to an Exception in PHP
- etc
What we do need to test is this:
public function getTheThingFromTheWebService($id){
// bunch of stuff elided
try {
$result = $this->connector->getThing($id);
// bunch of stuff elided
return $finalResult;
} catch (exception\NotFoundException $e) {
// bunch of stuff elided
throw new exception\ServiceException("The Thing with ID $id could not be retrieved");
}
}
- The ID comes in...
- ... it's used...
- ... and when the call that uses it fails...
- ... we use it in the exceptional scenario.
The logic here is that it's the same ID that's passed to the function is the one reported in the exception message. Not what the exception message is: that's PHP's job.
So in reality here... we just need to test the ID is referenced in the exception. TBH, I'd not even usually bother doing that. It's just we're pushing to have more useful exception messages at the moment, so we're trying out "enforcing" this behaviour. I think it's a worthwhile exercise at least whilst the devs are getting used to it. Once it's part of our standard practice, I reckon we can stop worrying about what's in the error message.
But in the interim I'm gonna fix me test and see if I can pass this code review...
Righto.
--
Adam
