Page MenuHomePhabricator
Create Task
Maniphest T176102

PasswordResetTest::testExecute_email errors out when CheckUser is installed
Closed, ResolvedPublic
Actions

Description

On a fresh installation of MediaWiki as of b347d8e where phpunit.php passed with no errors or failures, installing latest version of CheckUser as of 7a43255 and running the update script results in phpunit returning this error message:

1) PasswordResetTest::testExecute_email
Wikimedia\Rdbms\DBQueryError: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
Query: INSERT  INTO `unittest_cu_changes` (cuc_namespace,cuc_title,cuc_minor,cuc_user,cuc_user_text,cuc_actiontext,cuc_comment,cuc_this_oldid,cuc_last_oldid,cuc_type,cuc_timestamp,cuc_ip,cuc_ip_hex,cuc_xff,cuc_xff_hex,cuc_agent) VALUES ('2','','0',NULL,NULL,'reset password for user \"User1\"','','0','0','3','20170918004929','1.2.3.4','01020304','0',NULL,'0')
Function: CheckUserHooks::updateCUPasswordResetData
Error: 1048 Column 'cuc_user' cannot be null (localhost)


/var/www/html/includes/libs/rdbms/database/Database.php:1149
/var/www/html/includes/libs/rdbms/database/Database.php:979
/var/www/html/includes/libs/rdbms/database/Database.php:1589
/var/www/html/extensions/CheckUser/CheckUser.hooks.php:124
/var/www/html/includes/Hooks.php:186
/var/www/html/includes/user/PasswordReset.php:203
/var/www/html/tests/phpunit/includes/user/PasswordResetTest.php:183
/var/www/html/maintenance/doMaintenance.php:92

This is because this line of code passes null for the second user, which works in MediaWiki core but fails when CheckUser is installed.

Details

SubjectRepoBranchLines +/-
Unregister hooks that interfere with unit testingmediawiki/coremaster+1 -1
user: Unregister hooks that interfere with unit testingmediawiki/coremaster+7 -1
Handle password reset in unit tests gracefullymediawiki/extensions/CheckUsermaster+7 -0
Customize query in gerrit

Related Objects
Search...

Event Timeline

Huji created this task.Sep 18 2017, 1:10 AM
gerritbot subscribed.Sep 18 2017, 1:11 AM

Change 378652 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/core@master] [WIP] Do not merge

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

gerritbot added a project: Patch-For-Review.Sep 18 2017, 1:11 AM
Huji added a comment.Sep 18 2017, 1:28 AM

The issue with the above patch is that while it fixes the DB error, it results in the unit test not passing. The reason is that we do not pass the mock user to the PasswordReset::execute function; we pass the username, and on line 144 it tries to get the user from this username, but fails with a "nosuchuser" error.

I am not familiar enough with the unit testing feature to know how to fix this.

Huji added a subscriber: Tgr.EditedSep 18 2017, 7:08 PM
This comment has been deleted.
Tgr added a comment.Sep 18 2017, 7:17 PM

I imagine this was meant for the other bug? Here CheckUser chokes on the User::mailPasswordInternal hook, probably because those users don't actually exist in the database. I guess the test should either clear the relevant hooks or use real test users.

gerritbot added a comment.Sep 18 2017, 7:19 PM

Change 378652 abandoned by Huji:
[WIP] Do not merge

Reason:
You are right. I found a way to fix it in CheckUser

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

gerritbot added a comment.Sep 18 2017, 7:20 PM

Change 378772 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/CheckUser@master] Handle password reset in unit tests gracefully

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

gerritbot added a comment.Sep 18 2017, 7:32 PM

Change 378652 restored by Huji:
[WIP] Do not merge

Reason:
Restoring per Grego's comment

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

Krinkle moved this task from Inbox to PHPUnit on the MediaWiki-Core-Tests board.Sep 19 2017, 6:09 PM
gerritbot added a comment.Sep 25 2017, 4:45 PM

Change 378772 abandoned by Huji:
Handle password reset in unit tests gracefully

Reason:
Submitting a new patch on 378652 which would unregister the hook

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

Huji added a comment.Sep 25 2017, 4:51 PM

@Tgr now https://gerrit.wikimedia.org/r/#/c/378652/ is ready for final review. It does get the problem fixed.

gerritbot added a comment.Sep 27 2017, 11:02 PM

Change 378652 merged by jenkins-bot:
[mediawiki/core@master] user: Unregister hooks that interfere with unit testing

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

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2017-10-03 (1.31.0-wmf.2)).Sep 28 2017, 12:00 AM
Huji closed this task as Resolved.Sep 28 2017, 12:45 PM
Huji claimed this task.
Huji removed a project: Patch-For-Review.
Huji reopened this task as Open.Sep 28 2017, 3:12 PM
Huji added a subscriber: Krinkle.

@Krinkle okay, this patch doesn't get the job done. The unit test still ends with an error:

php tests/phpunit/phpunit.php tests/phpunit/includes/user/PasswordResetTest.php
Using PHP 7.0.22-0ubuntu0.16.04.1
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

...........E.

Time: 357 ms, Memory: 22.00MB

There was 1 error:

1) PasswordResetTest::testExecute_email
Wikimedia\Rdbms\DBQueryError: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
Query: INSERT  INTO `unittest_cu_changes` (cuc_namespace,cuc_title,cuc_minor,cuc_user,cuc_user_text,cuc_actiontext,cuc_comment,cuc_this_oldid,cuc_last_oldid,cuc_type,cuc_timestamp,cuc_ip,cuc_ip_hex,cuc_xff,cuc_xff_hex,cuc_agent) VALUES ('2','','0',NULL,NULL,'reset password for user \"User1\"','','0','0','3','20170928151127','1.2.3.4','01020304','0',NULL,'0')
Function: CheckUserHooks::updateCUPasswordResetData
Error: 1048 Column 'cuc_user' cannot be null (localhost)


/var/www/html/includes/libs/rdbms/database/Database.php:1166
/var/www/html/includes/libs/rdbms/database/Database.php:979
/var/www/html/includes/libs/rdbms/database/Database.php:1607
/var/www/html/extensions/CheckUser/CheckUser.hooks.php:124
/var/www/html/includes/Hooks.php:177
/var/www/html/includes/Hooks.php:205
/var/www/html/includes/user/PasswordReset.php:203
/var/www/html/tests/phpunit/includes/user/PasswordResetTest.php:189
/var/www/html/tests/phpunit/MediaWikiTestCase.php:416
/var/www/html/maintenance/doMaintenance.php:92
gerritbot added a comment.Sep 28 2017, 3:22 PM

Change 381231 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/core@master] Unregister hooks that interfere with unit testing

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

gerritbot added a project: Patch-For-Review.Sep 28 2017, 3:22 PM
gerritbot added a comment.Sep 28 2017, 4:01 PM

Change 381231 merged by jenkins-bot:
[mediawiki/core@master] Unregister hooks that interfere with unit testing

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

Huji closed this task as Resolved.Sep 28 2017, 5:10 PM
Huji removed a project: Patch-For-Review.
Krinkle added a comment.Sep 28 2017, 5:20 PM

Thanks!

Huji mentioned this in T176103: UserTest::testAutoblockCookieDisabled errors out when CheckUser is installed.Oct 9 2017, 1:19 AM
MarcoAurelio moved this task from General / Unsorted to Done on the CheckUser board.Nov 22 2017, 9:42 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL