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 project: Discovery-Search. · View Herald TranscriptJul 6 2020, 10:52 AM
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!).

daniel added a subscriber: daniel.Sep 1 2020, 8:44 PM

The fix for T255056 should address this as well.