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.6] Allow closure to determine if event should be faked #24887

Merged
merged 3 commits into from
Jul 22, 2018

Conversation

arjanwestdorp
Copy link
Contributor

This PR adds a way to dynamically determine if an event should be faked or not. Currently we can do this only based on an event name. By also allowing closures we can with one simple closure include or exclude multiple events at once.

// Ignore all 'eloquent.*' events so observers will still work ...
\Event::fake(function($event) {
    return !starts_with($event, 'eloquent.');
})

It's even possible to make them more specific and inspect the payload to determine if we should fake the event or dispatch it as normal:

\Event::fake(function($event, $payload) {
    return $event === 'Foo' && $payload->user_id != 1;
})

It's still possible to use only event names or a combination of closures and event names:

\Event::fake([
    function($event, $payload) {
        return $event === 'Foo' && $payload->user_id != 1;
    },
    function($event) {
        return !starts_with($event, 'eloquent.');
    },
    EventOne::class,
    EventTwo::class,
])

@taylorotwell taylorotwell merged commit 33db136 into laravel:5.6 Jul 22, 2018
@aliselcuk
Copy link

this merge broke some of my tests...
I might already be using EventFake incorrectly but replacing isEmpty with isNotEmpty in shoudFakeEvent method fixed my tests.

@aliselcuk
Copy link

aliselcuk commented Jul 27, 2018

@arjanwestdorp your assertions seem not right

public function testAssertDispatchedWithIgnore()
    {
        $dispatcher = m::mock(Dispatcher::class);
        $dispatcher->shouldReceive('dispatch')->twice();

        $fake = new EventFake($dispatcher, [
            'Foo',
            function ($event, $payload) {
                return $event === 'Bar' && $payload['id'] === 1;
            },
        ]);

        $fake->dispatch('Foo');
        $fake->dispatch('Bar', ['id' => 1]);
        $fake->dispatch('Baz');

        $fake->assertNotDispatched('Foo');
        $fake->assertNotDispatched('Bar');
        $fake->assertDispatched('Baz');
    }

You are listing Foo as an event to be faked.
So when you dispatch it with the fake dispatcher you should expect it is caught by EventFake and test with assertDispatched not assertNotDispatched.

Your test passes but it is not valid which leads to breaking the actual implementation.

Here's a test that should pass, but it is failing with the current merge:

    public function testAssertDispatchedAndFaked()
    {
        $dispatcher = m::mock(Dispatcher::class);
        $dispatcher->shouldReceive('dispatch')->once();

        $fake = new EventFake($dispatcher, ['FakeEvent']);

        $fake->dispatch('FakeEvent');
        $fake->dispatch('RealEvent');

        $fake->assertDispatched('FakeEvent');
        $fake->assertNotDispatched('RealEvent');
    }

https://github.com/arjanwestdorp/framework/blob/33db136da97d2111790e591f0c32497e3ce11e51/src/Illuminate/Support/Testing/Fakes/EventFake.php#L238

isEmpty() should be replaced by isNotEmpty()

@arjanwestdorp
Copy link
Contributor Author

@aliselcuk You are completely right! You were using the fake as it was intended, but i've inverted the logic by mistake 😳. I'm really sorry for breaking your tests!

I've submitted a PR #24985 to fix the behaviour.

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.

None yet

3 participants