Last modified: 2014-10-17 11:46:59 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T60095, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 58095 - trigger_error in wfWarn prevents to disclose database error in unit tests
trigger_error in wfWarn prevents to disclose database error in unit tests
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Logging (Other open bugs)
1.23.0
All All
: Normal normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: easy
Depends on:
Blocks: 57922
  Show dependency treegraph
 
Reported: 2013-12-06 14:22 UTC by Marcin Cieślak
Modified: 2014-10-17 11:46 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Marcin Cieślak 2013-12-06 14:22: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.
Comment 1 Gerrit Notification Bot 2013-12-06 14:27:41 UTC
Change 99648 had a related patch set uploaded by saper:
wfWarn() should NOT cause unit tests to fail

https://gerrit.wikimedia.org/r/99648
Comment 2 Marcin Cieślak 2013-12-06 16:58:35 UTC
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.
Comment 3 Marcin Cieślak 2013-12-07 12:11:03 UTC
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).
Comment 4 Andre Klapper 2014-01-23 14:55:21 UTC
[Patch needs rework]
Comment 5 Gerrit Notification Bot 2014-03-28 23:19:51 UTC
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
Comment 6 Gerrit Notification Bot 2014-05-30 06:55:07 UTC
Change 136287 had a related patch set uploaded by Jjanes:
PostgreSQL: Only rollback when in a transaction

https://gerrit.wikimedia.org/r/136287
Comment 7 Gerrit Notification Bot 2014-05-30 16:27:47 UTC
Change 136287 merged by jenkins-bot:
PostgreSQL: Only rollback when in a transaction

https://gerrit.wikimedia.org/r/136287
Comment 8 Andre Klapper 2014-08-17 11:27:26 UTC
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?
Comment 9 Andre Klapper 2014-10-17 11:46:59 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links