Page MenuHomePhabricator

Tests introduced in tests/phpunit/includes/db/TestORMRowTest.php fail on PostgreSQL
Closed, ResolvedPublic

Description

maintenance/update.php script was run, but the tests fail nethertheless on PostgreSQL:

$ sh -x /usr/home/saper/bin/runphpunit.sh /usr/home/saper/public_html/pg/w/tests/phpunit/includes/db/TestORMRowTest.php
+ php -c /usr/home/saper/php.ini tests/phpunit/phpunit.php --configuration tests/phpunit/suite.xml --exclude-group Broken,Stub,Dump,ParserFuzz --log-junit /usr/home/saper/tests/log/postgres-log.xml /usr/home/saper/public_html/pg/w/tests/phpunit/includes/db/TestORMRowTest.php
PHPUnit 3.6.10 by Sebastian Bergmann.

Configuration read from /usr/home/saper/public_html/pg/w/tests/phpunit/suite.xml

EFF

Time: 1 second, Memory: 31.25Mb

There was 1 error:

1) TestORMRowTest::testConstructor with data set #0 (array('Foobar', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
DBQueryError: A database error has occurred.  Did you forget to run maintenance/update.php after upgrading?  See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script
Query: CREATE TABLE IF NOT EXISTS "unittest_orm_test"(
				test_id                    INT unsigned        NOT NULL auto_increment PRIMARY KEY,
				test_name                  VARCHAR(255)        NOT NULL,
				test_age                   TINYINT unsigned    NOT NULL,
				test_height                FLOAT               NOT NULL,
				test_awesome               TINYINT unsigned    NOT NULL,
				test_stuff                 BLOB                NOT NULL,
				test_moarstuff             BLOB                NOT NULL,
				test_time                  varbinary(14)       NOT NULL
			);
Function: DatabaseBase::safeQuery
Error: 42601 ERROR:  syntax error at or near "unsigned"
LINE 2:     test_id                    INT unsigned        NOT NULL ...
                                           ^



/usr/home/saper/public_html/pg/w/includes/db/Database.php:953
/usr/home/saper/public_html/pg/w/includes/db/DatabasePostgres.php:394
/usr/home/saper/public_html/pg/w/includes/db/Database.php:920
/usr/home/saper/public_html/pg/w/includes/db/Database.php:1006
/usr/home/saper/public_html/pg/w/includes/db/Database.php:1031
/usr/home/saper/public_html/pg/w/tests/phpunit/includes/db/TestORMRowTest.php:82
/usr/home/saper/public_html/pg/w/tests/phpunit/MediaWikiTestCase.php:75
/usr/home/saper/public_html/pg/w/tests/phpunit/MediaWikiPHPUnitCommand.php:45
/usr/home/saper/public_html/pg/w/tests/phpunit/phpunit.php:103

--


There were 2 failures:

1) TestORMRowTest::testSave with data set #0 (array('Foobar', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
Failed asserting that false is true.

/usr/home/saper/public_html/pg/w/tests/phpunit/includes/db/ORMRowTest.php:102
/usr/home/saper/public_html/pg/w/tests/phpunit/MediaWikiTestCase.php:75
/usr/home/saper/public_html/pg/w/tests/phpunit/MediaWikiPHPUnitCommand.php:45
/usr/home/saper/public_html/pg/w/tests/phpunit/phpunit.php:103

2) TestORMRowTest::testRemove with data set #0 (array('Foobar', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
Failed asserting that false is true.

/usr/home/saper/public_html/pg/w/tests/phpunit/includes/db/ORMRowTest.php:117
/usr/home/saper/public_html/pg/w/tests/phpunit/MediaWikiTestCase.php:75
/usr/home/saper/public_html/pg/w/tests/phpunit/MediaWikiPHPUnitCommand.php:45
/usr/home/saper/public_html/pg/w/tests/phpunit/phpunit.php:103

FAILURES!
Tests: 3, Assertions: 6, Failures: 2, Errors: 1.
+ exit

Version: 1.20.x
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=43475 T45475

Details

Reference
bz37601

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 12:24 AM
bzimport set Reference to bz37601.
bzimport added a subscriber: Unknown Object (MLST).
saper created this task.Jun 14 2012, 5:48 PM
aaron added a comment.Jun 14 2012, 6:04 PM

Nothing should be using safeQuery() like that anyway.

scfc added a comment.Sep 19 2012, 9:54 PM

Created attachment 11128
Patch for table creation in test case.

The attached patch solves the table creation bug (although the BLOB data type needs more thought), but the actual test then fails with:

There was 1 error:
1) TestORMRowTest::testSave with data set #0 (array('Foobar', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
DBQueryError: A database error has occurred. Did you forget to run maintenance/update.php after upgrading? See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script
Query: INSERT INTO "unittest_orm_test" (test_id,test_name,test_age,test_height,test_awesome,test_stuff,test_moarstuff) VALUES (NULL,'Foobar','42','9000.1',1,'a:6:{i:0;i:13;i:1;i:11;i:2;i:7;i:3;i:5;i:4;i:3;i:5;i:2;}','O:8:"stdClass":3:{s:3:"foo";s:3:"bar";s:3:"bar";a:2:{i:0;i:4;i:1;i:2;}s:3:"baz";b:1;}')
Function: ORMRow::insert
Error: 23502 FEHLER: NULL-Wert in Spalte »test_id« verletzt Not-Null-Constraint

This is due to explicitly passing NULL as the value for the ID column which in MySQL means "use next value", but in PostgreSQL means "use NULL". It can be worked around by passing the keyword "DEFAULT" (without quotes) as the value, e. g. "INSERT INTO t (c) VALUES (DEFAULT);".

Jeroen, what do you think is the best approach to support PostgreSQL here? From a glance at the code, I would probably exclude ID columns in ORMRow::getWriteValues() when their value is null.

Attached:

Marcin: thanks for pointing this out

Aaron: the ORMTable/Row code is not using safeQuery. I can in fact not find the method, so am guessing it got killed?

Tim Landscheidt: thanks for the patch. Will also try to look at the blob issue soonish.

scfc added a comment.Sep 25 2012, 1:31 PM

(In reply to comment #3)

[...]
Tim Landscheidt: thanks for the patch. Will also try to look at the blob issue
soonish.

Just to clarify: There isn't necessarily an "issue" with PostgreSQL's blob data type, it just has to be reviewed that BYTEA is an appropriate data type in this case as well as whether for example test_age's TINYINT unsigned can be translated to SMALLINT or should be translated to "SMALLINT CHECK(test_age >= 0)". As this is "just" a test case with the emphasis on the ORM part, it's probably not that important, but I wanted to have pointed it out.

scfc added a comment.Dec 28 2012, 1:39 AM
  • Bug 38930 has been marked as a duplicate of this bug. ***

Change 100180 had a related patch set uploaded by saper:
Database independent test for bug 37601

https://gerrit.wikimedia.org/r/100180

saper added a comment.EditedDec 7 2013, 9:09 PM

Reverting state to NEW:

Gerrit change Iebe9d6ff73fac930b456b24b6317cafb56d0163f is a test case to expose the failure in the database-independent way.

It seems to me that` ORMRow::getWriteValues()` had a test preventing "id"
fields and values from being ever written to the database (rightly so).

ece97c358424015777db3881d87aa33966d634bd has moved this responsibility from the row-level to the table level, but the test has been removed, therefore this bug.

I am not sure what is the proper place to fix this - to reintroduce the following lines from ORMRow into ORMTable

227  // Skip null id fields so that the DBMS can set the default.
228  if ( $name === 'id' && is_null( $value ) ) {
229        continue;
230  }

The test introduced in Iebe9d6ff73fac930b456b24b6317cafb56d0163f would be more elegant if there were some way to test getWriteValues() directly; maybe it even belongs to ORMTableTest instead but I could not figure out how to inject required IORMRow dependency into that test and lost too much energy trying.

Change 100183 had a related patch set uploaded by saper:
Teach ORMTable to use sequences

https://gerrit.wikimedia.org/r/100183

saper added a comment.EditedDec 7 2013, 10:09 PM

This is a very very early attempt to do something about primary keys.

With gerrit change 100183 PostgreSQL and MySQL sucessfully pass tests/phpunit/includes/db/TestORMRowTest.phptests/phpunit/includes/db/TestORMRowTest.php in isolation, but fail when a whole test suite is run:

10) TestORMRowTest::testSaveAndRemove with data set #0 (array('Foobar', '2012-01-01 02:02:02 GMT', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
DBQueryError: A database error has occurred. Did you forget to run maintenance/update.php after upgrading?  See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script
Query: SELECT nextval('orm_test_test_id_seq')
Function: DatabaseBase::query
Error: 42P01 ERROR:  relation "orm_test_test_id_seq" does not exist
LINE 1: ...ELECT /* DatabaseBase::query 127.0.0.1 */ nextval('orm_test_...
                                                             ^



/usr/home/saper/test/mytest/includes/db/Database.php:1111
/usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:511
/usr/home/saper/test/mytest/includes/db/Database.php:1077
/usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:991
/usr/home/saper/test/mytest/includes/db/ORMTable.php:1025
/usr/home/saper/test/mytest/includes/db/ORMRow.php:352
/usr/home/saper/test/mytest/tests/phpunit/includes/db/ORMRowTest.php:165
/usr/home/saper/test/mytest/tests/phpunit/MediaWikiTestCase.php:123
/usr/home/saper/test/mytest/tests/phpunit/MediaWikiPHPUnitCommand.php:80
/usr/home/saper/test/mytest/tests/phpunit/MediaWikiPHPUnitCommand.php:64
/usr/home/saper/test/mytest/tests/phpunit/phpunit.php:115

I am kind of at loss to figure out whether we are using mocked table and row instance but still somehow touch the database anyway; should we also mock sequences??? This is getting crazy.

Btw, shouldn't those tests be moved into @Database groups since the CREATE TABLEs and stuff?

saper added a comment.EditedDec 7 2013, 10:29 PM

I tend to believe that this patch fixes T45475 somehow, but tests are still broken.

Change 172662 had a related patch set uploaded by saper:
Mark TestORMRowTest as broken

https://gerrit.wikimedia.org/r/172662

saper updated the task description. (Show Details)Nov 27 2014, 12:17 PM
saper set Security to None.
saper updated the task description. (Show Details)
Krinkle removed subscribers: Unknown Object (MLST), Krinkle.Dec 21 2014, 7:44 AM

Change 172662 had a related patch set uploaded (by saper):
Mark TestORMRowTest as broken

https://gerrit.wikimedia.org/r/172662

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 18 2015, 1:11 PM

Change 172662 abandoned by Legoktm:
Mark TestORMRowTest as broken

Reason:
This code is no longer in core, yaaaay!

https://gerrit.wikimedia.org/r/172662

Legoktm closed this task as Resolved.Dec 14 2015, 1:19 AM
Legoktm claimed this task.
Legoktm added a subscriber: Legoktm.

Resolved by removing all of the ORM related code out of core.

saper added a comment.Dec 14 2015, 6:42 AM

This is good news! Thank you @Legoktm!

Change 100180 abandoned by saper:
Database independent test for bug 37601

Reason:
This code has been removed from core

https://gerrit.wikimedia.org/r/100180

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Migrating from the old tracking task to a tag for PostgreSQL-related tasks.