Page MenuHomePhabricator

Upgrade to PHPUnit 10
Open, Needs TriagePublic

Description

PHPUnit 10 was released a few days ago. 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 (T328922)
  • 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
  • Test::getLinesToBeCovered, used in MediaWikiCoversValidator, has been moved to \PHPUnit\Metadata\Api::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 10 months now.
  • Config file schema has changed. Can be fixed quite easily by running PHPUnit with --migrate-configuration
  • Support for custom printers has been removed. We currently use this for MediaWikiPHPUnitResultPrinter, which adds the "Logs generated by test case" thing.
  • 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.
  • 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, 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; after r937980, MediaWikiDeprecatedConfigPHPUnitExtension as well) must be updated with PHPUnit 10's event system.

Related Objects

StatusSubtypeAssignedTask
StalledNone
OpenNone
OpenNone
StalledKrinkle
Resolved tstarling
ResolvedJdforrester-WMF
ResolvedJdforrester-WMF
Resolved tstarling
ResolvedReedy
ResolvedBUG REPORT tstarling
Resolved tstarling
ResolvedDaimona
ResolvedDaimona
ResolvedNone
ResolvedJdforrester-WMF
ResolvedBUG REPORTNone
Resolved tstarling
ResolvedJdforrester-WMF
Resolvedssastry
Resolvedkostajh
Resolvedkostajh
Resolvedthiemowmde
Resolved tstarling
Resolved tstarling
ResolvedBUG REPORTLucas_Werkmeister_WMDE
Resolvedhoo
Resolvedhoo
ResolvedJdforrester-WMF
Resolvedthiemowmde
Resolvedkostajh
ResolvedUmherirrender
ResolvedPRODUCTION ERROR brion
ResolvedTheresNoTime
Resolved tstarling
ResolvedJdforrester-WMF
Resolvedlarissagaulia
ResolvedJMeybohm
ResolvedMoritzMuehlenhoff
OpenNone
Resolvedjijiki
Resolvedaaron
Openjijiki
In ProgressClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
StalledClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedJoe
Resolvedcolewhite
ResolvedClement_Goubert
ResolvedClement_Goubert
In ProgressClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
InvalidClement_Goubert
ResolvedJoe
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedJoe
ResolvedJoe
ResolvedJoe
ResolvedJMeybohm
ResolvedJoe
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
DeclinedClement_Goubert
ResolvedClement_Goubert
OpenNone
StalledKrinkle
Resolvedjijiki
ResolvedJoe
ResolvedJoe
ResolvedClement_Goubert
ResolvedBUG REPORTClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedJoe
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedJclark-ctr
ResolvedJMeybohm
ResolvedJoe
ResolvedJoe
ResolvedNone
Resolvedjijiki
Resolvedjijiki
Resolveddancy
Resolveddancy
ResolvedJoe
ResolvedJoe
Resolvedjeena
ResolvedJoe
ResolvedJoe
Resolveddancy
ResolvedJoe
Resolved dpifke
Resolveddancy
ResolvedJoe
ResolvedClement_Goubert
Resolvedcolewhite
Resolvedjijiki
Resolved dpifke
ResolvedLegoktm
ResolvedClement_Goubert
ResolvedJMeybohm
ResolvedClement_Goubert
ResolvedClement_Goubert
OpenNone
OpenClement_Goubert
In ProgressClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
Resolvedhnowlan
Resolvedakosiaris
Openhnowlan
ResolvedClement_Goubert
ResolvedNone
ResolvedDreamy_Jazz
ResolvedPRODUCTION ERRORDreamy_Jazz
Resolvedkostajh
Resolvedjijiki
OpenNone
Resolvedkamila
ResolvedClement_Goubert
OpenClement_Goubert
Resolvedakosiaris
OpenNone
Resolvedakosiaris
Resolveddancy
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
ResolvedClement_Goubert
OpenClement_Goubert
Openjijiki
ResolvedJoe
In Progressjijiki
OpenNone
ResolvedJdforrester-WMF
ResolvedLucas_Werkmeister_WMDE
ResolvedDaimona
ResolvedDaimona
ResolvedDaimona
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedthiemowmde
OpenNone
OpenNone
ResolvedLucas_Werkmeister_WMDE
OpenNone
OpenNone
OpenNone
Resolved tstarling
OpenNone
ResolvedDreamy_Jazz
ResolvedDreamy_Jazz
ResolvedPhysikerwelt
ResolvedTgr
OpenNone
OpenNone
ResolvedNone
OpenNone
OpenNone
OpenNone
OpenNone

Event Timeline

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.

For the following:

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.

I ran PHPUnit with excimer enabled, and from the speedscope output it was immediately clear that 40% of the overall runtime was spent in TestSuiteBuilder::process, and particularly TestCase::valueObjectForEvents -> TestMethodBuilder::fromTestCase -> TestMethodBuilder::dataFor -> Exporter::export, which then recurses a ton of times (Exporter::recursiveExport). Looking at the code in question, what it does is take the arguments provided by a data provider, and obtain a string representation for them. Exporter is responsible for the "obtain a string representation" part, which as you can see has lots of special cases and really iterates objects deeply to obtain a full representation. This becomes an issue when a data provider returns a huge object, for which the exporting process takes a veeeery long time. I added some debug code to PHPUnit to print info about the exported data when the string representation is longer than 10M. The biggest offenders for core's "includes" suite are Language (13M), Message (14M), MockApi mocks (15M, and yes, we're mocking something called "Mock"), AuthenticationResponse (15M, often inside StatusValue), SystemBlock (15M), CompositeBlock (16M), FileBackendGroup (13M) and ParserOptions (15M).

I guess this would be addressed by the "return plain data" of T332865, but this may be quite hard to enforce, unlike the "static functions" part.

On top of that, TestCase::valueObjectForEvents is also called while tests are running, though I haven't looked into what it's used for. Overall, according to the excimer output, valueObjectForEvents() is responsible for 53% of the overall runtime of the PHPUnit process.

One feature I particularly like in PHPUnit 10, is the ability to pass multiple directories or files to phpunit. I've considered it a bug not to have this, in particular because the command is completely silent about the subsequent arguments, e.g. you can run phpunit tests/includes/ResourceLoader/* today and it would "succeed" with a bunch of meaningless dots, having actually only executed the first file that bash expanded the star to.

Likewise if you run phpunit includes/foo includes/bar it passes today, but actually only runs the first directory. No error, no warning.

https://github.com/sebastianbergmann/phpunit/pull/5462