Last modified: 2014-10-17 11:46:59 UTC
wfWarn() is called, among others, from Database::rollback where it warns that there is no transaction to rollback. Rollback is used already in the error situation; in this case wfWarn exits and this prevents subsequent printing of the real database error message that caused the database problem in the first place. With 4b291909e0e91ad4e8ed98030c1312a872ca3bd4 in place, SQL transactions on PostgreSQL fail with: TestORMRowTest::testSaveAndRemove with data set #0 (array('Foobar', '2012-01-01 02:02:02 G MT', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true) DatabasePostgres::reportQueryError: No transaction to rollback, something got out of sync! [Ca lled from DatabaseBase::rollback in /usr/home/saper/test/mytest/includes/db/Database.php at li ne 3482] /usr/home/saper/test/mytest/includes/debug/Debug.php:301 /usr/home/saper/test/mytest/includes/debug/Debug.php:157 /usr/home/saper/test/mytest/includes/GlobalFunctions.php:1110 /usr/home/saper/test/mytest/includes/db/Database.php:3482 /usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:510 /usr/home/saper/test/mytest/includes/db/Database.php:1077 /usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:871 /usr/home/saper/test/mytest/includes/db/ORMTable.php:1015 /usr/home/saper/test/mytest/includes/db/ORMRow.php:352 /usr/home/saper/test/mytest/tests/phpunit/includes/db/ORMRowTest.php:143 /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:119 while the real error should be: TestORMRowTest::testSaveAndRemove with data set #0 (array('Foobar', '2012-01-01 02:02:02 GM T', 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 afte r upgrading? See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script Query: INSERT INTO "orm_test" (test_id,test_name,test_age,test_height,test_awesome,test_stuff, test_moarstuff,test_time) 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;}','2012-01-01 02:02:02 GMT') Function: ORMTable::insertRow Error: 23502 ERROR: null value in column "test_id" violates not-null constraint /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:871 /usr/home/saper/test/mytest/includes/db/ORMTable.php:1015 /usr/home/saper/test/mytest/includes/db/ORMRow.php:352 /usr/home/saper/test/mytest/tests/phpunit/includes/db/ORMRowTest.php:143 /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:119 Warning about transaction not being open is completely irrelevant in this context. It causes some less useful error reports like bug 57724. Reverting 4b291909e0e91ad4e8ed98030c1312a872ca3bd4 restores proper error reporing and actually fixes ORMTableTest::testIgnoreErrorsOverride and some others on PostgreSQL.
Change 99648 had a related patch set uploaded by saper: wfWarn() should NOT cause unit tests to fail https://gerrit.wikimedia.org/r/99648
Seems that SpecialPageTest::testInvalidGetTitleFor and SpecialPageTest::testGetTitleFor seem to be affected by this in some way: (1) on one hand they depend on wfWarn() calling trigger_error and PHPUnit catches this (and the test is annotated with @expectedException PHPUnit_Framework_Error_Notice), so disabling $wgDevelopmentWarnings makes PHPUnit to complain that this exception does not occur, (2) on the other hand though further execution is stopped with trigger_error and $this->assertEquals( $expected, $title ); clauses in the test are never executed. I think we could easily have something like @expectWarningString in the PHPUnit test annotations, but the question is how to properly catch warnings in a way that they don't interrupt further execution of the tested code.
Few more ideas for a better implementation: 1) with $wgDevelopmentWarnings = true, allow the test code to continue to run after wfWarn 2) have a list of expected warnings (for tests like SpecialPageTest::testInvalidGetTitleFor mentioned above). We can have annotation for that. 3) collect all warnings in an array 4) check the list of warnings collected vs. list of expected warnings 5) if warnings are still there, fail the test (even if it otherwise passes).
[Patch needs rework]
Change 99648 abandoned by Siebrand: wfWarn() should NOT cause unit tests to fail Reason: Per last bug report comment. Patch needs rework. Can be restored if it's based on this one. https://gerrit.wikimedia.org/r/99648
Change 136287 had a related patch set uploaded by Jjanes: PostgreSQL: Only rollback when in a transaction https://gerrit.wikimedia.org/r/136287
Change 136287 merged by jenkins-bot: PostgreSQL: Only rollback when in a transaction https://gerrit.wikimedia.org/r/136287
All patches mentioned in this report are either merged or abandoned - is there more work left to do here (if yes: please reset the bug report status to NEW or ASSIGNED), or can you close this ticket as RESOLVED FIXED?
No reply to comment 8. All patches mentioned in this report were merged or abandoned - assuming this bug is FIXED. If that is not the case: Please reopen and elaborate what is left to do here to get this report fixed.
