PHPORM-45 Add Query\Builder::toMql() to simplify comprehensive query tests #6
Conversation
tests/QueryBuilderTest.php
Outdated
| { | ||
| /** | ||
| * Builder::aggregate() and Builder::count() cannot be tested because they return the result, | ||
| * without modifying the builder. |
There was a problem hiding this comment.
Same problem as Laravel Eloquent: https://laravel.io/forum/12-29-2016-eloquent-hell-tosql-doesnt-show-aggregation
tests/QueryBuilderTest.php
Outdated
| ]; | ||
|
|
||
| yield [ | ||
| ['aggregate' => [[['$group' => ['_id' => ['foo' => '$foo'], 'foo' => ['$last' => '$foo']]]], []]], |
There was a problem hiding this comment.
This translation of the "group" operation will be investigated. #4 (comment)
https://jira.mongodb.org/browse/PHPORM-48
| use MongoDB\BSON\UTCDateTime; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class BuilderTest extends TestCase |
There was a problem hiding this comment.
This new test class will be reserved to unit-tests on generated queries.
The QueryBuilderTest class have a tearDown method that slow down the tests and make it depend on a server.
There was a problem hiding this comment.
In PHPLIB we have a few of those instances, and we usually name the database-dependent test SomethingFunctionalTest to indicate the difference to FunctionalTest. We may want to apply the same pattern here to avoid confusion between QueryBuilderTest and BuilderTest.
| ]; | ||
|
|
||
| return new Collection($results); | ||
| return ['count' => [$wheres, []]]; |
There was a problem hiding this comment.
Related to #4 (comment)
I came back to "count" for now. Otherwise there is a test failure.
1) Jenssegers\Mongodb\Tests\TransactionTest::testQuery
Failed asserting that 0 matches expected 1.
tests/TransactionTest.php:302
src/Query/Builder.php
Outdated
| if ($this->columns === null) { | ||
| $this->columns = $columns; | ||
| if ($this->columns !== null) { | ||
| $columns = $this->columns; |
There was a problem hiding this comment.
I'm not entirely sure about this change as it may have side effects. Looking at the usages of $this->columns, it is used in various other methods of this class and in the Laravel framework itself. What was the reason for not storing the columns? Was it to be able to run toMql repeatedly without modifying the underlying object?
There was a problem hiding this comment.
I removed the $columns argument from toMql to stick to the current behavior. My goal is that toMql method does not alter the query.
…tests (#6) * PHPORM-45 Add Query\Builder::toMql() to simplify comprehensive query tests * Move Query/Builder unit tests to a dedicated test class
Fix PHPORM-45
Extracted from #4