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

Add assert with params convenience methods #286

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

patrickomeara
Copy link
Contributor

Problem

When I am testing that actions are pushed correctly, I almost always want to check the parameters that it is pushed with.

Currently it is a little cumbersome to check the params, as they are passed in as an array.

SendPodcastPublishedEmail::dispatch($user, $podcast);

// currently...
SendPodcastPublishedEmail::assertPushed(fn ($a, $params) => $params[0]->is($user) && $params[1]->is($podcast));

Additionally, the thing I want to check is in the second argument so I have an unused variable.

Solution

If we spread the params into the callback we can use type safety to check we have the correct type and shortcut the param check.

SendPodcastPublishedEmail::dispatch($user, $podcast);

// assert by closure
SendPodcastPublishedEmail::assertPushedWithParams(fn (User $u, Podcast $p) => $user->is($u) && $podcast->is($p));

Or we can assert by an array

// assert by array
SendPodcastPublishedEmail::assertPushedWithParams([$user, $podcast]);

Or my favourite way if the action only has a single param.

SendWelcomeEmail::dispatch($user);

// assert by first class callable
SendWelcomeEmail::assertPushedWithParams($user->is(...));

This PR also adds the ability to assert the queue using assertPushedWithParamsOn(), and assert that an action wasn't pushed with params using assertNotPushedWithParams()

I later found a test helper assertJobPushedWith() that could be replaced.

Note: Naming the methods assertPushedWith(), assertNotPushedWith() also works well, but sounds a little strange when checking the queue as well assertPushedWithOn() I'm easy either way.

@lorisleiva
Copy link
Owner

Hiya! 👋

Thanks for the great PR!

I'm really liking this and I love the fact that this kills a test helper. It's usually a good sign that this is needed for the API. However, I'm having a hard time digesting the name assertPushedWithParamsOn haha.

Also, I'm not sure if this is now the convention in Laravel, but in testing frameworks, I'm more used to a single With suffix instead of WithParams. I think the following API could be a bit more intuitive.

  • Rename assertPushedWithParams to assertPushedWith. E.g. SendPodcastPublishedEmail::assertPushedWith([$user, $podcast]); looks very clean to me.
  • Same with assertNotPushedWith.
  • Kill assertPushedWithParamsOn. We already have assertPushedWith and assertPushedOn so why not just make people do two assertions in order to compose them instead of trying to create helpers that do it all. See example below.
SendPodcastPublishedEmail::assertPushedWith([$user, $podcast]);
SendPodcastPublishedEmail::assertPushedOn("someQueue");

What do you think?

@patrickomeara
Copy link
Contributor Author

I think you're right about the naming, I'll update that and clean it up. Let me have a think about the queue, because having it in two different assertions could assert two different jobs have been pushed.

* the queue can be asserted using the second argument of assertPushedWith
@patrickomeara
Copy link
Contributor Author

patrickomeara commented Aug 20, 2024

I've cleaned up the naming, and it feels much better.

The queue can be asserted using:

SendPodcastPublishedEmail::assertPushedWith([$user, $podcast], 'someQueue');

@lorisleiva lorisleiva merged commit 5bff2b2 into lorisleiva:main Aug 20, 2024
8 checks passed
@lorisleiva
Copy link
Owner

Perfect, thank you for this! 🍺

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.

2 participants