Page MenuHomePhabricator

CirrusSearch\Api\CheckSanityTest::testProblems is failing on CheckUser builds
Closed, ResolvedPublic

Description

A CirrusSearch test is failing on a php 8.3 CheckUser build. Specifically https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php83-noselenium/23673/consoleFull

Extract of the failing test:

15:18:22 There were 2 failures:
15:18:22 
15:18:22 1) CirrusSearch\Api\CheckSanityTest::testProblems with data set #0 ('redirectInIndex', Closure Object (...), array(1, 0, 'wikidb_content', array(42, 1, 'wikidb_general')))
15:18:22 Failed asserting that two strings are equal.
15:18:22 --- Expected
15:18:22 +++ Actual
15:18:22 @@ @@
15:18:22 -'wikidb_content'
15:18:22 +'wikidb-unittest__content'
15:18:22 
15:18:22 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/Api/CheckSanityTest.php:145
Logs generated by test
15:18:22 
15:18:22 2) CirrusSearch\Api\CheckSanityTest::testProblems with data set #4 ('pageInWrongIndex', Closure Object (...), array('wikidb_general', 'wikidb_content'))
15:18:22 Failed asserting that two strings are equal.
15:18:22 --- Expected
15:18:22 +++ Actual
15:18:22 @@ @@
15:18:22 -'wikidb_general'
15:18:22 +'wikidb-unittest__general'
15:18:22 
15:18:22 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/Api/CheckSanityTest.php:145
Logs generated by test
15:18:22 === Logs generated by test case
15:18:22 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"Wikimedia\\ObjectCache\\HashBagOStuff"}
15:18:22 [localisation] [debug] LocalisationCache using store LCStoreNull []
15:18:22 [objectcache] [debug] MainObjectStash using store {class} {"class":"Wikimedia\\ObjectCache\\HashBagOStuff"}
15:18:22 [wfDebug] [debug] MediaWiki\Parser\ParserFactory: using default preprocessor {"private":false}
15:18:22 [wfDebug] [debug] MediaWiki\Context\ContextSource::getContext (MediaWiki\Api\ApiModuleManager): called and $context is null. Using RequestContext::getMain() {"private":false}
15:18:22 ===
15:18:22 
15:18:22 FAILURES!
15:18:22 Tests: 2921, Assertions: 13537, Failures: 2, Skipped: 5.

Event Timeline

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

Having looked more closely at the relevant failing test, it seems that the data provider calls WikiMap::getCurrentWikiId at the point where it is called. Because PHPUnit constructs the data providers before the tests are executed, it seems possible that WikiMap::getCurrentWikiId is called before the unit test DB can be set as the wiki ID.

That may cause issues because the return value might change by the time the test is executed.

It might be fixable by calling WikiMap::getCurrentWikiId inside a callback so that it is called when the test is actually being executed.

Change #1144590 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CirrusSearch@master] Obtain WikiMap::getCurrentWikiId in callback when in data provider

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

Change #1144590 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] Obtain WikiMap::getCurrentWikiId in callback when in data provider

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

To give more context, this issue may have been caused by CheckUser modifying the DB prefix in CheckUser change 1137000 where we saw the failing build. The side effect of this then being that the prefix was different when the CirrusSearch test was run.

However, I think the change that was merged is still a good one given that data providers could be called outside a context where things like the wiki ID is ready for inspection.

It appears the rest of the issues might be related to CheckUser, so marking this tentatively as resolved.

Change #1145244 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CirrusSearch@master] [WIP] Call Connection::clearPool before running tests

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

I've found that the route cause of the problem is SearchConfig caching the wrong value of wikiId that seems to be caused for no particular reason.

I found that just running CheckUserGetIPsPager even with no code in any test method causes the CirrusSearch test to fail. It seems to be likely the instance being created somewhere in test setup where the wiki ID is not properly prefixed.

I'm going to upload another fix that should address the route cause. The fix forces the wiki ID to be reset on the local SearchConfig instance before each test. It kind of seems a brute force way of fixing it, but I'm not sure I'm able to find a better way to fix it that wouldn't still follow the "reset the wiki ID / SearchConfig instance between each test".

Change #1145255 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CirrusSearch@master] Reset the wikiId in SearchConfig instance between tests

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

Change #1145244 abandoned by Dreamy Jazz:

[mediawiki/extensions/CirrusSearch@master] [WIP] Call Connection::clearPool before running tests

Reason:

Doesn't appear to fix the issue at hand.

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

Marking this as Done again, as the fix is +2'd

Change #1145255 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] Reset the wikiId in SearchConfig instance between tests

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