Permalink
Browse files

adding default order on id to CakeTestModel, to ensure correct order.…

… Postgres can return results in any order it considers appropriate if none specified
  • Loading branch information...
ceeram committed Nov 22, 2011
1 parent f57c287 commit 05940ae1ec703a23714ff815e4e2e19cd1c6b5b7
Showing with 18 additions and 0 deletions.
  1. +1 −0 lib/Cake/Test/Case/Model/ModelReadTest.php
  2. +17 −0 lib/Cake/TestSuite/Fixture/CakeTestModel.php
@@ -6082,6 +6082,7 @@ public function testBuildQuery() {
$this->loadFixtures('User');
$TestModel = new User();
$TestModel->cacheQueries = false;
+ $TestModel->order = null;
$expected = array(
'conditions' => array(
@@ -27,4 +27,21 @@
class CakeTestModel extends Model {
public $useDbConfig = 'test';
public $cacheSources = false;
+
+/**
+ * Constructor, sets default order for the model to avoid failing tests caused by
+ * incorrect order when no order has been defined in the finds.
+ * Postgres can return the results in any order it considers appropriate if none is specified
+ *
+ * @param mixed $id Set this ID for this model on startup, can also be an array of options, see Model::__construct().
+ * @param string $table Name of database table to use.
+ * @param string $ds DataSource connection name.
+ */
+ function __construct($id = false, $table = null, $ds = null) {
+ parent::__construct($id, $table, $ds);
+ if ($this->useTable && $this->primaryKey && $this->schema($this->primaryKey)) {
+ $this->order = array($this->alias . '.' . $this->primaryKey => 'ASC');
+ }
+ }
+
}

6 comments on commit 05940ae

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Nov 24, 2011

Owner

This commit broke the build. Removing the constructor fixes the failing tests, but I'm sure you had a reason for the change. http://ci.cakephp.org:8080/job/CakePHP%202.0%20-%20MySQL/413/ Is where the build started failing.

Owner

markstory replied Nov 24, 2011

This commit broke the build. Removing the constructor fixes the failing tests, but I'm sure you had a reason for the change. http://ci.cakephp.org:8080/job/CakePHP%202.0%20-%20MySQL/413/ Is where the build started failing.

@renan

This comment has been minimized.

Show comment Hide comment
@renan

renan Nov 24, 2011

Member

This was supposed to fix the ordering problems we get from time to time and makes few tests fail. I proposed changing it to a beforeFind or even override the find method.

Member

renan replied Nov 24, 2011

This was supposed to fix the ordering problems we get from time to time and makes few tests fail. I proposed changing it to a beforeFind or even override the find method.

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Nov 24, 2011

Member

Are either of you getting fails yourselves with those 3 tests with 2.0 ? I don't, also somewhat hard to see how this change could cause the errors reported - they are all missing table errors.

EDIT: I'm all for this functional change (however implemented) - the tests are riddled with assumptions that dbs return results in id ascending order if no order is specified, and that means they simply break if you are not using mysql (and sometimes even if you are).

Member

AD7six replied Nov 24, 2011

Are either of you getting fails yourselves with those 3 tests with 2.0 ? I don't, also somewhat hard to see how this change could cause the errors reported - they are all missing table errors.

EDIT: I'm all for this functional change (however implemented) - the tests are riddled with assumptions that dbs return results in id ascending order if no order is specified, and that means they simply break if you are not using mysql (and sometimes even if you are).

@renan

This comment has been minimized.

Show comment Hide comment
@renan

renan Nov 24, 2011

Member

I got two fails every time I run AllTests: FixtureTaskTest.testImportRecordsNoEscaping and FormHelperTest.testCheckboxDisabling.

Running those individually works.

Member

renan replied Nov 24, 2011

I got two fails every time I run AllTests: FixtureTaskTest.testImportRecordsNoEscaping and FormHelperTest.testCheckboxDisabling.

Running those individually works.

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Nov 24, 2011

Member
Member

AD7six replied Nov 24, 2011

@ceeram

This comment has been minimized.

Show comment Hide comment
@ceeram

ceeram Nov 24, 2011

Member

Sorry, guys. Yes individually tests pass. I am working on a fix which includes moving this form constructor to beforeFind, to ensure correct ordering. Sometimes tests fail because the find results are incorrectly ordered. I have setup postgres now, so i can ensure the fix wont break postgres either. I wasnt able to test with postgres before. I was suprised by the fact, that mysql and sqlite accept ORDER BY, where postgres doesnt: example query:
SELECT MAX("AfterTree"."rght") AS "rght" FROM "after_trees" AS "AfterTree" WHERE 1 = 1 AND "AfterTree"."id" <> 8 ORDER BY "AfterTree"."id" ASC LIMIT 1

I will work on it tonight to get the test passing again, and include the fix for correct ordering

Member

ceeram replied Nov 24, 2011

Sorry, guys. Yes individually tests pass. I am working on a fix which includes moving this form constructor to beforeFind, to ensure correct ordering. Sometimes tests fail because the find results are incorrectly ordered. I have setup postgres now, so i can ensure the fix wont break postgres either. I wasnt able to test with postgres before. I was suprised by the fact, that mysql and sqlite accept ORDER BY, where postgres doesnt: example query:
SELECT MAX("AfterTree"."rght") AS "rght" FROM "after_trees" AS "AfterTree" WHERE 1 = 1 AND "AfterTree"."id" <> 8 ORDER BY "AfterTree"."id" ASC LIMIT 1

I will work on it tonight to get the test passing again, and include the fix for correct ordering

Please sign in to comment.