Page MenuHomePhabricator

Allow tests to reset/clear non-legacy hook handlers
Closed, DuplicatePublic

Description

CirrusSearch's MappingConfigBuilderTest compares a computed mapping config with a known good value stored in a JSON file. To ensure that other extensions don’t interfere with that config, it resets several hooks:

$this->mergeMwGlobalArrayValue( 'wgHooks', [
	'CirrusSearchMappingConfig' => [],
	'GetContentModels' => [],
	'SearchIndexFields' => [],
] );

In I0209821b3e, I’m trying to migrate Wikibase’s handler for the SearchIndexFields hook to the new hook container system. However, that makes that test fail:

12:02:06 1) CirrusSearch\Maintenance\MappingConfigBuilderTest::testBuild with data set "empty" ('mapping/empty.expected', array(), 'CirrusSearch\Maintenance\Mapp...uilder')
12:02:06 Failed asserting that two strings are equal.
12:02:06 --- Expected
12:02:06 +++ Actual
12:02:06 @@ @@
12:02:06                      "all"\n
12:02:06                  ]\n
12:02:06              },\n
12:02:06 +            "wikibase_item": {\n
12:02:06 +                "type": "text",\n
12:02:06 +                "analyzer": "keyword",\n
12:02:06 +                "norms": false,\n
12:02:06 +                "index_options": "docs"\n
12:02:06 +            },\n
12:02:06              "all": {\n
12:02:06                  "type": "text",\n
12:02:06                  "analyzer": "text",\n
12:02:06 
12:02:06 /workspace/src/tests/phpunit/MediaWikiTestCaseTrait.php:129
12:02:06 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/Maintenance/MappingConfigBuilderTest.php:61
12:02:06 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:438
12:02:06 /workspace/src/maintenance/doMaintenance.php:107

The reason for this seems to be that the mergeMwGlobalArrayValue() call doesn’t reset non-legacy hook handlers. I tried to fix this in CirrusSearch, but the only solution I found is this:

$scopedCallback = ExtensionRegistry::getInstance()->setAttributeForTest( 'Hooks', [] );

But that feels very internal (I just found it by going through HookContainer::getHandlers() and the methods it calls), and also clears all non-legacy hook handlers, not just the three that the test is meant to clear. I think there should be some better way to clear hook handlers, legacy and non-legacy together, ideally directly in MediaWikiIntegrationTestCase.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 609763 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/CirrusSearch@master] Also clear non-legacy hooks for test

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

Change 609763 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Also clear non-legacy hooks for test

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

CBogen moved this task from Incoming to Needs Reporting on the Discovery-Search (Current work) board.
CBogen added a subscriber: CBogen.

What's needed before this can be closed? Thanks!

Well, the task isn’t done yet… the attached Gerrit change is just a workaround. (But the real work would be in core, not CirrusSearch.)

Gehel added a subscriber: Gehel.

Removing Discovery-Search from this ticket since there isn't any additional work on our side (at least, that's my understanding, feel free to disagree and send this back our way!).

The fix for T255056 should address this as well.