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.8] Bugfix - QueueFake size method and added a test #29761

Merged
merged 7 commits into from Sep 2, 2019

Conversation

@hlorofos
Copy link

commented Aug 27, 2019

This is fixing #29755

Oleg Khimich added 4 commits Aug 27, 2019
Oleg Khimich
Oleg Khimich
Oleg Khimich
tests/Queue/QueueSizeTest.php Outdated Show resolved Hide resolved
Oleg Khimich
tests/Queue/QueueSizeTest.php Outdated Show resolved Hide resolved

@hlorofos hlorofos marked this pull request as ready for review Aug 27, 2019

Oleg Khimich
@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

This will break if the queue is null?

@hlorofos

This comment has been minimized.

Copy link
Author

commented Aug 27, 2019

This will break if the queue is null?

@GrahamCampbell thats why I put first assertion in the test without pushing anything to a queue:

Queue::fake();
        $this->assertEquals(0, Queue::size());

there is initializer for $job in QueueFake

    protected $jobs = [];
@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

No, I mean if there are jobs, but the queue name is null, it should be non-zero.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

To clarify, in Laravel, using null for the queue name means "use the default". This needs to be handled within the implementation.

@hlorofos

This comment has been minimized.

Copy link
Author

commented Aug 27, 2019

To clarify, in Laravel, using null for the queue name means "use the default". This needs to be handled within the implementation.

@GrahamCampbell , here are my explanations in the test, please consider if possible to explain what's wrong based on the test:

        dispatch($job); // Pushed to default queue
        dispatch(new TestJob2); //  Other job pushed to default queue, this would validate we traverse through different job classes properly
        dispatch($job)->onQueue('Q2'); // Pushed to queue 'Q2'

        $this->assertEquals(2, Queue::size()); // Get the amount of jobs in default queue, internally its NULL for QueueFake
        $this->assertEquals(1, Queue::size('Q2')); // Get the amount of jobs in queue `Q2`

If you're talking about default property for Queue implementations this have nothing to do with QueueFake implementation.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

I think you can already assert on Queue::pushedJobs() when using the fake. I'm not sure having size always return the number of pushed jobs makes sense.

@hlorofos

This comment has been minimized.

Copy link
Author

commented Aug 30, 2019

@taylorotwell you interpret this wrong, I'm testing my code which expose Queue::size through API. Since I'm faking queue and testing API which calling size() - as a developer I expect size method return proper value instead of some random number.

@taylorotwell taylorotwell reopened this Sep 2, 2019

@taylorotwell taylorotwell merged commit b27be4f into laravel:5.8 Sep 2, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@driesvints

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

@hlorofos we got this merged 👍

@hlorofos

This comment has been minimized.

Copy link
Author

commented Sep 3, 2019

@hlorofos we got this merged 👍

@driesvints appreciate all your help along the way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.