Flow integration tests block GrowthExperiments merges with an unexpected ContainerDisabledException
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Sgs
	Nov 23 2023, 1:46 PM

Description

It seems Flow tests are consistently failing, example build 138397 and it is blocking GrowthExperiments merging.

1) Flow\Tests\Api\ApiFlowEditTopicSummaryTest::testEditTopicSummary
13:19:25 Wikimedia\Services\ContainerDisabledException: Container disabled!
13:19:25 
13:19:25 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:403
13:19:25 /workspace/src/includes/MediaWikiServices.php:349
13:19:25 /workspace/src/includes/MediaWikiServices.php:1602
13:19:25 /workspace/src/includes/ServiceWiring.php:2324
13:19:25 /workspace/src/includes/user/UserGroupManager.php:903
13:19:25 /workspace/src/extensions/Flow/includes/TalkpageManager.php:209
13:19:25 /workspace/src/extensions/Flow/includes/TalkpageManager.php:83
13:19:25 /workspace/src/extensions/Flow/includes/SubmissionHandler.php:158
13:19:25 /workspace/src/extensions/Flow/includes/WorkflowLoader.php:66
13:19:25 /workspace/src/extensions/Flow/includes/Api/ApiFlowBasePost.php:35
13:19:25 /workspace/src/extensions/Flow/includes/Api/ApiFlow.php:100
13:19:25 /workspace/src/includes/api/ApiMain.php:1931
13:19:25 /workspace/src/includes/api/ApiMain.php:877
13:19:25 /workspace/src/tests/phpunit/includes/api/ApiTestCase.php:169
13:19:25 /workspace/src/extensions/Flow/tests/phpunit/Api/ApiTestCase.php:71
13:19:25 /workspace/src/tests/phpunit/includes/api/ApiTestCase.php:209
13:19:25 /workspace/src/extensions/Flow/tests/phpunit/Api/ApiTestCase.php:86
13:19:25 /workspace/src/extensions/Flow/tests/phpunit/Api/ApiFlowEditTopicSummaryTest.php:20
13:19:25 /workspace/src/tests/phpunit/includes/api/ApiTestCase.php:322
13:19:25 phpvfscomposer:///workspace/src/vendor/phpunit/phpunit/phpunit:97

There are 5 errors in total yielding a ContainerDisabledException

Details

	Subject	Repo	Branch	Lines +/-
	tests: Remove ApiTestCase::setCurrentUser	mediawiki/extensions/Flow	master	+2 -17

Customize query in gerrit

Related Objects

Mentioned In: T351898: Reduce size of growthexperiments_user_impact.geui_data json blobs
Mentioned Here: T346108: [EPIC] IP Masking: StructuredDiscussions (Flow)/LiquidThreads Community discussion
T170330: Replace Flow DI (pimple) with MediaWikiServices

Event Timeline

Sgs created this task.Nov 23 2023, 1:46 PM

Urbanecm_WMF triaged this task as High priority.Nov 23 2023, 7:05 PM

Urbanecm_WMF added a project: ci-test-error.

I can reproduce the issue locally, but only when I run all Flow tests together (composer phpunit:entrypoint extensions/Flow/tests/phpunit/). When I run only composer phpunit:entrypoint extensions/Flow/tests/phpunit/Api/ApiFlowEditTopicSummaryTest.php, everything passes. This makes me think the issue is caused by another Flow test not being perfectly self-contained and breaking the service container by somehow calling MediaWikiServices::destroy and not re-initing the container afterwards, which appears to be the only way how this exception can be triggered. The question is why this happens.

Since the issue is reproducible by running only Flow's API tests, the issue has to be somewhere within the API tests. Checking calls to MediaWikiServices::destroy in test code shows:

MediaWikiIntegrationTestCase::overrideMwServices
MediaWikiIntegrationTestCase::restoreMwServices

I individually commented out both destroy calls to find out if disabling any of them would get the tests passing. When I commented only the second one, the Flow tests passed.

Urbanecm_WMF renamed this task from Exception while running phpunit tests to Flow integration tests block GrowthExperiments merges with an unexpected ContainerDisabledException.Nov 24 2023, 9:20 AM

Urbanecm_WMF mentioned this in T351898: Reduce size of growthexperiments_user_impact.geui_data json blobs.Nov 24 2023, 11:03 PM

The issue is caused by 946538: Replace MediaWikiIntegrationTestCase::$tablesUsed with automatic query tracking. I don't currently understand how exactly, but reverting that patch locally makes the issue go away. Pinging @Daimona @tstarling as the patch author/reviewer – can you help with fixing this problem, please?

FWIW, this currently breaks all merges to GrowthExperiments.

In T351888#9355937, @Urbanecm_WMF wrote:

I can reproduce the issue locally, but only when I run all Flow tests together (composer phpunit:entrypoint extensions/Flow/tests/phpunit/).

Yup, I too can reproduce this locally, by running just ApiFlowEditTitleTest and ApiFlowEditTopicSummaryTest.

In T351888#9357489, @Urbanecm_WMF wrote:

The issue is caused by 946538: Replace MediaWikiIntegrationTestCase::$tablesUsed with automatic query tracking. I don't currently understand how exactly, but reverting that patch locally makes the issue go away.

Reverting that patch does not fix the issue for me. In fact, the test still fails if I check out REL1_41 of both core and Flow. BUT, reverting the tablesUsed patch does fix the issue if I run all the tests in Flow/tests/phpunit/Api, instead of just the two tests above. Which presumably means the code has "always" been broken, but some mysterious side effect (caused by a combination of running multiple tests AND using tablesUsed) was hiding the failure. The tablesUsed patch removed one of the two causes of that side effect, and the bug came up. Mostly, I'm happy that things have (finally) broken visibly.

I still haven't determined what the side effect in question is.

Also, it looks like the failure is caused by the setCurrentUser call in Flow's ApiTestCase::setUp, and ::getContainer in particular. I'm really looking forward to discovering the new, fun way in which Flow's custom service container is setting MediaWiki on fire.

Umherirrender edited projects, added ci-test-error (WMF-deployed Build Failure); removed ci-test-error.Nov 26 2023, 11:19 AM

Change 977290 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/Flow@master] tests: Remove ApiTestCase::setCurrentUser

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

gerritbot added a project: Patch-For-Review.Nov 26 2023, 12:39 PM

Just removing this Container call in the test shows other places with failure from AbuseFilter/CheckUser involved, clearing the hooks works in that situation, not sure if that is the best fix.

Flow's container logic is not the best as there are now services (T170330). The global user is in the container and in the FlowUser service, two places are not good to setup tests correctly.

Change 977290 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] tests: Remove ApiTestCase::setCurrentUser

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

Maintenance_bot removed a project: Patch-For-Review.Nov 26 2023, 5:30 PM

ReleaseTaggerBot added a project: MW-1.42-notes (1.42.0-wmf.7; 2023-11-28).Nov 26 2023, 6:00 PM

In T351888#9357772, @Umherirrender wrote:

Just removing this Container call in the test shows other places with failure from AbuseFilter/CheckUser involved, clearing the hooks works in that situation, not sure if that is the best fix.

Thank you so much for the fix! I've merged it, since the patch does its job and it's preferable having a working CI over a perfect solution. Since the tests pass now, I'm also resolving the task – feel free to log a task for future test improvements if needed.