Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix ModifyLimitQueryTest #295

Merged
merged 2 commits into from Apr 1, 2013

Conversation

Projects
None yet
4 participants
Member

deeky666 commented Mar 30, 2013

The functional ModifyLimitQueryTest makes false expectations about the query results. LIMIT queries without ORDER BY clause return non-deterministic results, therefore the test cannot make any expectations about which values are returned and in which order. You can only expect a certain number of results.
The test might still work on platforms like mysql when you assume results to be in an ascending order, because the database might put the results of a non-deterministic query in an ascending order by default. Still this is not reliable and not all vendors do that.
This PR changes the tests to have an ordered, deterministic results by default to have more reliable expectations. Also one test about a non-deterministic query was added, which just asserts the number of results returned.

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DBAL-478

@stof stof and 1 other commented on an outdated diff Mar 30, 2013

...ctrine/Tests/DBAL/Functional/ModifyLimitQueryTest.php
@@ -109,6 +110,14 @@ public function assertLimitResult($expectedResults, $sql, $limit, $offset)
$row = array_change_key_case($row, CASE_LOWER);
$data[] = $row['test_int'];
}
- $this->assertEquals($expectedResults, $data);
+
+ /**
+ * Do not assert the order of results when results are non-deterministic
+ */
+ if ($deterministic) {
+ $this->assertEquals($expectedResults, $data);
+ } else {
+ $this->assertEquals(count($expectedResults), count($data));
@stof

stof Mar 30, 2013

Member

This should use assertCount IMO

@deeky666

deeky666 Mar 30, 2013

Member

Ah you're right!

beberlei added a commit that referenced this pull request Apr 1, 2013

@beberlei beberlei merged commit ecdb069 into doctrine:master Apr 1, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment