Page MenuHomePhabricator

Upgrade PHPUnit in MediaWiki code to PHPUnit 10
Open, Needs TriagePublic

Description

PHPUnit 10 was released on February 3, 2023. According to the announcement, this release brings lots of changes to the internals of PHPUnit, like a new event system that replaces test listeners and hooks. It would also be interesting to see if upgrading would speed up our test suites. Note that PHPUnit 10 requires PHP >= 8.1, so we can't upgrade until prod is on 8.1. (T319432) and the minimum version requirement is bumped in the code. Also, we will need to make some changes to both the tests (for methods that were deprecated/removed) and the configuration (e.g., update the listeners we're using to the new event system). This task will be similar to T243600.

Incomplete list of known blockers/TODOs

  • Everything covered by the 9.6 upgrade task T342110
  • PHP 8.1+ dependency (T328921)
  • Deprecation notices are emitted for non-static data providers (T332865)
  • TestCase::run became final, currently overridden in MediaWikiIntegrationTestCase (T342259)
  • Return type hint of mixed added to TestCase::runTest, currently overridden in MediaWikiUnitTestCase and ApiTestCase, function also now private
  • Test::getLinesToBeCovered, used in MediaWikiCoversValidator, has been moved to PHPUnit\Metadata\Api\CodeCoverage::linesToBeCovered
  • PHPUnit prints the PHP version before starting tests, so we can remove the code which does the same thing in our bootstrap
  • johnkary/phpunit-speedtrap is currently not compatible with PHPUnit 10. A PR for this has been open for 3 years now.
  • T395743: Use of package ockcyp/covers-validator in libaries is not compatible with PHPUnit 10
  • Config file schema has changed. Can be fixed quite easily by running PHPUnit with --migrate-configuration, but this needs manual review (unsupported options are just quietly removed)
  • Make sure that PHPUnit is configured to display notices, deprecations, etc:
displayDetailsOnTestsThatTriggerDeprecations="true"
displayDetailsOnTestsThatTriggerErrors="true"
displayDetailsOnTestsThatTriggerNotices="true"
displayDetailsOnTestsThatTriggerWarnings="true"
displayDetailsOnPhpunitDeprecations="true"
  • Support for custom printers has been removed. We currently use this for MediaWikiPHPUnitResultPrinter, which adds the "Logs generated by test case" thing.
  • Make sure that data providers do not return complex object structures. PHPUnit will stringify them before running tests, potentially leading to massive slowdowns. This has been previously observed as PHPUnit hanging for ~40 seconds before actually starting tests. See T328919#9138704.
  • The order in which @before and similar methods are called has been changed. For some reason, it looks like it now calls the @before method in a subclass before the @before method in a parent class. This is a huge issue because our base test classes (Integration and UnitTestCase) use them, and for instance, MediaWikiLangTestCase also defines a @before method. Those are written with the assumption that the parent @before is called first. This may or may not be intentional: the code that lists hook methods pays attention to the ordering by using array_unshift (see HookMethods.php). The method which generates a list of class methods also takes inheritance into account and sorts methods accordingly, see Reflection.php. This piece of code was changed to sort methods highest to lowest, then the change was reverted, and then the revert was also reverted. I can confirm that in PHPUnit 10.3.2 the methods are sorted highest to lowest, which is the opposite of what we want (as long as array_unshift is used). But the code in HookMethods.php has been using array_unshift for a long time, before those changes to Reflection were made; additionally, the commit message of the Reflection change states "so that tests implemented in a parent class are run before the tests implemented in a child class", which doesn't mention hook methods. To me, it looks like Sebastian had no intention of changing the ordering of test methods, and simply forgot to update the array_unshift calls when reversing the ordering in Reflection. Note, this is also true, but in reverse, for @after and the other hook methods that are run after tests. Upstream issue: https://github.com/sebastianbergmann/phpunit/issues/5498 --> fixed in PHPUnit 10.3.3
  • TestSuite is now officially impossible to extend; the static suite() method has been removed, and the constructor has been made final and private. It was already pretty clear that this was going to happen when we upgraded to PHPUnit 8 and had to introduce SuiteEventsTrait. We currently use custom suite classes for two main use cases: parser tests, skin tests, and extension tests (including unit tests after r937559). These will need to be re-done differently. Note that these custom test suites are also currently preventing us from using paratest (T50217), because the list of tests is not known soon enough. See T345481.
  • Our custom PHPUnit extensions (currently MediaWikiLoggerPHPUnitExtension and MediaWikiTeardownPHPUnitExtension) must be updated with PHPUnit 10's event system.
  • TestCase::$backupGlobalsExcludeList and TestCase::$backupStaticPropertiesExcludeList now private, remove set in MediaWikiIntegrationTestCase::__construct (ref)
  • More stuff from T328919#10855098

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
StalledNone
StalledNone
OpenNone
OpenNone
ResolvedLucas_Werkmeister_WMDE
ResolvedDaimona
ResolvedDaimona
OpenNone
OpenNone
ResolvedArendpieter
OpenNone
ResolvedUmherirrender
Resolved larissagaulia
ResolvedUmherirrender
ResolvedJdforrester-WMF
ResolvedJdforrester-WMF
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenUmherirrender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

To figure out: it looks like the data provider phase runs MUCH slower in PHPUnit 10; locally, PHPUnit hangs for ~40 seconds before actually starting tests, as opposed to just a few seconds in PHPUnit 9. I still don't know what causes this, whether it's caused by the data provider changes, whether it's caused by our bootstrap code, etc.

Have we reported this to upstream PHPUnit? Sebastian seems pretty interested in performance recently. It's possible that there's a way we can do something different or better to avoid it, but I don't think he would mind finding out that this is something that can happen during upgrades. Perhaps he'd improve docs as a result to make the recommended solution easier to find, or perhaps some kind of warning or deprecation notice for whatever unusual thing we do.

To figure out: it looks like the data provider phase runs MUCH slower in PHPUnit 10; locally, PHPUnit hangs for ~40 seconds before actually starting tests, as opposed to just a few seconds in PHPUnit 9. I still don't know what causes this, whether it's caused by the data provider changes, whether it's caused by our bootstrap code, etc.

Have we reported this to upstream PHPUnit? Sebastian seems pretty interested in performance recently. It's possible that there's a way we can do something different or better to avoid it, but I don't think he would mind finding out that this is something that can happen during upgrades. Perhaps he'd improve docs as a result to make the recommended solution easier to find, or perhaps some kind of warning or deprecation notice for whatever unusual thing we do.

No, but at the time I came across issue #5184 (and duplicate #5183) which seems to be closely related. AIUI, the recommended solution is to use data providers as they're meant to be used, i.e. returning only plain data from them, as noted in T328919#9138704.

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

[mediawiki/core@master] tests: Replace phpunit-speedtrap with phpunit-slow-test-detector

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

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

[mediawiki/core@master] tests: Remove use of phpunit internal function TestCase::getName

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

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

[mediawiki/extensions/GrowthExperiments@master] tests: Remove use of phpunit internal function TestCase::getName

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

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

[mediawiki/extensions/Wikibase@master] tests: Remove use of phpunit internal function TestCase::getName

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

Change #1148443 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] tests: Remove use of phpunit internal function TestCase::getName

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

Change #1148444 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] tests: Remove use of phpunit internal function TestCase::getName

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

Change #1148442 merged by jenkins-bot:

[mediawiki/core@master] tests: Remove use of phpunit internal function TestCase::getName

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

Change #1149755 had a related patch set uploaded (by Krinkle; author: Umherirrender):

[mediawiki/extensions/Cognate@master] tests: Remove use of phpunit internal function TestCase::getActualOutput

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

Change #1149758 had a related patch set uploaded (by Krinkle; author: Umherirrender):

[mediawiki/core@master] tests: Remove use of phpunit internal function TestCase::getActualOutput

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

Change #1149755 merged by jenkins-bot:

[mediawiki/extensions/Cognate@master] tests: Remove use of phpunit internal function TestCase::getActualOutput

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

Change #1149758 merged by jenkins-bot:

[mediawiki/core@master] tests: Remove use of phpunit internal function TestCase::getActualOutput

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

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

[mediawiki/core@master] tests: Remove unused argument from data provider in PageUpdaterTest

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

Change #1149844 merged by jenkins-bot:

[mediawiki/core@master] tests: Remove unused argument from data provider in PageUpdaterTest

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

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

[mediawiki/core@master] Demo: Upgrade to PHPUnit 10.5

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

After fixing many of the non-static data provider in wmf-deployed code (T332865) I have tested PHPUnit on core. My demo patch can be found under https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1149986.
This should not block any other work to upgrade PHPUnit 10, feel free to start your own work, starting on my work or not, mention my work or not.

I have fixed some issues, with own patch sets, found in the failures, that were already invalid in PHPUnit 9 without failure (like use of internal PHPUnit functions). But there are many phpunit messages still to investigate and fix.

I have not start work for "big" things like use of the event system or running dynamic suites (T345481)

My notes from playing around with just the test cases:

  • dataProvider: PHPUnit 10 is more strict here
    • non-static provider are deprecated (replacement as part of T332865)
    • arguments on provider are deprecated
    • empty provider are deprecated (there are some in core) (via empty iterable or markSkippedTests)
    • string keys in provider data must match argument names (in preparation for use as named arguments in PHPUnit 11), there are a lot
  • many messages with output buffer not closed correctly in tests ("Test code or tested code did not close its own output buffers" and "Test code or tested code closed output buffers other than its own"), maybe related to teardown after exception, counted as "risky tests"
  • Suppressed warnings or notice from AtEase are showing up, maybe using wrong options as replacement in the xml config
  • bootstrap using many phpunit internal classes or functions, that are not under the compatibility promise
  • some test cases using internal functions, that no longer named like that or now private, or arguments changed (like TestCase::__construct)
  • Mocked class names must be unique (via getMockForAbstractClass or setMockClassName), but sometimes called repeatly
  • count of test cases are different, needs check via listing of tests if something runs twice or in wrong group

To get the upgrade done, many work of some person is still needed.

Thank you for all the work done on this! Indeed, it seems like there's a lot more to do -- and to simply investigate, even. And that's just for "smaller" changes. Basically, the conclusion is the same I reached a long time ago, by considering even just the dynamic suites stuff: this work can't be completed by a single person, and it's going to be difficult even for a small group of people doing it in their spare time. OTOH, I'm not sure if our PHPUnit tests/config are officially owned by someone, and I'm even more doubtful that this upgrade is part of anybody's plans. I'm not sure how to move this forward.

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

[mediawiki/core@master] tests: Rework ApiTestCase::expectApiErrorCode

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

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

[mediawiki/core@master] tests: Add ApiTestCase::expectApiErrorCodeFromCallback

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

Change #1152006 merged by jenkins-bot:

[mediawiki/core@master] tests: Add ApiTestCase::expectApiErrorCodeFromCallback

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

Change #1151803 merged by jenkins-bot:

[mediawiki/core@master] tests: Rework ApiTestCase::expectApiErrorCode

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

Jdforrester-WMF renamed this task from Upgrade to PHPUnit 10 to Upgrade to PHPUnit in MediaWiki code to PHPUnit 10.Jun 11 2025, 1:55 PM
Jdforrester-WMF subscribed.

Before the final push, is it worth working on libraries, switching them all over to PHPUnit 10? FWICS most of them should be trivial upgrades at this point.

Novem_Linguae renamed this task from Upgrade to PHPUnit in MediaWiki code to PHPUnit 10 to Upgrade PHPUnit in MediaWiki code to PHPUnit 10.Jun 11 2025, 4:36 PM

One thing that would be nice for CI: add --no-progress when running tests, because we don't really need to scroll through pages of progress output to get to the test failures.

That might put every test run in the state that T404107: Better visibility into PHPUnit split group status in CI would really like to fix where it is impossible to tell what the CI system is doing or if it is doing anything at all. We can certainly test it out, but we should be ready to drop that if it does obfuscate system status.

What might be nicer for surfacing test failures is reporting via an xunit json file. I assume that idea has come up before somewhere. Does anyone remember why we aren't already doing that?

Change #1148424 abandoned by Umherirrender:

[mediawiki/core@master] tests: Replace phpunit-speedtrap with phpunit-slow-test-detector

Reason:

Replacement package not usable in its current form

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

Change #1149986 abandoned by Umherirrender:

[mediawiki/core@master] Demo: Upgrade to PHPUnit 10.5

Reason:

demo patch only

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

Change #1148424 restored by Umherirrender:

[mediawiki/core@master] tests: Replace phpunit-speedtrap with phpunit-slow-test-detector

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

Change #1148424 merged by jenkins-bot:

[mediawiki/core@master] tests: Replace phpunit-speedtrap with phpunit-slow-test-detector

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

The change in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1148424
caused T418217: the new ergebnis/phpunit-slow-test-detector extension assumes that the toString() output of tests follows the ClassName::methodName convention. Scribunto’s custom toString() prepends EngineName: , which breaks that assumption. I submitted a patch that hopefully fixes this: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/1243176

Change #1243186 had a related patch set uploaded (by Arendpieter; author: Arendpieter):

[mediawiki/extensions/Scribunto@master] tests: Stop overriding TestCase::toString()

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

Change #1243186 merged by jenkins-bot:

[mediawiki/extensions/Scribunto@master] tests: Stop overriding TestCase::toString()

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

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

[mediawiki/core@master] tests: Adjust string keys in data provider or arguments on tests

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

Change #1243948 merged by jenkins-bot:

[mediawiki/core@master] tests: Adjust string keys in data provider or arguments on tests

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

Change #1253561 had a related patch set uploaded (by Arendpieter; author: Arendpieter):

[mediawiki/core@master] phpunit: Migrate PHPUnit extensions and config to PHPUnit 10

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

Change #1253561 abandoned by Arendpieter:

[mediawiki/core@master] phpunit: Migrate PHPUnit extensions and config to PHPUnit 10

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