Skip to content

Loading…

Fix #90 PropelOnDemandFormatter formatter broken when using single table inheritance #177

Closed
wants to merge 1 commit into from

3 participants

@shane-l

No description provided.

@fzaninotto
Propel member

The correct unit test for your change should be in PropelFormatterTest. The test you added is a functional test - which is good - but we need a unit test. Apart from that, why isn't $class enough for the key to the worker object hash?

I can't seem to remember why I used $col as the key in the first place. It doesn't make much sense and is prone to error.

@willdurand
Propel member

heya @shane-I you've changed file permissions, can you change that ? Files have to be 644

@shane-l

hey, have fixed the file permissions and squashed commits.

@willdurand
Propel member

There is a problem with your PR:

There was 1 failure:

1) ModelCriteriaTest::testFindPksSimpleKey
Failed asserting that 'findPks() returns a PropelCollection' matches expected true.

/Users/william/projects/Propel/Propel/test/testsuite/runtime/query/ModelCriteriaTest.php:1562
@shane-l

Works for me?

phpunit testsuite/runtime/

PHPUnit 3.5.15 by Sebastian Bergmann.

...............................................................  63 / 858 (  7%)
............................................................... 126 / 858 ( 14%)
............................................................... 189 / 858 ( 22%)
............................................................... 252 / 858 ( 29%)
..............................................................S 315 / 858 ( 36%)
SSSSSSSSSSSSSS................................................. 378 / 858 ( 44%)
............................................................... 441 / 858 ( 51%)
...SS.......................................................... 504 / 858 ( 58%)
............................................................... 567 / 858 ( 66%)
............................................................... 630 / 858 ( 73%)
..................................SSSSSS....................... 693 / 858 ( 80%)
............................................S...SSSSS.......... 756 / 858 ( 88%)
............................................................... 819 / 858 ( 95%)
.......................................

Time: 10 seconds, Memory: 61.75Mb

OK, but incomplete or skipped tests!
Tests: 858, Assertions: 2083, Skipped: 29.
@willdurand
Propel member

You have to run all unit tests :) the problem is in the runtime.

@shane-l

on master:

Time: 38 seconds, Memory: 244.25Mb

OK, but incomplete or skipped tests!
Tests: 2257, Assertions: 5429, Skipped: 14.

on fix-on-demand-formatter

Time: 38 seconds, Memory: 244.50Mb

OK, but incomplete or skipped tests!
Tests: 2260, Assertions: 5438, Skipped: 14.

markTestSkipped() is being used so I don't see how I can get "Skipped" to be zero.

And again, running the test that you say is failing:

phpunit testsuite/runtime/query/ModelCriteriaTest.php
PHPUnit 3.5.15 by Sebastian Bergmann.

...............................................................  63 / 163 ( 38%)
............................................................... 126 / 163 ( 77%)
.....................................

Time: 1 second, Memory: 28.00Mb

OK (163 tests, 413 assertions)
@willdurand
Propel member

Hum, weird..

@willdurand
Propel member

I can confirm this bug again

@willdurand
Propel member

Any news?

@willdurand willdurand closed this
@willdurand willdurand reopened this
@shane-l

Unfortunately, no. I can't make the tests fail. Ideally you or someone else could spend some time investigating. If not you can probably close this request.

@fzaninotto
Propel member

The tests are all green for me:

$ git checkout master
$ git remote add shane-l https://github.com/shane-l/Propel.git
$ git checkout -b fix_formatter_test
$ git fetch shane-l
$ git merge shane-l/fix-on-demand-formatter
$ phpunit testsuite/runtime/query/ModelCriteriaTest.php
PHPUnit 3.5.14 by Sebastian Bergmann.

...............................................................  63 / 166 ( 37%)
............................................................... 126 / 166 ( 75%)
........................................

Time: 1 second, Memory: 28.00Mb

OK (166 tests, 421 assertions)
@willdurand
Propel member

I get the following output:

Time: 44 seconds, Memory: 295.00Mb

There was 1 failure:

1) ModelCriteriaTest::testFindPksSimpleKey
Failed asserting that 'findPks() returns a PropelCollection' matches expected true.

/Users/william/projects/Propel/Propel/test/testsuite/runtime/query/ModelCriteriaTest.php:1562

FAILURES!
Tests: 2260, Assertions: 5437, Failures: 1, Skipped: 13.
@fzaninotto
Propel member
@willdurand
Propel member

Hum, ok so if I rebase the PR, I don't get any error..

@willdurand willdurand added a commit that referenced this pull request
@willdurand willdurand Merged PR #177.
Fixes #90
6984ed0
@willdurand willdurand closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 29, 2011
  1. @shane-l
This page is out of date. Refresh to see the latest.
View
10 runtime/lib/formatter/PropelFormatter.php
@@ -169,12 +169,14 @@ protected function isWithOneToMany()
*/
protected function getWorkerObject($col, $class)
{
- if(isset($this->currentObjects[$col])) {
- $this->currentObjects[$col]->clear();
+ $key = $col . '_' . $class;
+
+ if (isset($this->currentObjects[$key])) {
+ $this->currentObjects[$key]->clear();
} else {
- $this->currentObjects[$col] = new $class();
+ $this->currentObjects[$key] = new $class();
}
- return $this->currentObjects[$col];
+ return $this->currentObjects[$key];
}
/**
View
65 test/testsuite/runtime/formatter/PropelFormatterTest.php
@@ -0,0 +1,65 @@
+<?php
+
+/**
+ * This file is part of the Propel package.
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ *
+ * @license MIT License
+ */
+
+require_once dirname(__FILE__) . '/../../../tools/helpers/bookstore/BookstoreEmptyTestBase.php';
+
+/**
+ * Test class for PropelObjectFormatter.
+ *
+ * @author Francois Zaninotto
+ * @version $Id$
+ * @package runtime.formatter
+ */
+class PropelFormatterTest extends BookstoreEmptyTestBase
+{
+ protected function setUp()
+ {
+ parent::setUp();
+ BookstoreDataPopulator::populate();
+ }
+
+ public function testGetWorkerObjectReturnsRightClass()
+ {
+ $formatter = $this->getMockForAbstractClass('PropelFormatter');
+
+ $method = new ReflectionMethod('PropelFormatter', 'getWorkerObject');
+ $method->setAccessible(true);
+
+ $classNames = array(
+ 'Bookstore',
+ 'BookReader',
+ 'BookClubList',
+ );
+
+ $col = 0;
+ foreach ($classNames as $className) {
+ // getWorkerObject() should always return an instance of the requested class, regardless of the value of $col
+ $result = $method->invoke($formatter, $col, $className);
+
+ $this->assertEquals($className, get_class($result), 'getWorkerObject did not return an instance of the requested class');
+ }
+ }
+
+ public function testGetWorkerObjectCachedInstance()
+ {
+ $formatter = $this->getMockForAbstractClass('PropelFormatter');
+
+ $method = new ReflectionMethod('PropelFormatter', 'getWorkerObject');
+ $method->setAccessible(true);
+
+ $className = 'Bookstore';
+ $col = 0;
+
+ $result1 = $method->invoke($formatter, $col, $className);
+ $result2 = $method->invoke($formatter, $col, $className);
+
+ $this->assertEquals(spl_object_hash($result1), spl_object_hash($result2), 'getWorkerObject should return a cached instance of a class at the same col index');
+ }
+}
View
21 test/testsuite/runtime/formatter/PropelOnDemandFormatterTest.php
@@ -148,4 +148,25 @@ public function testFormatOneManyResults()
$this->assertTrue($book instanceof Book, 'PropelOnDemandFormatter::formatOne() returns a model object');
}
+ public function testFormatSingleTableInheritanceManyResults()
+ {
+ $con = Propel::getConnection(BookPeer::DATABASE_NAME);
+ BookstoreDataPopulator::populate($con);
+
+ $stmt = $con->query('SELECT * FROM bookstore_employee');
+ $formatter = new PropelOnDemandFormatter();
+ $formatter->init(new ModelCriteria('bookstore', 'BookstoreEmployee'));
+
+ $employees = $formatter->format($stmt);
+
+ foreach ($employees as $employee) {
+ $row = array();
+ $row[1] = $employee->getClassKey();
+
+ $omClass = BookstoreEmployeePeer::getOMClass($row, 0, false);
+ $actualClass = get_class($employee);
+
+ $this->assertEquals($omClass, $actualClass, 'PropelOnDemandFormatter::format() should handle single table inheritance');
+ }
+ }
}
View
5 test/tools/helpers/bookstore/BookstoreDataPopulator.php
@@ -167,6 +167,11 @@ public static function populate($con = null)
$bemp2->setSupervisor($bemp1);
$bemp2->save($con);
+ $bemp3 = new BookstoreCashier();
+ $bemp3->setName("Tim");
+ $bemp3->setJobTitle("Cashier");
+ $bemp3->save($con);
+
$role = new AcctAccessRole();
$role->setName("Admin");
Something went wrong with that request. Please try again.