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

[8.x] Added new assertDispatchedSync methods to BusFake #37350

Merged
merged 1 commit into from
May 12, 2021
Merged

[8.x] Added new assertDispatchedSync methods to BusFake #37350

merged 1 commit into from
May 12, 2021

Conversation

frankieeedeee
Copy link
Contributor

@frankieeedeee frankieeedeee commented May 12, 2021

Added new assertDispatchedSync methods to the Bus Fake, following the pattern of assertDispatched and assertDispatchedAfterResponse.

This allows applications to assert that a given command was explicitly sent to the sync driver. This is useful where applications may want to conditionally handle a job synchronously in the current request/thread, rather than sending it to the application's default queue driver; And of course, assert that such logic executed correctly:

use App\Jobs\ProcessAvatar;

public function test_it_dispatches_later()
{
    // ... set up

    Bus::assertDispatched(ProcessAvatar::class);
    Bus::assertNotDispatchedSync(ProcessAvatar::class);
}

public function test_it_dispatches_immediately()
{
    // ... set up

    Bus::assertDispatchedSync(ProcessAvatar::class);
}

Since this PR includes quite a few new lines (222 additions), it's understandable if this is too much change for a minor update. If that's the case, please feel free to let me know and I'll suggest over on the master branch for 9.x.

Cheers!

Additions to the Public API of BusFake

These methods follow the existing patterns of assertDispatched and assertDispatchedAfterResponse:

  • + assertDispatchedSync($command, $callback = null)
  • + assertDispatchedSyncTimes($command, $times = 1)
  • + assertNotDispatchedSync($command, $callback = null)
  • + dispatchedSync(string $command, $callback = null)
  • + hasDispatchedSync($command)

6 additional tests have been added to Illuminate\Tests\Support\SupportTestingBusFakeTest to test these above new methods.

No Breaking Changes

Existing applications that dispatch jobs using dispatchSync and test that such jobs were dispatched using assertDispatched, assertDispatchedTimes or assertNotDispatched will not be affected by this change. This is because these assertion methods have been updated in this PR to check against the dispatchedSync method.

@ahinkle
Copy link
Contributor

ahinkle commented May 12, 2021

Nice PR but seems like a low edge case usage for Laravel to maintain. Might be better suited as a community package?

@frankieeedeee
Copy link
Contributor Author

frankieeedeee commented May 12, 2021

Hey @ahinkle, Thanks!

You might be right, however the BusFake already includes assertions against jobs being dispatched after the response is sent (assertDispatchedAfterResponse, assertDispatchedAfterResponseTimes etc), so it feels like synchronous assertion is missing functionality from Bus Fake..?

To me, I would think most applications would make a choice to decide whether to dispatch a job after the response is sent, or not; I don't think there's a lot of use case for conditionally dispatching a job before/after. On the other hand, I think there's quite a few use cases for conditionally pushing a job to the queue for later processing, or handling the job immediately.

Given that, it would make more sense to have the 'synchronous' assertions, over the 'after response' assertions. That's why the existence of the 'after response' assertions makes it feel like (to me!) that Bus Fake was missing that last bit of testing functionality.

Just my 2¢! :)

@frankieeedeee
Copy link
Contributor Author

Also, given that the dispatch() and dispatchSync() methods exist on the Dispatcher contract, I think it's important for applications' integration testing to not only be able to assert that a job was dispatched, but importantly how the job was dispatched. Seems to me like that might have been the original reason to include the assertDispatchedAfterResponse methods in the first place :)

@taylorotwell taylorotwell merged commit bbfa760 into laravel:8.x May 12, 2021
@frankieeedeee frankieeedeee deleted the assert-dispatched-sync branch May 12, 2021 23:16
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