[5.2] Add chunkById query builder tests #12906
Conversation
| @@ -330,8 +330,8 @@ public function value($column) | |||
| /** | |||
| * Chunk the results of the query. | |||
| * | |||
| * @param int $count | |||
| * @param callable $callback | |||
| * @param int $count | |||
There was a problem hiding this comment.
You'll get hit on the fingers for these! 😱
The formats are @param__type__$variable (2/2 spaces) and @return_type (1 space).
Always the same number of spaces, no vertical alignment.
|
Ha... whoops. I started that way, but then realized that other methods in the file seemed to vertically align, so i changed it. Changed it back now 😄 Thanks for clarifying. |
|
Some phpdocs don't follow this convention simply because they haven't been updated. It's not supported by StyleCI ;) |
|
IMO, other part of phpdocs fix should go to new PR |
| @@ -332,7 +332,7 @@ public function value($column) | |||
| * | |||
| * @param int $count | |||
| * @param callable $callback | |||
| * @return bool | |||
| * @return bool | |||
There was a problem hiding this comment.
I wrote @return_type, one space!
(also you may want to stash the commits to not clutter git history)
There was a problem hiding this comment.
don't worry about phpdoc cs - as long a it's valid phpdoc, taylor or I can clean it up after merge
There was a problem hiding this comment.
Please do squash this to one commit though.
GrahamCampbell
changed the title
|
More important, because of the mocking, these tests failed to detect that the results contain only the id column, as I also noted in #12861 (comment). |
|
No thoughts concerning the "only id column in results" issue? Seems pretty important to me. |
|
@vlakoff yeah, good point on the id, totally missed that. Easy to fix -- will get it done today. Re: duplication between query builder and eloquent builder -- yes, the chunk method is also duplicated. It seems it would be relatively easy to remove that duplication if it's not there for a specific reason. If i have time tonight, i'll look into it. |
|
@kevindoole Any news on this "select only id" stuff? As a reminder, this goes unnoticed because tests mock |
|
Yeah, I've been dragging my heels with this! I'll get it done tonight; looks like probably a change to one line, unless I figure out a nicer way to test it. |
|
@vlakoff Looks like someone beat me to it 👍 |
Yep: #13137 |
This just adds a test for query builder chunkById, and fixes up some crappy docblocks from my last pr.
There were no existing tests for the query builder chunk method -- possibly because the method is essentially duped from the eloquent builder file. Seems nice to have both sides tested.
Happy to add a couple more tests if they bring any value.