Hooks (before, after) should support expectations

[ciarand]

Just found a bug in my tests caused by a misunderstanding of how beforeEach and afterEach work in the context of expectations.

I use prophecy for mocks, although this is framework agnostic. To test predictions, I've been using something like this:

<?php

use Prophecy\Exception\Prediction\AggregateException;

describe("blah blah", function () {
    $prophet = new Prophecy\Prophet;

    afterEach(function () use ($prophet) {
        $check = function () use ($prophet) {
            $prophet->checkPredictions();
        };
        expect($check)->not()->toThrow("AggregateException");
    });

    it("should do something", function () use ($prophet) {
        $mock = $prophet->prophesize();
        $mock->myMethod()->shouldBeCalled();

        $caller = new MethodCaller();
        $caller->call($mock->reveal());
    });
});

This silently succeeds though, because expectations in the afterEach (and beforeEach, although I don't think that's particularly useful) are not considered. I'd like, similar to PHPUnit, to be able to fail a test from the teardown method.

Is this a change you'd be interested in merging in? If it's not, would you be interested in merging in a change to the documentation to point this behavior out?

[danielstjules]

I'm open to either a merge in documentation, or a change in behaviour. :) I like the idea of being able to include expectations in hooks. I'd imagine it would be limited to the beforeEach/afterEach hooks? Since I'm not sure what the expected behaviour would be if included in a suite's before/after. I don't think I'd want to it to fail the first spec of the suite.

If you were to submit a change, it could get a bit messy, since I'll admit pho's architecture isn't the most flexible. I haven't had a lot of time for open source this past month, but I'm hoping to get back to it shortly. When I do - I'm planing to do a light rewrite of pho's runner to use an event dispatcher. It'll make things a lot easier for some new features I hope to add.

So, it's up to you! If you can come up with a clean way of implementing this behaviour, I'd be happy to merge. Otherwise I'd say a documentation fix would suffice, and the feature could be introduced after the runner gets cleaned up.

[ciarand]

Thanks for the prompt response.

tl;dr it probably could be done, but it'd be hacky and hard to test.

Long version:

I spent an hour or two yesterday looking for an easy way to implement it. It can be done, but I couldn't think of a decent way to test it so I didn't bother committing the change.

Imo, it can be tacked on by changing the runSpecs method in the Runner class from:

$this->runRunnable($spec);

$this->reporter->afterSpec($spec);
$this->runAfterEachHooks($suite);

To something like this:

$this->runRunnable($spec);

try {
    $this->runAfterEachHooks($suite);
} catch (\Exception $exception) {
    $spec->exception = $exception;
    $spec->fail();
}

$this->reporter->afterSpec($spec);

Obviously that needs some adjustments elsewhere ($spec->exception is private right now, $spec->fail() isn't a method yet, and it should probably distinguish between the types of exceptions similar to how Runnable does). Trouble is that feels like a hack, and, more importantly, I'm stumped on how to test it.

It feels like a rewrite for some of the core wouldn't be a bad idea. The biggest stopping point for making this change was the level of intermingling between the objects.

The other benefit beyond the abstract "clean code" idea is that if you could separate the runner from the result set you could, feasibly, add a translation layer between a PHPUnit reporter (the TestListener interface) and Pho. That'd let you piggyback off the work already done for some of the PHPUnit reporters, including JSON, TAP, the Agile reporter, and even Nyancat. I'd be happy to implement something like that post-rewrite.

---

Anywho, #8 adds a note to the documentation along with an example of how to check expectations in a mostly-DRY way.

[danielstjules]

I appreciate you looking into a solution - I figured it would be tough to implement cleanly. As you probably noticed when thinking of how to test the behaviour, pho\Runner is currently responsible for far too much. It sorta ballooned out from a lack of planing.

The rewrite should simplify things, making them much easier to test, and doing exactly what you mentioned - separating results from the runner. This will mean being able to offer xunit and json reporters (without restricting console output), as well as code coverage.

And good idea with writing a translation layer! I hadn't thought of that. Being able to leverage that work would be nice. I'm not sure how the PHPUnit_Framework_TestListener interface would help though - since that's for adding your own output when using phpunit as the runner. I'm assuming what you meant was to still use pho as the runner? I'd love to figure out a way that didn't explicitly introduce phpunit as a dependency, but rather benefited from it if a flag was used, for example.

[ciarand]

👍

Yeah, that's what I meant. I got the direction wrong, hahaha. I imagine it being something like --reporter phpunit-tap or similar. I don't think PHPUnit should be a requirement, but being able to specify a reporter from the PHPUnit src if it's present would be awesome.

[danielstjules]

Closing in ref to #8

Thanks again. :)