Page MenuHomePhabricator

trigger_error in wfWarn prevents to disclose database error in unit tests
Closed, ResolvedPublic

Description

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.


Version: 1.23.0
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=57724

Details

Reference
bz58095

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:25 AM
bzimport set Reference to bz58095.
bzimport added a subscriber: Unknown Object (MLST).

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
  1. have a list of expected warnings (for tests like SpecialPageTest::testInvalidGetTitleFor mentioned above). We can have annotation for that.
  1. collect all warnings in an array
  1. check the list of warnings collected vs. list of expected warnings
  1. if warnings are still there, fail the test (even if it otherwise passes).

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.