Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Add support for testing eloquent model events #14374

Merged
merged 3 commits into from
Jul 26, 2016

Conversation

thecrypticace
Copy link
Contributor

Take 2. I've added a separate method for mocking model events so it shouldn't completely break code using observers (sorry!).

Notes:

  • Observers are explicitly setup to not fire. This is tested for.
  • I copied the eloquent setup from the integration tests. Perhaps this code could be moved to a trait? Though, a little duplication here isn't a terribly big deal.
  • I'm not 100% pleased with how I had to write the test but I couldn't think of another way to do it. Specifically I extended Illuminate\Foundation\Testing\TestCase to treat it as an integration test to be sure it worked within the context of using that class.

@taylorotwell
Copy link
Member

Can you give a quick example of what was broken before this PR so I can do a quick before / after test?

@Jono20201
Copy link
Contributor

@taylorotwell:

Please see:

Essentially $this->expectsEvents('eloquent.saving: App\Model'); doesn't work because the Mocked dispatcher doesn't get set statically on the Model. Additional code is needed to "add" mocked methods onto the mocked dispatcher.

@taylorotwell
Copy link
Member

I think it would be a better API if I could say:

->expectsModelEvents(Foo::class, 'saved') and let that format that to the proper event string the user doesn't have to worry about the somewhat custom event name format used by Eloquent.

@thecrypticace
Copy link
Contributor Author

Oh, I like that idea! I'll update the PR in the next hour or two.

@thecrypticace
Copy link
Contributor Author

Updated. How does that look?

@taylorotwell taylorotwell merged commit 698d908 into laravel:5.3 Jul 26, 2016
@taylorotwell
Copy link
Member

One thing I did change was I made withoutEvents() call withoutModelEvents(). I think it makes sense that if you call withoutEvents you would expect NO events to fire, even model events. If you disagree let me know. Thanks! 😄

@thecrypticace
Copy link
Contributor Author

That will break observers for people using expectsEvents. That's why I kept them as separate methods. See @GrahamCampbell's comments in the previous PR: #12904

@taylorotwell
Copy link
Member

Hmm, it's a little hard for me to follow what Graham means there. Do you have a simple example you could give me of how it would break things?

@thecrypticace
Copy link
Contributor Author

Yeah, I'll write one up later today.

@thecrypticace
Copy link
Contributor Author

Any testing code that is mocking events but requires observers to fire would break (see the test case below).

This could be one of the behavioral changes for 5.3 if you feel that the change in functionality is intended. If it isn't an intended change, I already have a branch ready to open a PR with the test case to ensure this does not regress in the future.

/** @test */
public function observers_do_fire_when_mocking_normal_events()
{
    $this->withoutEvents();

    EloquentTestModel::observe(EloquentTestModelSucceedingObserver::class);

    EloquentTestModel::create(['field' => 1])->delete();

    $this->assertEquals(4, EloquentTestModelSucceedingObserver::$eventsFired);

    EloquentTestModelSucceedingObserver::reset();
}
class EloquentTestModelSucceedingObserver {
    public static $eventsFired = 0;
    public function saved() { static::$eventsFired += 1; }
    public function saving() { static::$eventsFired += 1; }
    public function deleted() { static::$eventsFired += 1; }
    public function deleting() { static::$eventsFired += 1; }
    public static function reset() { static::$eventsFired = 0; }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants