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

[9.x] Improve EventFake::assertListening() support for subscribers #42193

Merged
merged 15 commits into from
May 2, 2022

Conversation

simonhamp
Copy link
Contributor

@simonhamp simonhamp commented Apr 29, 2022

EventFake::assertListening() currently works well for 'standard' event Listeners and Subscribers that us array syntax to listen to events or 'auto-subscribers' that contain a single 'handle' method to route all events through.

Testing that a listener has been registered where the subscriber listens to events using the '@-callback' string syntax convention currently fails.

However, the underlying Dispatcher supports the subscriber defining its own method name using the '@-callback' approach and will route events correctly.

For those who use subscribers with '@-callback' syntax, having tests that can assert the appropriate listener methods have been registered by the subscriber, assertListening needs to change a little.

This is only additive to the assertListening method allowing it to work more completely with the range of mechanisms supported by the framework for registering event listeners.

It doesn't create any regressions for existing test suite assertions.

This allows more folks to write more robust tests around Events without having to change their code just to make their tests pass.

@GrahamCampbell GrahamCampbell changed the title Improve EventFake::assertListening() support for subscribers [9.x] Improve EventFake::assertListening() support for subscribers Apr 30, 2022
Should fix prior test (but doesn't break functionality - only highlights that the other test is perhaps overly strict?)
@simonhamp
Copy link
Contributor Author

I'm just looking through this again and seeing that there are integration tests in tests/Integration/Events/EventFakeTest.php that aim to prove what I'm doing here....

So my test changes are probably wrong (and redundant)

@taylorotwell taylorotwell merged commit 19e98d6 into laravel:9.x May 2, 2022
@simonhamp simonhamp deleted the feature/improve-eventfake branch May 2, 2022 14:42
@crynobone
Copy link
Member

This PR broke observer-based events on Eloquent such as below:

/**
 * @test
 */
public function it_can_assert_fake_event_on_global_observer()
{
    $user = UserFactory::new()->create([
        'name' => 'Taylor Otwell',
    ]);

    User::observe(new UserObserver());

    Event::fake();

    $user->name = 'David Hemphill';
    $user->save();

    Event::assertListening('eloquent.updated: '.User::class, UserObserver::class.'@updated');
}

Screenshot 2022-05-05 at 10 39 12 PM

@taylorotwell
Copy link
Member

Thoughts @simonhamp?

@simonhamp
Copy link
Contributor Author

Interesting. The tests all passed prior and with this change, so it looks like something peculiar to observers isn't well covered here.

I'll take a look shortly. Sorry if this has caused you trouble @crynobone!

@simonhamp
Copy link
Contributor Author

@crynobone can I just check that you're importing the right class in your test file?

i.e. are you really expecting the class Laravel\Nova\Tests\Fixtures\UserObserver to be registered as the listener for this event?

Perhaps you need a use App\Observers\UserObserver statement at the top of your test?

@crynobone
Copy link
Member

@simonhamp what different does that make when it used to work before this PR?

@simonhamp simonhamp restored the feature/improve-eventfake branch May 6, 2022 18:38
@simonhamp
Copy link
Contributor Author

@crynobone

@simonhamp what different does that make when it used to work before this PR?

I'm just making sure the test code sample you've supplied is actually sound. If this is an actual test you've got in your app that was working before, I can completely understand the concern.

This PR broke observer-based events on Eloquent

To be clear, there's nothing in this PR that touches observer-based events, so that's technically not possible. What it most likely is is just a failing test due to this type of assertion not being well accounted for in the framework's battery of tests for the EventFake.

If you change your assertion from the string UserObserver::class.'@updated' to the array [UserObserver::class, 'updated'], does that fix your test?

@simonhamp
Copy link
Contributor Author

@crynobone I've managed to test my theory and have narrowed it down to just that... the string-based assertion is failing because of the EventFake logic changes in this PR.

Sorry about that.

I've fixed it in #42289, but in the interim you can change your test to use an array assertion instead (per my previous message) and your test shouldn't fail because of this supposedly-missing listener.

Appreciate that you may have more than one test to fix though, so I'm not suggesting you go through your entire test suite just to change some string assertions to array assertions. I'd recommend waiting for #42289 to be merged and released instead, as that will make your existing assertions work again.

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