Skip to content

Commit

Permalink
adding default order on id to CakeTestModel, to ensure correct order.…
Browse files Browse the repository at this point in the history
… 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 05940ae
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 0 deletions.
1 change: 1 addition & 0 deletions lib/Cake/Test/Case/Model/ModelReadTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6082,6 +6082,7 @@ public function testBuildQuery() {
$this->loadFixtures('User');
$TestModel = new User();
$TestModel->cacheQueries = false;
$TestModel->order = null;

$expected = array(
'conditions' => array(
Expand Down
17 changes: 17 additions & 0 deletions lib/Cake/TestSuite/Fixture/CakeTestModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@renan renan commented on 05940ae Nov 24, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@AD7six AD7six commented on 05940ae Nov 24, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@renan renan commented on 05940ae Nov 24, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Running those individually works.

@AD7six
Copy link
Member

@AD7six AD7six commented on 05940ae Nov 24, 2011 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ceeram
Copy link
Contributor Author

@ceeram ceeram commented on 05940ae Nov 24, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.