Eloquent model events are not triggered when testing
PascalZajac opened this issue · 55 comments
The various Eloquent model events ending with ing are not being triggered during tests.
I discovered this a while ago and assumed it was intended functionality (forcing you to test your event handlers separately, much as route filters are not run in test mode).
However today I discovered that the post-action event handler created is triggered when run in test mode. Is there a reason for the discrepancy?
Upon further investigation, the behaviour gets weirder. It seems like the model event only executes the very first time the action occurs within the test suite.
To explain my particular scenario: I have an Image model, and an associated ImageService which handles created and deleted events. When I first wrote the unit tests for the service, the model events were triggered correctly. I then added an ImageController, with tests covering creation of an Image via the controller. Suddenly, my ImageService tests began to fail. Disabling the tests for the ImageController would fix the ImageService tests, as would manually invoking the ImageService event methods within the ImageService test.
Paste test.
On May 5, 2013, at 2:58 AM, Pascal Zajac notifications@github.com wrote:
Upon further investigation, the behaviour gets weirder. It seems like the model event only executes the very first time the action occurs within the test suite.
To explain my particular scenario: I have an Image model, and an associated ImageService which handles created and deleted events. When I first wrote the unit tests for the service, the model events were trigger correctly. I then added an ImageController, with tests covering creation of an Image via the controller. Suddenly, my ImageService tests began to fail. Disabling the tests for the ImageController would fix the ImageService tests, as would manually invoking the ImageService event methods within the ImageService test.
—
Reply to this email directly or view it on GitHub.
Ok, this is the most succinct example I can produce:
public function testFolderCreationAndDeletion()
{
// Create a new image.
$image = Image::create(array());
// Make sure a new folder was created.
$path = ImageService::getPath($image);
$this->assertTrue(is_dir($path)); // Passes.
// Cleanup.
$image->delete();
$this->assertFalse(is_dir($path));
}
public function testAgain()
{
// Create a new image.
$image = Image::create(array());
// Make sure a new folder was created.
$path = ImageService::getPath($image);
$this->assertTrue(is_dir($path)); // Fails
// Cleanup.
$image->delete();
$this->assertFalse(is_dir($path));
}The exact same test, if run again as a separate test, will fail where noted.
Can you just paste the entire test class that fails?
On May 5, 2013, at 6:31 PM, Pascal Zajac notifications@github.com wrote:
Ok, this is the most succinct example I can produce:
public function testFolderCreationAndDeletion() { // Create a new image. $image = Image::create(array()); // Make sure a new folder was created. $path = ImageService::getPath($image); $this->assertTrue(is_dir($path)); // Passes. // Cleanup. $image->delete(); $this->assertFalse(is_dir($path)); } public function testAgain() { // Create a new image. $image = Image::create(array()); // Make sure a new folder was created. $path = ImageService::getPath($image); $this->assertTrue(is_dir($path)); // Fails // Cleanup. $image->delete(); $this->assertFalse(is_dir($path)); }The exact same test, if run again as a separate test, will fail where noted.
—
Reply to this email directly or view it on GitHub.
use Instinct\Image;
use Instinct\User;
class ImageServiceTest extends TestCase {
public function testFolderCreationAndDeletion()
{
// Create a new image.
$image = Image::create(array());
ImageService::created($image);
// Make sure a new folder was created.
$path = ImageService::getPath($image);
$this->assertTrue(is_dir($path));
// Cleanup.
$image->delete();
ImageService::deleted($image);
$this->assertFalse(is_dir($path));
}
public function testEntityCleanup()
{
// Create a new image.
$image = Image::create(array());
ImageService::created($image);
// Create a new user.
$user = User::create(array(
'image_id' => $image->id
));
// Now delete the image.
$image->delete();
ImageService::deleted($image);
// Reload the user and make sure the image was removed from their profile.
$user = User::find($user->id);
$this->assertEquals(0, $user->image_id);
// Cleanup.
$user->delete();
}
}That's the whole class, as it stands, including my manual invocations of ImageService methods (which should actually be triggered by event bindings, and are when run outside of a testing environment).
I've done some further investigation on this. It seems that the Dispatcher class is re-instantiated between each test run. Some events, like artisan.start, are re-registered on each test run. However, Eloquent model events do not seem to be re-registered - they are registered once the first time the class is referenced anywhere in the test suite, and that's it.
Honestly I don't get your test. You're passing model instances into the events methods. What is that even supposed to do?
Ok, I think the class definition for the Image model may be relevant:
namespace Instinct;
class Image extends \Eloquent {
/**
* The database table used by the model.
*
* @var string
*/
protected $table = 'images';
/**
* The fields that can be changed via mass assignment.
* @var array
*/
protected $fillable = array(
'extension',
'title',
'description'
);
}
// Register event handlers.
Image::created('ImageService@created');
Image::deleted('ImageService@deleted');As you can see, in the model's class file I'm defining two event handlers. When an Image model is created, I want a folder to be created on the file system to handle the actual image which will soon be uploaded. When an Image is deleted, I want to remove the files from disk and delete the folder.
Now comes my assumption: since I create the event handlers in the model class file, they are not being reloaded between tests because composer does a require_once on the file. Thereafter it already has the class definition, so the file is never reloaded, and the event bindings are never recreated.
So it seems like the problem isn't with Laravel's event system but rather with where I'm defining my event bindings. Doing so in the model's class file seemed logical so they were clearly grouped together. Is there somewhere else I should put these sort of bindings?
I read back over the documentation for Eloquent events and saw the static boot method (that I had somehow not noticed before). So I duly moved my event bindings into the boot method for each model, but the problem persists - the boot code is only run the first time the model is encountered in the test suite, but the event dispatcher is re-instantiated between each test.
Is there a reason for resetting the event dispatcher?
I would move your event bindings into a separate method like Image::registerEvents(); and then you can call that from the setUp method.
That creates a different set of problems though - the boot method is still called the first time the model is referenced in the test suite, so in the first unit test that uses the class, if you have a setUp method as well, two event listeners will be created.
I appreciate that we've travelled far from the original assertions of this ticket, so I'm happy to open a new one, but the underlying issues remain unresolved - either the Dispatcher class needs to discard attempts to register the same event handler twice, or the Dispatcher should not be re-instantiated between test executions. I'm not sure which is better/possible.
Could also use Model::flushModelEvents to clear them out.
On May 17, 2013, at 11:20 PM, Pascal Zajac notifications@github.com wrote:
That creates a different set of problems though - the boot method is still called the first time the model is referenced in the test suite, so in the first unit test that uses the class, if you have a setUp method as well, two event listeners will be created.
I appreciate that we've travelled far from the original assertions of this ticket, so I'm happy to open a new one, but the underlying issues remain unresolved - either the Dispatcher class needs to discard attempts to register the same event handler twice, or the Dispatcher should not be re-instantiated between test executions. I'm not sure which is better/possible.
—
Reply to this email directly or view it on GitHub.
Ah ok, that method (which is actually Model::flushEventListeners) hadn't been added the last time I updated. That's neater, now I can just maintain an array of classes to flush and reboot between tests where I need to. Thank you.
So is the solution for unit tests to
Create Model::registerEvents();
Call that method in the boot method.
Then in unit tests, in the setup method call
$modelsToUpdate = array('User', 'Group', ... etc)
And loop through the models to reset the events?
Model::flushEvenListeners() Model::registerEvents();
@markalanevans Laravel already looks for a static boot method on the models under normal circumstances, so I wound up with code in app/tests/TestCase.php along the lines of:
public function setUp()
{
parent::setUp();
$this->resetEvents();
}
private function resetEvents()
{
// Define the models that have event listeners.
$models = array('Calendar', 'Event', ...);
// Reset their event listeners.
foreach ($models as $model) {
// Flush any existing listeners.
call_user_func(array($model, 'flushEventListeners'));
// Reregister them.
call_user_func(array($model, 'boot'));
}
}This is a hack or a work-around, not a solution to a problem that still remains in the framework (as far as I know)
I've face this similar problem recently (see #4036), and I've investigated quite a bit. Sadly, most of the problem is related to static/non-static behavior (the Model class having a static $booted relying on a static $dispatcher which changes on a new application).
A potential solution (which is far from the best) would be to reset the booted models when the dispatcher used is changed in Model::setEventDispatcher.
public static function setEventDispatcher(Dispatcher $dispatcher)
{
static::$dispatcher = $dispatcher;
static::$booted = array();
}I agree with @buzzware that this is a workaround and not a fix for the issue. Events being run once then cleared for subsequent tests creates an inconsistent testing environment. Maintaining a list of models which contain observers and reattaching their events isn't a fix - but a workaround.
Maybe models with events can be detected somehow and this functionality can be pushed down into a core TestCase class?
We just ran into this exact issue setting up our tests after migrating from CodeIgniter. The above-mentioned method works but it is far from ideal.
Same story here. Bummer.
The solution to this is to register your model events in app/start/global.php, not the model's boot() method as recommended in the documentation, and especially not in the same file that the model class is defined! That's mixing procedural code with non-procedural and is just a huge no-no.
Can you elaborate @anlutro? I would much prefer to define these event bindings outside the model class, but when I moved them to app/start/global.php they didn't run (according to my unit test suite), even after I wrapped the lines in an App::before() block.
Same problem here. Any tips on this?
@PascalZajac: Your solution didn't help as well. It recreates listeners, but still the event isn't triggered on second call from unit tests. I am trying to use the creating event.
@harshilmathur the hack I outlined above last year still works for me in my test suite. Feel free to paste the code you're using in a gist and I'll comment on it.
@PascalZajac FYI I have event listeners in the global file and my tests run fine without any hacks.
Hrm I wonder what I did wrong then.
I have same issue. Event is not triggered on second call from unit tests when using "protected static function boot()" in my model. I had to use @PascalZajac resetEvents() code in my TestCase.php file to fix the issue
I've modified @PascalZajac code to automatically register and reset every model in the application - rather than having to manually update the array each time:
private function resetEvents()
{
// Get all models in the Model directory
$pathToModels = '/vagrant/app/models';
$files = File::files($pathToModels);
// Remove the directory name and the .php from the filename
$files = str_replace($pathToModels.'/', '', $files);
$files = str_replace('.php', '', $files);
// Remove "BaseModel" as we dont want to boot that moodel
if(($key = array_search('BaseModel', $files)) !== false) {
unset($files[$key]);
}
// Reset each model event listeners.
foreach ($files as $model) {
// Flush any existing listeners.
call_user_func(array($model, 'flushEventListeners'));
// Reregister them.
call_user_func(array($model, 'boot'));
}
}
Edit: this is just a temp workaround until a proper Laravel solution is done.
All hacks & bypasses aside, but why isn't the issue fixed in laravel itself? or is it the intended behavior because it seems the issue is closed since long?
I agree with @harshilmathur I think a solution for this kind of issue should be provided by Laravel and should be part of the framework.
I'm not sure whether the comparison would be apt, but in Rails (which is ideologically very close to Laravel) this works out-of-box. You define an watcher ('before_save' for ex.) and it just works. In tests, in production, everywhere, without the need for manual binding.
All hacks & bypasses aside, but why isn't the issue fixed in laravel itself? or is it the intended behavior because it seems the issue is closed since long?
But, the next response to #1181 (comment) says that it does not work, isn't it?
@slava-vishnyakov have you tried it? I been testing model observer class without any issue all this while using unit testing approach (no hack needed). Also has anyone tried running the test using phpunit --process-isolation flag?
@crynobone You're right. I should try it.
Yes, actually @anlutro's way works!
slava-vishnyakov/fake-model-laravel@4d72ab9...8487ec9
Maybe we should get this added to Laravel Docs? http://laravel.com/docs/eloquent#model-events
@crynobone : @anlutro's comment has other sets of issues. The method doesn't work if you bind the event to the parent class and expect it to work for child class's event. The boot method works for that.
This question explains what I mean: http://stackoverflow.com/questions/21517258/laravel-eloquent-event-inheritance
Currently, I have to declare events using global.php for all child classes which isn't very nice way to do it if you have lot of child classes requiring same event binding.
I agree with @harshilmathur, model event binding inheritance is a very great feature and it needs to declare bindings in the boot() method.
And if mixing procedural with non-procedural code is a problem for somebody here, we have to remove PHP right now ! There is a lot of code in models that is close to configuration code ($fillable attribute, relationship declaration...) and, for me, event binding is in the same category (they are just one line functions).
I think a solution for this kind of issue should be provided by Laravel and should be part of the framework.
+1, Will be work Model Events out of the box? Thanks.
@harshilmathur I found your comment when searching for a way around that exact issue today. I ended up doing the following:
Event::listen('eloquent.saving: *', function($model) {
if ($model instance of \My\Base\Model) {
// event code here
}
});This allowed all of my models to inherit the event handling code without binding to any Eloquent models in 3rd party packages. Did you happen to come up with a better way?
@travis-rei I ended up overriding the save function of the eloquent model.php in my eloquent model class to do what I wanted and then call the parent's save function. This solved all my issues because it can now be unit tested without issues as well as inherited saving me from writing same function in multiple child classes.
Throwing some support in for a solution here. When using third party packages model events have to be registered in the boot() method, registering them in global.php isn't really an option. Would be great to see something baked in instead of having to hack it together.
When using third party packages model events have to be registered in the
boot()method
Problem being?
The discussion above seemed to indicate that this problem could be resolved if the model events were registered in global.php instead of in the boot() method.
Ah sorry, let me clarify. In a package, the equivalent of app/start/global.php is its service provider's boot() method, which is where you would place event listeners - not in a model's boot() method.
Ah, gotcha. This still wouldn't work in my instance (referring to watson/validating where it's a trait that registers an observer when the trait is booted on a model.
I ran in the same issue today and seem to have fixed it by adding Model::flushEventListeners(); and Model::boot(); before each subsequent test. (sugested by PascalZajac) Should we not have some mention of this in the docs?
Very frustrating. Have just spent quite some time debugging this and then stumbled across this issue.
I'm using @dwightwatson great watson/validating package, and it seems really hacky in my tests to flush and reboot before each. And if it is the road we should be using, then I think this should be mentioned somewhere in the unit testing docs.
oh no, I've struggled with this problem for more than 3 hours and then I just now find now there are a lot of related issues in here. Can't believe it already exists for so long and Laravel doesn't want solve it at all?!
@taylorotwell any new thoughts on this? Do you consider this not to be an issue with Laravel, and we should just use the fixes above? If not, maybe re-open the issue so that other people can find it more easily?
This is super crazy! Here is my hacky workaround:
trait ModelEventOverride {
public $events = [ 'saving' => 'beforeSave', 'saved' => 'afterSaved',
'creating' => 'beforeCreate', 'created' => 'afterCreated',
'updating' => 'beforeUpdate', 'updated' => 'afterUpdated',
'deleting' => 'beforeDelete', 'deleted' => 'afterDeleted',
'validating' => 'beforeValidate','validated' => 'afterValidated'
];
protected function fireModelEvent($event, $halt = true)
{
$_event = $event;
if ( isset(static::$dispatcher) ) {
$event = "eloquent.{$event}: ".get_class($this);
if ( !empty(static::$dispatcher->getListeners($event)) )
{
$method = $halt ? 'until' : 'fire';
return static::$dispatcher->$method($event, $this);
}
}
$event = $_event;
if ( ! isset($this->events[$event])) return true;
$method = $this->events[$event];
if(method_exists($this, $method))
{
return call_user_func(array($this, $method),$this);
}
return true;
}
}
lol
Just bumped into this one myself in Codeception's acceptance tests. Spent whole morning debugging just to discover that events aren't fired while in test......
@igorpan yep, I can understand how you feel. been there...
I don't know why, if it can't be fixed, adding a note on the doc is okay too. This could save us some time.
I've been using Laravel for three months now and have been very impressed with it so far, but running into this issue after writing only two small testcases for a small Model was an unpleasant (and unexpected) surprise. It took me a couple of hours to find out that it was not my own class, but the test framework itself causing some of my tests to fail. And to pass when ran individually... I came up with the following "solution" which I didn't see in this thread, so I thought I'd share it with you:
As an introduction: I'm using the models boot() method to implement record-level authorization: users may only update a model (in this case a Boat) if they own it. For example:
class Boat extends Eloquent {
protected static function boot()
{
parent::boot();
static::updating(
function ($boat) {
if (Authority::cannot('update', 'Boat', $boat)) {
return false;
}
}
);
}
Some test failed failed because the boot() method on my Model was only called once. The model events like 'update' were only registered with the event dispatcher of the first test, but not with the (new) event dispatchers of following tests because the Model class was already "$booted".
Since you can't "unload" a class in PHP, I decided to add an unboot() method to my model. By calling it from TestCase::tearDown() I tell the model it its no longer "$booted". It will then boot() again during the next test (whenever it instantiates a model). In short:
First, I created the following trait to implement two methods, unbootIfBooted and unboot(). Maybe a bit of overhead, but basically the counterparts of bootIfNotBooted and boot() from the Model. Note that unbootIfBooted() was made public static to be able to call it from a TestCase.
trait UnbootTrait {
public static function unbootIfBooted()
{
$class = get_called_class();
if (isset(static::$booted[$class])) {
static::$booted[$class] = null;
// fireModelEvent('unbooting', false);
static::unboot();
// fireModelEvent('unbooted', false);
}
}
protected static function unboot()
{
}
}
Then you can simply add this trait to the models you want to test, without the need to change its inheritance:
class Boat extends Eloquent {
use UnbootTrait;
And finally, tell the TestCase->teardown() method to "unboot" the models it used (usually only the model that's being tested):
class BoatTest extends TestCase {
protected function tearDown()
{
parent::tearDown();
Boat::unbootIfBooted();
}
public function testCanUpdateMyOwnBoat() {}
public function testCannotUpdateSomeoneElsesBoat() {}
It's just a quick fix, but it works. Comments and suggestions are welcome. I hope it'll be a contribution to a definitive solution to this problem.
Interesting solution, thanks) Although, I prefer not to think about the need unbooting models when writing Unit tests. Therefore, I prefer the solution based on EloquentEventsMechanic class.
Still, I'm looking forward to that this "feature" will fixed and everything will work out of the box)