RevisionFormatterTest.php fails when run in a suite with PermissionsTest.php
Open, Needs TriagePublic
Actions

Description

Problem

The RevisionFormatterTest class fails if run in a test suite with PermissionsTest before it. 2 tests fail with errors. Running the classes individually causes no problems.

Steps to reproduce
In a Mediawiki checkout with the Flow extension installed.

Copy phpunit.dist.xml to phpunit.xml
Add a test suite with the following two tests:

 <testsuite name="failing_group">
   <file>extensions/Flow/tests/phpunit/PermissionsTest.php</file>
   <file>extensions/Flow/tests/phpunit/Formatter/RevisionFormatterTest.php</file>
</testsuite>

Run the named test suite:

mw docker mediawiki exec -- composer run phpunit:entrypoint -- --testsuite failing_group

Observed behaviour
The test run fails:

$ mw docker mediawiki exec -- composer run phpunit:entrypoint -- --testsuite failing_group
> phpunit '--testsuite' 'failing_group'
Running with MediaWiki settings because there might be integration tests
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

...............................................................  63 / 131 ( 48%)
............................................................... 126 / 131 ( 96%)
.EE..                                                           131 / 131 (100%)

Time: 00:01.382, Memory: 129.00 MB

There were 2 errors:

1) Flow\Tests\Formatter\RevisionFormatterTest::testMockFormatterBasicallyWorks
Wikimedia\Services\ContainerDisabledException: Container disabled!

/src/vendor/wikimedia/services/src/ServiceContainer.php:403
===

2) Flow\Tests\Formatter\RevisionFormatterTest::testFormattingEditedTitle
Wikimedia\Services\ContainerDisabledException: Container disabled!

/src/vendor/wikimedia/services/src/ServiceContainer.php:403
===

ERRORS!
Tests: 131, Assertions: 132, Errors: 2.

Expected Behaviour
The tests should pass.

Details

Subject	Repo	Branch	Lines +/-
Don't hold reference to MediaWikiServices instance in tests	mediawiki/extensions/GrowthExperiments	master	+3 -8
Don't hold reference to MediaWikiServices instance in tests	mediawiki/extensions/Echo	master	+3 -11
Drop extra reference to OccupationController service	mediawiki/extensions/Flow	master	+4 -17
Don't hold reference to MediaWikiServices instance	mediawiki/extensions/MobileFrontend	master	+15 -21
Don't hold reference to MediaWikiServices instance in tests	mediawiki/extensions/FileImporter	master	+19 -25
Reset Hooks in testcase setup	mediawiki/extensions/Flow	master	+2 -0

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T225730 Reduce runtime of MW shared gate Jenkins jobs to 5 min
Open	pmiazga	T50217 Speed up MediaWiki PHPUnit build by running integration tests in parallel
Open	None	T361118 [REPO][CLIENT][SW] Reduce CI test runtime for Wikibase and related extensions
Open	None	T361190 Split PHPUnit test suite for wmf-quibble-vendor-mysql-php74-docker
Open	ArthurTaylor	T365567 [REPO][CLIENT][SW] Implement PHPUnit test suite splitting as composer tasks
Open	None	T365976 [REPO][CLIENT][SW] Create a `composer phpunit:prepare-parallel-test-suites` command to split suite into smaller groups
Open	None	T368390 RevisionFormatterTest.php fails when run in a suite with PermissionsTest.php
Open	None	T365978 [REPO][CLIENT][SW] Migrate Quibble from Python test suite splitting to PHP / composer implementation

Event Timeline

ArthurTaylor created this task.Tue, Jun 25, 11:51 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptTue, Jun 25, 11:51 AM

ArthurTaylor added a parent task: T365976: [REPO][CLIENT][SW] Create a `composer phpunit:prepare-parallel-test-suites` command to split suite into smaller groups.Tue, Jun 25, 11:52 AM

ArthurTaylor moved this task from In Task Breakdown to In Development on the Wikidata Dev Team (Wikidata.org Slice) board.

Maintenance_bot moved this task from Incoming to Ongoing on the wmde-wikidata-tech board.Tue, Jun 25, 12:29 PM

Maintenance_bot added a project: Wikidata.

On https://gerrit.wikimedia.org/r/1020276 in the FileImporter extension we currently get CI failures as well. While not identical they sound very similar. For future reference:

1) Flow\Tests\Collection\PostCollectionTest::testGetCollection
14:36:46 Wikimedia\Services\ContainerDisabledException: Container disabled!
14:36:46 
14:36:46 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:403
14:36:46 /workspace/src/includes/MediaWikiServices.php:354
14:36:46 /workspace/src/includes/MediaWikiServices.php:1653
14:36:46 /workspace/src/includes/ServiceWiring.php:2406
14:36:46 /workspace/src/includes/user/UserGroupManager.php:908
14:36:46 /workspace/src/extensions/Flow/includes/TalkpageManager.php:210
14:36:46 /workspace/src/extensions/Flow/includes/TalkpageManager.php:84
14:36:46 /workspace/src/extensions/Flow/tests/phpunit/PostRevisionTestCase.php:226
14:36:46 /workspace/src/extensions/Flow/tests/phpunit/Collection/PostCollectionTest.php:30
14:36:46 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:106
14:36:46 === Logs generated by test case
14:36:46 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"HashBagOStuff"}
14:36:46 [localisation] [debug] LocalisationCache using store LCStoreNull []
14:36:46 [DeferredUpdates] [debug] DeferredUpdates::run: started MediaWiki\Deferred\MWCallableUpdate_MediaWiki\User\UserGroupManager->addUserToGroup #{updateId} {"updateId":33324}
14:36:46 [DeferredUpdates] [debug] DeferredUpdates::run: ended MediaWiki\Deferred\MWCallableUpdate_MediaWiki\User\UserGroupManager->addUserToGroup #{updateId}, processing time: {walltime} {"updateId":33324,"walltime":0.0007109642028808594}
14:36:46 [MessageCache] [debug] MessageCache using store {class} {"class":"HashBagOStuff"}
14:36:46 [wfDebug] [debug] ParserFactory: using default preprocessor {"private":false}
14:36:46 [DeferredUpdates] [debug] DeferredUpdates::run: started MediaWiki\Deferred\MWCallableUpdate_MediaWiki\User\UserGroupManager->addUserToGroup #{updateId} {"updateId":42501}
14:36:46 [DeferredUpdates] [debug] DeferredUpdates::run: ended MediaWiki\Deferred\MWCallableUpdate_MediaWiki\User\UserGroupManager->addUserToGroup #{updateId}, processing time: {walltime} {"updateId":42501,"walltime":0.0006711483001708984}
14:36:46 [localisation] [debug] LocalisationCache::isExpired(en): cache missing, need to make one []
14:36:46 [objectcache] [debug] fetchOrRegenerate(wikidb-unittest_:linktargetstore-id:3%3ATest): miss, new value computed []
14:36:46 [BlockManager] [debug] Block cache miss with key BlockCacheKey{request=#33988,user=#41973,primary} []
14:36:46 [CentralAuth] [debug] Loading state for global user {user} from DB {"user":"Flow talk page manager"}
14:36:46 [DeferredUpdates] [debug] DeferredUpdates::run: started MediaWiki\Deferred\MWCallableUpdate_MediaWiki\User\UserGroupManager->addUserToGroup #{updateId} {"updateId":41970}
14:36:46 [DeferredUpdates] [debug] DeferredUpdates::run: ended MediaWiki\Deferred\MWCallableUpdate_MediaWiki\User\UserGroupManager->addUserToGroup #{updateId}, processing time: {walltime} {"updateId":41970,"walltime":0.0006620883941650391}
14:36:46 ===
14:36:46 
14:36:46 2) Flow\Tests\Collection\PostCollectionTest::testNewFromId
14:36:46 Wikimedia\Services\ContainerDisabledException: Container disabled!

The test in question is marked as integration test, even with @group Database. Nothing should be disabled.

Restricted Application added a project: Growth-Team. · View Herald TranscriptWed, Jun 26, 8:07 AM

thiemowmde moved this task from Sprint Backlog to Watching / Epic / Stalled on the WMDE-TechWish-Sprint-2024-06-12 board.Wed, Jun 26, 8:07 AM

My best guess at the moment is that somebody holds a local reference to the entire MediaWikiServices instance. But there is a chance that gets destroyed and rebuild. The old instance gets reported as "Container disabled!" then. Example CodeSearch: https://codesearch.wmcloud.org/search/?q=%3E.*%3D.*Services%3A%3AgetInstance%5CW*%3B&files=%5C.php%24. I will start cleaning up a few of these places.

The issues with the Flow tests are usually related to the fact that the flow plugin has its own container / service lifecycle management. The message Container disabled! in this case simply means that a service has a reference to a MediaWikiServices instance that has already been disposed (by the teardown of a previous test, for example). In this case, it seems to be coming from clearCacheCallbacks in the UserGroupManager:

foreach ( $this->clearCacheCallbacks as $callback ) {
	$callback( $user );
}

Probably one of the registered callbacks has a static reference to a service in a disposed container.

Change #1049839 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FileImporter@master] Don't hold reference to MediaWikiServices instance in tests

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

gerritbot added a project: Patch-For-Review.Wed, Jun 26, 8:35 AM

Change #1049847 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/MobileFrontend@master] Don't hold reference to MediaWikiServices instance

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

Change #1049856 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/GrowthExperiments@master] Don't hold reference to MediaWikiServices instance in tests

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

Change #1049849 had a related patch set uploaded (by Arthur taylor; author: Arthur taylor):

[mediawiki/extensions/Flow@master] Reset Hooks in testcase setup

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

ArthurTaylor added a parent task: T361190: Split PHPUnit test suite for wmf-quibble-vendor-mysql-php74-docker.Wed, Jun 26, 9:10 AM

ArthurTaylor moved this task from In Development to Ready for Peer Review on the Wikidata Dev Team (Wikidata.org Slice) board.Wed, Jun 26, 9:19 AM

Change #1049867 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Echo@master] Don't hold reference to MediaWikiServices instance in tests

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

Change #1049849 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Reset Hooks in testcase setup

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

Change #1049880 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Flow@master] Drop extra reference to OccupationController service

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

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.12; 2024-07-02).Wed, Jun 26, 11:00 AM

Change #1049847 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Don't hold reference to MediaWikiServices instance

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

Change #1049839 merged by jenkins-bot:

[mediawiki/extensions/FileImporter@master] Don't hold reference to MediaWikiServices instance in tests

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

Change #1049880 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Drop extra reference to OccupationController service

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

Change #1049867 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Don't hold reference to MediaWikiServices instance in tests

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

ArthurTaylor removed ArthurTaylor as the assignee of this task.Thu, Jun 27, 11:57 AM

ArthurTaylor moved this task from Ready for Peer Review to Ready for Tech Verification on the Wikidata Dev Team (Wikidata.org Slice) board.

Change #1049856 merged by jenkins-bot: