UnqueryableQueryMock::all() returns Exception but 0 count is expected

[fcaldarelli]

https://github.com/yiisoft/active-record/blob/bed8118208835fd4bf6081e6e24f9c36f449b7cd/tests/unit/ActiveDataProviderTest.php#L193

Here we expect that count is 0. But UnqueryableQueryMock::all() returns an Exception mandatory.

https://github.com/FabrizioCaldarelli/db/blob/b1bb19a4bafbe5a84aa62388cd3f0e81b5db809f/tests/unit/UnqueryableQueryMock.php#L28

[fcaldarelli]

I found the problem, it is about how prepareModels() has changed.

In current ActiveDataProvider version, prepareModels() calls prepareQuery(). When there are not records, $query->emulateExecution(); is called and anyway the methods returns $query object.

https://github.com/yiisoft/active-record/blob/bed8118208835fd4bf6081e6e24f9c36f449b7cd/src/data/ActiveDataProvider.php#L122

In previous implementation, prepareModels() returned an empty array where there are not records:

https://github.com/yiisoft/active-record/blob/e93f97cd0cee1f23e217d4d32b2a88f8fab52b3a/src/data/ActiveDataProvider.php#L111

This is from last commit, where @GHopperMSK wanted to explicit $query content to simplify debug activities.

https://github.com/yiisoft/active-record/commit/bed8118208835fd4bf6081e6e24f9c36f449b7cd

We can solve checking emulateExecution property that it has been used to track this case, from last commit:

    protected function prepareModels()
    {
        $query = $this->prepareQuery();
        return ($query->emulateExecution) ? [] : $query->all($this->db);
    }

[np25071984]

I don't think it is a good idea to return an empty array. Now we return $query object and it is quite convenient in all cases. If the query is empty, it sets emulate mode witch protects the DB from unnecessary requests. If the query isn't empty, it works as usual. We don't need to check the results every time, we just work with obtained object.

Maybe we should just adapt the test to new conditions?

[fcaldarelli]

I don't think it is a good idea to return an empty array. Now we return $query object and it is

In previous implementation, prepareModels() did the same work that you now do with two methods, so it made sense that it returned an empty array, in that condition.

[np25071984]

In previous implementation, prepareModels() did the same work that you now do with two methods

If so, then how test $this->assertCount(0, $provider->getModels()); passed successfully?

[fcaldarelli]

I have explained in previous post, because prepareModels() returned an empty array:

https://github.com/yiisoft/active-record/blob/e93f97cd0cee1f23e217d4d32b2a88f8fab52b3a/src/data/ActiveDataProvider.php#L111

and test succeded.

[rob006]

This should be fixed in UnqueryableQueryMock - it should not throw exception if emulateExecution is enabled.

[fcaldarelli]

In this case it is not about emulateExecution , because it has been added from last commit.

UnqueryableQueryMock throw an exception independently from emulateExecution.

[rob006]

UnqueryableQueryMock throw an exception independently from emulateExecution.

Yes, and that is the problem.

[fcaldarelli]

Yes, and that is the problem.

Why? In previous release it worked fine, because as class name says it has to test query not queryable.

[rob006]

The purpose of this mock is to ensure that no actual query will be performed. With emulateExecution there will be not actual query, there should be no exception in this case.

[fcaldarelli]

Take a look at differences from last commit (not working) and previous (working).

[rob006]

@FabrizioCaldarelli I don't get your point. These changes are correct, the tests should be fixed to not throw exception when there is no need for it. It was already done in https://github.com/yiisoft/yii2/pull/17073

[fcaldarelli]

I showed how keep current tests and current changes together.

[rob006]

Why would you want add additional conditions to (correct) code instead of fixing (incorrect) tests? It is actually a bug that UnqueryableQueryMock throws exception for query with emulateExecution, it should be changed anyway.