Page MenuHomePhabricator

UserTest::testAutoblockCookieDisabled errors out when CheckUser is installed
Closed, ResolvedPublic

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:

UserTest::testAutoblockCookiesDisabled
MWException: Block::delete() requires that the mId member be filled


/var/www/html/includes/Block.php:459
/var/www/html/tests/phpunit/includes/user/UserTest.php:697
/var/www/html/tests/phpunit/MediaWikiTestCase.php:416
/var/www/html/maintenance/doMaintenance.php:92

Not sure why it only happens after CheckUser is installed, and disappears if I remove CheckUser from LocalSettings.php.

While investigating it a bit more, I came to the understanding that $block->getId() returns 0 for the block created in that function.

Notifying @MusikAnimal, @Tgr, @Krinkle and @aaron who are among the latest contributors to this part of code.

Event Timeline

Huji created this task.Sep 18 2017, 1:58 AM
Tgr added a comment.Sep 18 2017, 5:00 AM

$block->insert() fails and the test doesn't notice, I suppose? Either it shouldn't store the block at all (doesn't seem to have much relevance for that test) or it should assert that it succeeded.

Would be good to know why it fails, of course.

Huji added a comment.Sep 18 2017, 5:25 PM
This comment was removed by Huji.
Huji added a comment.Sep 18 2017, 6:03 PM

@Tgr, in fact, all blocks on that page are failing. As a first step, I am submitting a patch that checks whether those block insertions succeeded or not. What advice do you have about finding out why insert() fails?

Change 378752 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/core@master] Assert that blocks were inserted successfully in UserTest

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

Tgr added a comment.Sep 18 2017, 7:07 PM

Remove the IGNORE in Block::insert and you'll get a proper DB error instead of silent failure.

Huji added a comment.EditedSep 18 2017, 7:16 PM

@Tgr the problem seems to be related to the fact that we are creating blocks but not specifying a performing user for the block (only specifying the target). CheckUser would want to log that action, but without a performing user it cannot, and the block fails.

In https://gerrit.wikimedia.org/r/378764 I created a proof of concept patch which demonstrates this. When the performing user is specified, that error goes away. But now the tests fail for other reasons. In this PoC patch, I focused on UserTest::testAutoblockCookies() and when you use this patch, it errors out on line 652, i.e. 'Autoblock still works' is not asserted.

1) UserTest::testAutoblockCookies
Autoblock still works
Failed asserting that false matches expected true.

/var/www/html/tests/phpunit/includes/user/UserTest.php:652
/var/www/html/tests/phpunit/MediaWikiTestCase.php:416
/var/www/html/maintenance/doMaintenance.php:92

On the other hand, you could argue that this is CheckUser's fault and should be fixed there. A possible solution is to ensure in CheckUser hooks that if the performing user is blank or non-existent, it gracefully let's the block go through but throws an exception or writes to the debug log.

PS: The problem is only manifested when CheckUser is enabled, due to the fact that doRetroactiveAutoblock is run in CheckUser.hooks.php.

Krinkle moved this task from Inbox to PHPUnit on the MediaWiki-Core-Testing board.Sep 19 2017, 6:09 PM
Krinkle removed a subscriber: Krinkle.Sep 19 2017, 6:47 PM
Huji claimed this task.Sep 28 2017, 3:05 PM

Change 378752 merged by jenkins-bot:
[mediawiki/core@master] Assert that blocks were inserted successfully in UserTest

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

Okay, that was only the smaller part of the problem. The main problem still exists. @Tgr any recommendations on next steps?

Tgr added a comment.Oct 8 2017, 4:39 AM

I thought the main problem was solved by https://gerrit.wikimedia.org/r/#/c/381231/ ?

Huji added a comment.Oct 9 2017, 1:19 AM

@Tgr: No, that was for T176102 not for this one (...102, ...103, close enough to get confused I'm sure).

Tgr added a comment.Oct 9 2017, 1:27 AM

I mean, it can be solved the same way, by disabling hooks. (Maybe the code or the test is doing something wrong though, and that would just hide the error?)

Huji added a comment.Oct 9 2017, 1:34 AM

Is that a generally good approach though? There is something fishy going on here, and all we do is basically hide it?

Tgr added a comment.Oct 9 2017, 4:16 AM

Probably not :) The Block class doesn't make clear which parameters are required but from reading the code, it doesn't really make sense to create a Block with no blocking user. So the tests should probably be updated to contain a setBlocker call.

Change 378764 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/core@master] Specify a blocker for all UserTest blocks

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

Huji added a comment.Oct 9 2017, 2:16 PM

@Tgr see this patch. It is basically a clean up of the PoC patch I mentioned before. It adds the setBlocker call for all of those tests. But now one of the tests fails; I have added a custom error message for it 'Autoblock does not work' which is shown. The full output of runing phpunit can be found in P6094

Change 378764 merged by jenkins-bot:
[mediawiki/core@master] Specify a blocker for all UserTest blocks

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

Huji closed this task as Resolved.Nov 9 2017, 2:32 AM
Huji removed a project: Patch-For-Review.
MarcoAurelio moved this task from Backlog to Closed on the CheckUser board.Nov 22 2017, 9:42 AM