[go: up one dir, main page]

Page MenuHomePhabricator

Phase out usage of tests/phpunit/suite.xml
Closed, ResolvedPublic

Description

We have phpunit.xml.dist in the root of the MediaWiki repo, and it's intended to use this file with vendor/bin/phpunit (or composer phpunit) rather than the PHPUnit MediaWiki maintenance class at tests/phpunit/phpunit.php.

In order to phase out the usage of tests/phpunit/suite.xml, we need to ensure that all tests currently run via the maintenance class can be run with the plain phpunit entrypoint. We also need to ensure that the test files are located in directories expected by the test suites defined in phpunit.xml.dist (e.g. unit and integration directories).

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+222 -237
mediawiki/coremaster+274 -260
mediawiki/coremaster+23 -10
mediawiki/coremaster+44 -4
mediawiki/coremaster+37 -8
mediawiki/coremaster+129 -16
mediawiki/coremaster+28 -1
mediawiki/coremaster+24 -31
mediawiki/coremaster+17 -9
integration/quibblemaster+17 -18
mediawiki/coremaster+24 -16
integration/configmaster+25 -25
integration/configmaster+63 -1
integration/quibblemaster+23 -2
mediawiki/coremaster+69 -63
mediawiki/coremaster+18 -9
mediawiki/coremaster+22 -21
mediawiki/coremaster+232 -135
mediawiki/coremaster+7 -1
mediawiki/coremaster+39 -7
Show related patches Customize query in gerrit

Related Objects

Event Timeline

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

Probably a starting point here is to run composer phpunit:integration in CI, maybe just starting with core rather than core + extensions. While it's pretty limited in scope now eventually this would lead to double the amount of execution time (since we're running the same tests twice). I don't have ideas for how else we'd manage this transition though. (I guess we could also have a discussion about whether we want this transition -- I would be in favor of it but if others are opposed, let's discuss.)

When trying out your composer phpunit locally, I run into a few issues that I don't encounter with tests/php/phpunit.php. It would be nice if these could be fixed somehow.

  1. It wants to run tests for all extensions and skins in the filesystem, not just the ones that are actually enabled on the local wiki.
    • I actually have my LocalSettings.php set up to not enable extensions by default when running PHPUnit tests. Just core's tests already take a long time to run.
  2. The convertWarningsToExceptions="true" and convertNoticesToExceptions="true" directives in phpunit.xml.dist seem to not be working. Possibly that's what phpunit.php is fixing with lines 50–54.
  3. How do I specify --wiki=foowiki? Integration tests, at least, can need this.
  4. With phpunit.php I could just do php7.4 tests/php/phpunit.php to run with a different version of PHP. How do I do that with composer?
  5. phpunit.php prints "Using PHP $VERSION" before actually running the tests, and locally I have some code in my LocalSettings.php to remind me of some other aspects of my environment are in effect for the test run (e.g. which wiki, and whether extensions are enabled). It would be nice to keep that. Currently LocalSettings.php doesn't get loaded until after several thousand tests were already run.
  6. T246076: Wrong definition of MW_ENTRY_POINT when integration tests run.

With phpunit.php I could just do php7.4 tests/php/phpunit.php to run with a different version of PHP. How do I do that with composer?

Just to respond to this point quickly, you should be able to do php7.4 $(which composer) phpunit:unit.

Just to respond to this point quickly, you should be able to do php7.4 $(which composer) phpunit:unit.

It doesn't though, at some point it switches back to /usr/bin/php.

Even worse is when I want to run tests using phpdbg -qrr.

Change 741970 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] [WIP] phpunit: Remove suite.xml

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

Change 745263 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] phpunit.xml.dist: Cleanup and copy suites from tests/phpunit/suite.xml

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

Change 745263 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Cleanup phpunit.xml.dist, copy suites from tests/phpunit/suite.xml

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

Change 741970 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Default to vendor/bin/phpunit, remove suites.xml

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

Change 804526 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] [DNM] phpunit: Default to vendor/bin/phpunit, remove suites.xml (take 2)

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

Change 813198 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] phpunit.xml.dist: Align contents with suite.xml

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

Change 813209 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] phpunit: Add Integration/Unit bootstrap files

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

Change 813198 merged by jenkins-bot:

[mediawiki/core@master] phpunit.xml.dist: Align contents with suite.xml

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

Change 813327 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] phpunit: Remove suite.xml, default to vendor/bin/phpunit

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

Change 813209 abandoned by Kosta Harlan:

[mediawiki/core@master] phpunit: Add Integration/Unit bootstrap files

Reason:

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

I think the main roadblock here is that the two config files use different bootstraps; and that said bootstraps are meant to do different things (one loads all the settings, the other doesn't). I guess this could be resolved by loading the settings later on, e.g. inside MediaWikiIntegrationTestCase, expanding the existing initializeForStandardPhpunitEntrypointIfNeeded. I've already pushed r936355 and r936380 to consolidate the two bootstraps, after which it would be easier to see where they differ.

In T227900#5921573, Anomie wrote:
  1. It wants to run tests for all extensions and skins in the filesystem, not just the ones that are actually enabled on the local wiki.

This is still an issue, and a big one IMHO. I reported the same thing in T90875#7990627, and the previous attempt of switching to phpunit.xml.dist was eventually rolled back. It's probably also the hardest issue to fix, but I'm not sure if there's an easy solution for it, given that extensions/skins are enabled in the same config files that are also responsible for another bazillion things.

  1. The convertWarningsToExceptions="true" and convertNoticesToExceptions="true" directives in phpunit.xml.dist seem to not be working. Possibly that's what phpunit.php is fixing with lines 50–54.

It looks like this is the case. initializeForStandardPhpunitEntrypointIfNeeded should reset the error handler.

  1. How do I specify --wiki=foowiki? Integration tests, at least, can need this.

Good point. The code that reads the MW_PHPUNIT_WIKI can easily be moved to the common bootstrap.

  1. With phpunit.php I could just do php7.4 tests/php/phpunit.php to run with a different version of PHP. How do I do that with composer?

This is a valid concern, but if we manage to reach a position where there's a single entry point and a single config file, you wouldn't even have to use composer anymore. You could just run phpX.Y vendor/bin/phpunit and that'll work. So I'd rather not spend much time into this, and instead invest that time into achieving the eventual goal of a single config file.

  1. phpunit.php prints "Using PHP $VERSION" before actually running the tests

r936380 adds it to unit tests.

Change 936795 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: More improvements for PHPUnit bootstrap files

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

Change 937546 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Use PHPUnit hooks to tear down the test DB after the last test

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

Change 937552 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Streamline loading of Setup.php

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

Once all the patches above are merged, the main (and pretty much only) blocker that remains is loading the right set of extensions and skins. I don't have a good solution to that, unfortunately. The list of extensions to load is determined in LocalSettings.php, i.e. in the middle of the config loading process. I don't think there's a safe way to do one without doing the other. This would be much easier to fix with the experimental YAML settings file introduced in 1.38, but I don't see that becoming the only supported config mechanism any time soon (if ever).

I'll try and see if I can come up with some monkey patching of the config loading to make this work acceptably well.

Change 937559 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Determine what extensions to load in unit tests via config

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

Change 936795 merged by jenkins-bot:

[mediawiki/core@master] phpunit: More improvements for PHPUnit bootstrap files

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

Change 937546 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Use PHPUnit hooks to tear down the test DB after the last test

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

Change 937552 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Streamline loading of Setup.php

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

Change 937974 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Sync phpunit.xml.dist with tests/phpunit/suite.xml

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

Change 937980 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Deprecate tests/phpunit/suite.xml and composer phpunit:entrypoint

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

r937980 deprecates suite.xml and defaults to phpunit.xml.dist, which is the goal of this task. The problem is that with that patch lots of tests fail because they access MediaWikiServices too early, probably in data providers. So the first thing to do would be to disable access to MediaWikiServices in data providers. T273261 is related. I'll redo something like https://gerrit.wikimedia.org/r/c/mediawiki/core/+/813203/.

Change 937993 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Throw exception when MediaWikiServices is accessed too early

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

Change 938005 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Make getPHPUnitExtensionsAndSkins run the UnitTestList hook

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

One thing to note is that unit test now require a LocalSettings.php file. While I agree that this shouldn't be the case eventually, it has to be the case for now as a consequence of config being global code. In particular, we need a LocalSettings.php file for two reasons:

  1. Determine which extensions and skins should be loaded. As noted in T90875#7990627 and T227900#5921573, loading everything in the FS is unwanted. And the only way around that is to load a LocalSettings.php file.
  2. Run the UnitTestList hook (T298509). More generally, running any hook requires MW to be set up or the HookContainer won't know what to do.

Change 938012 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[integration/quibble@master] Run PHPUnit unit tests after installing MediaWiki

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

Next issue: the current way of handling integration tests from the unit entrypoint is to do a deferred initialization of MediaWiki (see MediaWikiIntegrationTestCase::initializeForStandardPhpunitEntrypointIfNeeded). This runs before any other method is called on the test class. However, it also runs much later than data providers. In practice, this means that data providers wouldn't be allowed to access configuration etc. One could say that this may not be too bad, but it's probably also too big a change to make now.

I think it would be much more predictable if the MediaWiki config was loaded on-demand after the bootstrap but before data providers, and only if at least a test extending MediaWikiIntegrationTestCase is going to be run. The problem is that PHPUnit 9 doesn't offer any way to do that. The hook system is very limited in its capabilities. The event system introduced in PHPUnit 10 is much more powerful, and I'm fairly confident that we'd be able to use one of its events to achieve this. However, PHPUnit 10 requires PHP >= 8.1, meaning we can't upgrade until we drop support for PHP 8 (T328922), which I don't see happening any time soon.

Instead, I'm considering trying a different approach: always load the MW configuration in unit tests (in the bootstrap), but then disable global-y things (Database, MediaWikiServices, global vars) in MediaWikiUnitTestCase (in beforeClass, and re-enable them afterwards). This is already done, to an extent, so I'd just be making the existing code stricter.

Note that test classes extending TestCase directly would bypass these restrictions. But that's its own can of worms and I don't want to open it now. We already have T319536 for fixing that in the longer term.

Like T227900#9014246, this is pretty much a step backwards. However, the current state of MW config (and, in this case, the lack of support for PHPUnit 10), put us on a steep slope where walking backwards is much easier, as it's what the environment naturally forces us to do.

Change 938019 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Disallow access to MediaWikiServices in unit tests

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

However, PHPUnit 10 requires PHP >= 8.1, meaning we can't upgrade until we drop support for PHP 8 (T328922), which I don't see happening any time soon.

Note that idea is to skip 8.0 and migrate to 8.1 right away: T319432. And while that task is indeed stalled for now on some minor-ish seeming dependency issues, I would hope that it might move fast after those are resolved. Though that may be wishful thinking, giving that I'm not even on that team and have no special insights.

However, PHPUnit 10 requires PHP >= 8.1, meaning we can't upgrade until we drop support for PHP 8 (T328922), which I don't see happening any time soon.

Note that idea is to skip 8.0 and migrate to 8.1 right away: T319432.

No, that's the plan for Wikimedia production, not MediaWiki's support. We're bound by https://www.mediawiki.org/wiki/Support_policy_for_PHP for when to drop PHP 8.0 and require 8.1+; given that Debian Bullseye shipped with 7.4, it's going to be a long while.

Change 938012 merged by jenkins-bot:

[integration/quibble@master] Run PHPUnit unit tests after installing MediaWiki

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

Change 938289 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/quibble@master] release: Quibble 1.5.5

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

However, PHPUnit 10 requires PHP >= 8.1, meaning we can't upgrade until we drop support for PHP 8 (T328922), which I don't see happening any time soon.

Note that idea is to skip 8.0 and migrate to 8.1 right away: T319432.

No, that's the plan for Wikimedia production, not MediaWiki's support. We're bound by https://www.mediawiki.org/wiki/Support_policy_for_PHP for when to drop PHP 8.0 and require 8.1+; given that Debian Bullseye shipped with 7.4, it's going to be a long while.

You're right. Thank you for pointing it out. Though that page is written in a way that is quite hard to parse for me. Even when putting https://wiki.debian.org/LTS and https://www.mediawiki.org/wiki/Version_lifecycle next to it, it is not clear to me with what MediaWiki version we can drop PHP 7.4 or 8.0 support according to that policy.

In any case, it seems to be a while until we can use enum and : never and all the other fanciness. Oh well.

Change 938289 merged by jenkins-bot:

[integration/quibble@master] release: Quibble 1.5.5

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

Change 938296 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/config@master] Docker [quibble] Upgrade Quibble to 1.5.5

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

Change 938296 merged by jenkins-bot:

[integration/config@master] Docker [quibble] Upgrade Quibble to 1.5.5

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

Mentioned in SAL (#wikimedia-releng) [2023-07-14T18:56:23Z] <James_F> Docker: Building and publishing quibble images # T90875 T227900 T341840

Change 938298 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/config@master] jjb: Update quibble jobs to images with Quibble 1.5.5

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

Change 938298 merged by jenkins-bot:

[integration/config@master] jjb: Update quibble jobs to images with Quibble 1.5.5

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

Change 937974 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Sync phpunit.xml.dist with tests/phpunit/suite.xml

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

r937980, the tip of my chain of patches, is now passing.

Change 937993 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Throw exception when MediaWikiServices is accessed too early

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

Change 945894 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] UrlUtils: Make assemble() and removeDotSegments() stateless

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

Change 945894 merged by jenkins-bot:

[mediawiki/core@master] UrlUtils: Make assemble() and removeDotSegments() stateless

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

Change 938019 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Disallow access to MediaWikiServices in unit tests

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

Change 937559 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Determine what extensions to load in unit tests via config

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

Change 938005 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Make getPHPUnitExtensionsAndSkins run the UnitTestsList hook

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

Change 937980 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Deprecate suite.xml and composer phpunit:entrypoint

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

Change 813327 abandoned by Kosta Harlan:

[mediawiki/core@master] phpunit: Default to vendor/bin/phpunit and phpunit.xml.dist

Reason:

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

Change 804526 abandoned by Kosta Harlan:

[mediawiki/core@master] phpunit: Default to vendor/bin/phpunit, remove suites.xml (take 2)

Reason:

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