Unit testing with model event listeners is broken

[bgrawi]

First see laravel/framework#1181 for context.

What winds up happening is that NONE of the event listeners for any of the traits ever get called after the first unit test is run. I currently have to use the hack provided by this comment laravel/framework#1181 (comment) in order to run more than one test using esensi/model.

I am inheriting my models directly from Esensi\Model\SoftModel, so the Esensi\Model\Model::bootTraits is only ever called on the first unit test. While yes, this is an issue with Laravel as a whole, the team seems to be recommending against assigning event watchers in a model's boot sequence regardless and have opted not to fix this issue. Instead, the generally accepted "non-hack" requires moving all event registration to app/start/global.php as per laravel/framework#1181 (comment)

This issue should be stated somewhere in your documentation. Also, the project should provide a way to quickly register all the required event listeners for models in app/start/global.php instead of in the boot process which has this severe defect. At the very least there should be some examples of manually registering the listeners.

Hey Ben-
This seems like a non-straightforward issue, so we'll need to spend some time looking at it. Someone here will update this issue after the holiday weekend. I'd be happy to look at a pull req for the hacky method, non-hacky method, or doc update this afternoon, though.
Thx!
MM

[dalabarge]

@bgrawi just wanted to let you know that we review these sort of things on Wednesdays. We closed all other issues today but didn't get a chance to get to this one. We'll review it as we have time before next Wednesday but like Matt mentioned, it's rather involved and will take some figuring out what's exactly going on.

As a follow up - have you actually confirmed that the events don't get fired on our unit tests? It sounds like you are testing from laravel/laravel application directly while we test this package without that overhead. So while I understand your custom application tests might not get fired, are you sure ours aren't getting fired? Like I said, we'll review in detail (thanks for bringing it up) but I'm just trying to figure out if you've actually observed this behavior in our tests or are guessing that's what's happening upon using our traits and your custom application tests?

[bgrawi]

Thanks for the update,

Yes, I was running my own tests in an application running laravel/laravel with esensi/model hooked into my models (to clarify, I'm not running your tests in conjunction with mine), but this is not an issue with my tests, as you can easily reproduce the problem in a fresh install of laravel. I also tried cloning this project separately and ran the unit tests which had no issues with them, so the problem is in the laravel/laravel -> esensi/model integration.

I looked into how your team was testing the its components and found that since you are not relying on laravel for calling your observers on an update or save (which I guess makes sense for unit-testing), the observers are just manually called. I don't see an issue with this per se, but it depends on the fact the the observers are being triggered at the appropriate times (e.g. when saving https://github.com/esensi/model/blob/master/tests/HashingModelObserverTest.php#L100).

See, the fact is that when you run in an external application, laravel only registers the event listeners/observers once when the tests start (because Eloquent::boot is static). This is fine for most real-world cases as the application is only set up once per request. But when unit-testing, Eloquent::boot is only called once in the beginning of the test suite and not after each test.

I, like others in the issue I linked to above (laravel/framework#1181), thought that this was a design flaw with Laravel itself, but as you can see in the issue comments, the team is doing nothing to address it. They instead recommended to move all observer/event registration out of the model's boot sequence and into the application bootstrapping/setup area which is called for every unit test apparently.

The bottom line is that your unit tests are fine, but since there are no integration/acceptance tests, this issue never cropped up.

Like I mentioned earlier, you can reproduce this problem from a fresh installation of both packages. Here are some high-level steps:

1. Install laravel/laravel and add esensi/model as a co-dependency
2. Set up a basic model following the instructions you have provided in the README
3. Setup at least one of the traits (e.g. set $hashable = ['password'];)
4. Setup a closure route that basically fills in the password field and then saves it.
5. Navigate to that route in your browser, and then verify in your database that the password field was properly hashed on save, (it should be).
6. Write a new Test that extends laravel's /app/tests/TestCase.php. (Ensure your testing environment is set up correctly as well)
7. Add TWO test methods in the class each have $this->call('GET', '/testroute'); in their bodies.
8. Check the database and you should see two new rows, one with a hashed password and the second one without.
9. At this point you can add some sentinels in the esensi/model package to test where and when certain things are being called in the lifecycle of the application.

Thanks, I hope I have been of help.

[dalabarge]

@bgrawi thanks for clarifying. Yes, we only unit test the package to ensure that our code is functional. Laravel is already unit tested to ensure that the event firing works if you implement it correctly. So there's no real need for us to do the integration/functional tests for this package. If you are running integration tests then I would suggest you look deeper into Laravel to see if there is a possible solution that Taylor will accept a PR for. The issue definitely lies up stream when it comes to those types of tests.

As possible solutions to help you just get it working since clearly it boots atleast once, could you just refresh the application between tests? http://laravel.com/docs/testing#refreshing-the-application Binding your events in a globals file seems rather un-SOLID but obviously it does work according to your tests and other's comments. With Laravel there are more than one way of doing things so sometimes you have to go with the way that works over the more elegant way or dig deeper to get an upstream fix.

One thing we ran into with our unit testing was that we had to ensure that traits were in the right order because from our actual integration usage we found that events weren't fired in the right sequence. You might look into that a bit to figure out a way to setup some unit tests that will guarantee integration tests work by ensuring that bindings are sequenced correctly.

I'll add some notes to the docs for others to find this topic. We'll update this issue when we or someone else solves the integration tests.

[dalabarge]

Updating this issue with a possible work around according to this comment: laravel/framework#1181 (comment)

[dalabarge]

@davidwinter it has disappeared because Taylor decided to deprecate issues in favor of pull requests only, using Liferaft preferably. It was disappointing to us too because there were a lot of great issues in there that had code and work arounds that just went away with that decision. In fact we had several proposals in the issues that while didn't include a PR, did include a code snippet that we were leaving up to Taylor to be inspired from and make better but could have been copy pasted in with any other related code update. Those all got thrown out too.

As you found, that PR raises the issue again.

[davidwinter]

Thanks @dalabarge - was completely unaware issues had been removed - and can't believe it really to be honest.

[kevdowney]

Great topic! I too had run into this issue and implemented the hack solution, but more cleanly as a trait. Maybe if I post this here you can use it, unfortunately I don't have an open source project to commit this to.

trait TestHelperTrait {

    /**
     * [$models description]
     * @var array
     */
    protected static $models = array();

    /**
     * [getModels description]
     * @return array [description]
     */
    public static function getModelClasses()
    {
        $files = array();

        // Get all models in the Model directory
        $pathToModels = app_path().'/models';

        // Get all entities in the Src directory
        $srcPath = app_path().'/src';   // <-- EDIT THIS LINE WITH YOUR OWN PATH
        $srcPSR4Namespace = 'My/Stuff';  // <-- EDIT THIS LINE WITH YOUR PSR4 namespace to /src dir

        // loop through models dir
        foreach (new \DirectoryIterator($pathToModels) as $fileInfo) {
            if ($fileInfo->isFile() AND ! $fileInfo->isDot() AND $fileInfo->getExtension() === 'php') {
                $name = $fileInfo->getBaseName('.php');

                // Exclude Base models
                if (strpos($name, 'Base') === FALSE) {
                    // Add model to list
                    $files[$name] = $name;
                }
            }
        }


        // entities are models too, loop those also.
        foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($srcPath)) as $name => $fileInfo) {

            // Exclude Base models and Interfaces, only include Entity
            if (strpos($name, 'Base') === FALSE AND strpos($name, 'Interface') === FALSE AND strpos($name, 'Entity.php')) {
                $name = $fileInfo->getBaseName('.php');
                $path = str_replace(DIRECTORY_SEPARATOR, '\\', str_replace($srcPath, $srcPSR4Namespace, $fileInfo->getPath())).'\\';

                // Add model to list
                $files[$path.$name] = $path.$name;
            }
        }

        return $files;
    }

    /**
     * [resetEvents description]
     * @return [type] [description]
     */
    protected function resetEvents()
    {
        // Reset each model event listeners.
        foreach (self::$models as $model) {

            // Flush any existing listeners.
            call_user_func(array($model, 'flushEventListeners'));

            // Reregister them.
            call_user_func(array($model, 'boot'));
        }
    }
}

Now in your TestCase.php

class TestCase extends Illuminate\Foundation\Testing\TestCase
{
    use Test\Helpers\Traits\TestHelperTrait;

    /**
     * Creates the application.
     *
     * @return \Symfony\Component\HttpKernel\HttpKernelInterface
     */
    public function createApplication()
    {
        $unitTesting = true;

        $testEnvironment = 'testing';

        $start = require __DIR__.'/../../bootstrap/start.php';

       ...

        // Load the model files from models dir
        self::$models = self::getModelClasses();

        return $start;
    }


    /**
     * Initialize our test state.
     *
     * @return void
     */
    public function setUp()
    {
        parent::setUp();

        // reset the models event bindings
        $this->resetEvents();
    }

[Alexwijn]

I as well had this issue and used the resetEvents method code of @kevdowney. And I put the following in my createApplication method too since my models are also in packages:

\Event::listen('eloquent.booted*', function ($model) {
  if (is_a($model, Model::class) && !is_a($model, Pivot::class)) {
      self::$models[] = get_class($model);
  }
});

With the code above I do not need the getModelClasses method that @kevdowney wrote.