Page MenuHomePhabricator

Convert tests/phpunit/phpunit.php entrypoint to plain PHPUnit with bootstrap file
Open, LowPublic

Description

Currently, core tests cannot be run by running the phpunit command directly. Instead, we have a custom entrypoint that wraps PHPUnit and calls it programmatically from tests/phpunit/phpunit.php.

We should convert all our logic in the wrapper to a PHPUnit bootstrap file, so that we (and other automated tools) can start PHPUnit through the regular entry point.

Next steps:

  • Adjust configuration
    • Do we still need two separate files? If not, merge. If yes, perhaps we could add a composer script to switch between configs easily.
    • Note that we also have two different bootstrap files
  • Move everything from phpunit.php to the bootstrap file
  • Delete phpunit.php and fix everything that refers directly to it

Related Objects

Event Timeline

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

Existing test suites on suite.xml:

<testsuites>
		<testsuite name="includes">
			<directory>includes</directory>
		</testsuite>
		<testsuite name="languages">
			<directory>languages</directory>
		</testsuite>
		<testsuite name="parsertests">
			<file>suites/CoreParserTestSuite.php</file>
			<file>suites/ExtensionsParserTestSuite.php</file>
		</testsuite>
		<testsuite name="skins">
			<directory>skins</directory>
			<directory>structure</directory>
			<file>suites/ExtensionsTestSuite.php</file>
			<file>suites/LessTestSuite.php</file>
		</testsuite>
		<!-- As there is a class Maintenance, we cannot use the name "maintenance" directly -->
		<testsuite name="maintenance_suite">
			<directory>maintenance</directory>
		</testsuite>
		<testsuite name="structure">
			<directory>structure</directory>
		</testsuite>
		<testsuite name="tests">
			<directory>tests</directory>
		</testsuite>
		<testsuite name="uploadfromurl">
			<file>suites/UploadFromUrlTestSuite.php</file>
		</testsuite>
		<testsuite name="extensions">
			<directory>structure</directory>
			<file>suites/ExtensionsTestSuite.php</file>
			<file>suites/ExtensionsParserTestSuite.php</file>
			<file>suites/LessTestSuite.php</file>
		</testsuite>
		<testsuite name="documentation">
			<directory>documentation</directory>
		</testsuite>
		<testsuite name="unit">
			<directory>unit</directory>
		</testsuite>
	</testsuites>

In phpunit.xml.dist we currently have two test suites, "unit" (no DB, no global functions) and "integration" (everything else).

A number of the test suites in suite.xml probably don't need to be defined as test suites, for example you could run vendor/bin/phpunit tests/phpunit/includes just as easily as vendor/bin/phpunit --testsuite=includes. So I would propose that we not bring over those types of test suites into phpunit.xml.dist.

We also want to ensure we don't have testsuites defined which result in the same test being run more than once.

Change 560835 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] Add a bootstrap file to run PHPUnit on vagrant through PhpStorm

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

I've uploaded a change that achieves what this task asks for. I originally intended to use it to debug tests using PhpStorm, until I found out that I could use it without PhpStorm too.
If T173899: autoload.ide.php needs to be updated or removed is a concern, I'll gladly volunteer as owner; I am the one using it after all.

Change 560835 abandoned by Mainframe98:
Add a bootstrap file to run PHPUnit on vagrant through PhpStorm

Reason:
xDebug on Vagrant is currently broken; based on Florianschmidtwelzow's feedback, the solution should probably live elsewhere.

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

Change 520316 abandoned by Kosta Harlan:
Bring parser tests suite from suite.xml to phpunit.xml.dist

Reason:
Will come back to this some other time.

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /520316

I've taken a look at this. Most things should be doable by moving everything to the bootstrap file. For a start, we might make phpunit.php not inherit from Maintenance, and copy from Maintenance only the things that we really need. The main problem is understanding what exactly we really need.

Also, I think one thing to tackle first is that we use some custom CLI options. This is not going to be implemented upstream (ref). I think removing these options in favour of something else (env variables?) would be a major step forward. And do the same with any parameter added from Maintenance::addDefaultParams that might be used in tests (e.g. --wiki).

A number of the test suites in suite.xml probably don't need to be defined as test suites, for example you could run vendor/bin/phpunit tests/phpunit/includes just as easily as vendor/bin/phpunit --testsuite=includes. So I would propose that we not bring over those types of test suites into phpunit.xml.dist.

I second this.

A number of the test suites in suite.xml probably don't need to be defined as test suites, for example you could run vendor/bin/phpunit tests/phpunit/includes just as easily as vendor/bin/phpunit --testsuite=includes. So I would propose that we not bring over those types of test suites into phpunit.xml.dist.

I second this.

Third'ed 🙂

But.. we also have change 581131 (T50217) which actually splits up stuff even more in preparation for paratest and the amazing runtime it would offer us (bringing down CI from over 25min to possibly well under 5min with this one change alone, anecdotally confirmed by @aaron on his laptop).

A number of the test suites in suite.xml probably don't need to be defined as test suites, for example you could run vendor/bin/phpunit tests/phpunit/includes just as easily as vendor/bin/phpunit --testsuite=includes. So I would propose that we not bring over those types of test suites into phpunit.xml.dist.

I second this.

Third'ed 🙂

But.. we also have change 581131 (T50217) which actually splits up stuff even more in preparation for paratest and the amazing runtime it would offer us (bringing down CI from over 25min to possibly well under 5min with this one change alone, anecdotally confirmed by @aaron on his laptop).

So IIUC paratest wants separate testsuites? If this is the case, personally I'd be very happy with adding a bazillion of testsuites, as long as that allows running tests in parallel.

A number of the test suites in suite.xml probably don't need to be defined as test suites, for example you could run vendor/bin/phpunit tests/phpunit/includes just as easily as vendor/bin/phpunit --testsuite=includes. So I would propose that we not bring over those types of test suites into phpunit.xml.dist.

I second this.

Third'ed 🙂

But.. we also have change 581131 (T50217) which actually splits up stuff even more in preparation for paratest and the amazing runtime it would offer us (bringing down CI from over 25min to possibly well under 5min with this one change alone, anecdotally confirmed by @aaron on his laptop).

So IIUC paratest wants separate testsuites? If this is the case, personally I'd be very happy with adding a bazillion of testsuites, as long as that allows running tests in parallel.

From a quick look at the paratest docs, I don't see why further fragmenting the test suites is necessary.


I've been thinking off and on about how we could move this task forward. It seems like a few pieces are needed:

  1. Modify quibble so that if tests/phpunit/phpunit.php is absent, it knows to use vendor/bin/phpunit instead.
  2. Make a patch that removes tests/phpunit/phpunit.php
  3. Have another patch does the lifting of moving stuff out of the maintenance class into bootstrap.maintenance.php, or wherever we decide to place it

+1 on using environment variables as a workaround to the maintenance class options/arguments, but maybe there are also some options/arguments we can drop if they are not being used in CI by Quibble.

Change 672758 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Decouple phpunit.php from Maintenance

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

Change 672762 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Simplify / clean up phpunit.php

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

I've pushed the two patches above to get the ball rolling. I think the main blocker right now is understanding what to do with the custom CLI options. We have the PHPUnit-specific options, plus the options from Maintenance that I wanted to keep for now: "wiki" and "db*". Once these are sorted out, we can probably move everything to the boostrap. Below is my opinion about specific parameters:

  • wiki, dbuser, dbpass, dbdefaultgroup: First, understand whether these are needed (is anybody using them?). If needed, we might move them to env variables.
  • use-filebackend: only two tests using it. Is it needed? Can be dropped or moved to env
  • use-bagostuff: Only BagOStuffTest uses it. Should be removed IMHO. A saner approach would be to have different tests for different BagOStuff mediums, skipping the ones for mediums not currently available
  • use-jobqueue: Only used by JobQueueTest, same as use-bagostuff.
  • use-normal-tables and reuse-db: Can become env variables if we need them

Change 672777 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Remove two MW-specific PHPUnit options

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

I've pushed the two patches above to get the ball rolling. I think the main blocker right now is understanding what to do with the custom CLI options. We have the PHPUnit-specific options, plus the options from Maintenance that I wanted to keep for now: "wiki" and "db*". Once these are sorted out, we can probably move everything to the boostrap. Below is my opinion about specific parameters:

  • wiki, dbuser, dbpass, dbdefaultgroup: First, understand whether these are needed (is anybody using them?). If needed, we might move them to env variables.

MediaWiki-Vagrant needs wiki because it uses MWMultiVersion for multi-wiki setup.

I've pushed the two patches above to get the ball rolling. I think the main blocker right now is understanding what to do with the custom CLI options. We have the PHPUnit-specific options, plus the options from Maintenance that I wanted to keep for now: "wiki" and "db*". Once these are sorted out, we can probably move everything to the boostrap.

Neat.

Below is my opinion about specific parameters:

  • wiki, dbuser, dbpass, dbdefaultgroup: First, understand whether these are needed (is anybody using them?). If needed, we might move them to env variables.

dbuser, dbpass, dbserver, and dbpath are all used by quibble. wiki is indeed used in multi-wiki environments. Moving them to env variables should be OK. Can't see any usage of dbdefaultgroup anywhere.

  • use-filebackend: only two tests using it. Is it needed? Can be dropped or moved to env
  • use-bagostuff: Only BagOStuffTest uses it. Should be removed IMHO. A saner approach would be to have different tests for different BagOStuff mediums, skipping the ones for mediums not currently available
  • use-jobqueue: Only used by JobQueueTest, same as use-bagostuff.

Agreed, let's re-do these to have different config tests?

  • use-normal-tables and reuse-db: Can become env variables if we need them

I think these are options for special local debugging. People will want them still, I imagine.

I've pushed the two patches above to get the ball rolling. I think the main blocker right now is understanding what to do with the custom CLI options. We have the PHPUnit-specific options, plus the options from Maintenance that I wanted to keep for now: "wiki" and "db*". Once these are sorted out, we can probably move everything to the boostrap. Below is my opinion about specific parameters:

  • wiki, dbuser, dbpass, dbdefaultgroup: First, understand whether these are needed (is anybody using them?). If needed, we might move them to env variables.

MediaWiki-Vagrant needs wiki because it uses MWMultiVersion for multi-wiki setup.

But is it used for PHPUnit?

  • wiki, dbuser, dbpass, dbdefaultgroup: First, understand whether these are needed (is anybody using them?). If needed, we might move them to env variables.

dbuser, dbpass, dbserver, and dbpath are all used by quibble. wiki is indeed used in multi-wiki environments. Moving them to env variables should be OK. Can't see any usage of dbdefaultgroup anywhere.

AFAIK, quibble uses these for install.php. I couldn't find any usage of these options for PHPUnit on codesearch (nor in quibble, which is not indexed on codesearch apparently). Also, there's basically nothing using PHPUnit options.

  • use-filebackend: only two tests using it. Is it needed? Can be dropped or moved to env
  • use-bagostuff: Only BagOStuffTest uses it. Should be removed IMHO. A saner approach would be to have different tests for different BagOStuff mediums, skipping the ones for mediums not currently available
  • use-jobqueue: Only used by JobQueueTest, same as use-bagostuff.

Agreed, let's re-do these to have different config tests?

Two of them done in the patch above, the third one is a tad more complicated.

  • use-normal-tables and reuse-db: Can become env variables if we need them

I think these are options for special local debugging. People will want them still, I imagine.

Makes sense.


Also, to expand my argument about wiki: in general, our test suite should not depend on the local config (it shouldn't use LocalSettings etc.), and shouldn't be customizable. This is so that whoever runs it, on whatever machine, is almost guaranteed to return consistent results. I am afraid that 'wiki' might be used for this. Ideally, it shouldn't matter what wiki you're running tests on.

Also, to expand my argument about wiki: in general, our test suite should not depend on the local config (it shouldn't use LocalSettings etc.), and shouldn't be customizable. This is so that whoever runs it, on whatever machine, is almost guaranteed to return consistent results. I am afraid that 'wiki' might be used for this. Ideally, it shouldn't matter what wiki you're running tests on.

Oh, and I should say: this is true as long as all wikis are using the same MW version. Running tests on a specific version of MW should indeed be possible, so if 'wiki' is sometimes needed for that, it should stay.

Also, to expand my argument about wiki: in general, our test suite should not depend on the local config (it shouldn't use LocalSettings etc.), and shouldn't be customizable. This is so that whoever runs it, on whatever machine, is almost guaranteed to return consistent results. I am afraid that 'wiki' might be used for this. Ideally, it shouldn't matter what wiki you're running tests on.

Oh, and I should say: this is true as long as all wikis are using the same MW version. Running tests on a specific version of MW should indeed be possible, so if 'wiki' is sometimes needed for that, it should stay.

It's for which extension is loaded on which wiki. E.g. you can't run the WikibaseRepo DB tests on your local test metawiki, but you can on your local test commonswiki.

MediaWiki-Vagrant needs wiki because it uses MWMultiVersion for multi-wiki setup.

But is it used for PHPUnit?

Yes, it won't run tests without it.

  • wiki, dbuser, dbpass, dbdefaultgroup: First, understand whether these are needed (is anybody using them?). If needed, we might move them to env variables.

dbuser, dbpass, dbserver, and dbpath are all used by quibble. wiki is indeed used in multi-wiki environments. Moving them to env variables should be OK. Can't see any usage of dbdefaultgroup anywhere.

AFAIK, quibble uses these for install.php. I couldn't find any usage of these options for PHPUnit on codesearch (nor in quibble, which is not indexed on codesearch apparently). Also, there's basically nothing using PHPUnit options.

Sorry, yes, you're right, they seem to be unused.

Also, to expand my argument about wiki: in general, our test suite should not depend on the local config (it shouldn't use LocalSettings etc.), and shouldn't be customizable. This is so that whoever runs it, on whatever machine, is almost guaranteed to return consistent results. I am afraid that 'wiki' might be used for this. Ideally, it shouldn't matter what wiki you're running tests on.

I agree with this direction as well, but, we do need to keep loading LocalSettings.php most likely for integration tests since they need a database, and we do support multiple different database backends. Having to specify one's dbtype/host/password on the CLI every time one runs phpunit tests seems needlessly impairing developer usability.

However, I think a compromise might be to keep expanding use of the *UnitTestCase base class where we only consider DefaultSettings, and to keep improving *IntegrationTestCase and tests using it such that only a very narrow slice of configs (web server, db, binary pointers, backend/cache services, maybe a handful of other things) are allowed to come through. These are not meant to be different virtual test cases though (so the bagostuff parameters indeed seem like an anti-pattern to me), but I do think it makes sense to support real integration tests with a real database, and we'll want to continue to test multiple PHP versions and DB types in CI, and similarly allow devs to test with the main config they prefer or care about.

It's for which extension is loaded on which wiki. E.g. you can't run the WikibaseRepo DB tests on your local test metawiki, but you can on your local test commonswiki.

This is a valid point. So IIUC, you'd need 'wiki' (and consequently MW_DB) in order to apply the right configuration in LocalSettings, right?

I agree with this direction as well, but, we do need to keep loading LocalSettings.php most likely for integration tests since they need a database, and we do support multiple different database backends. Having to specify one's dbtype/host/password on the CLI every time one runs phpunit tests seems needlessly impairing developer usability.

However, I think a compromise might be to keep expanding use of the *UnitTestCase base class where we only consider DefaultSettings, and to keep improving *IntegrationTestCase and tests using it such that only a very narrow slice of configs (web server, db, binary pointers, backend/cache services, maybe a handful of other things) are allowed to come through. These are not meant to be different virtual test cases though (so the bagostuff parameters indeed seem like an anti-pattern to me), but I do think it makes sense to support real integration tests with a real database, and we'll want to continue to test multiple PHP versions and DB types in CI, and similarly allow devs to test with the main config they prefer or care about.

Fair. And loading extensions is also a reason to keep LocalSettings. However, we might hack it so that only certain config options are picked up. E.g.

function requireSettings() {
   global $globalWeWant1, $globalWeWant2;

   require_once 'path/to/LS.php';
}

Change 673069 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] phpunit.php: Remove DB-related options

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

Change 673074 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] phpunit.php: Move remaining CLI options to env variables

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

Change 672758 merged by jenkins-bot:
[mediawiki/core@master] Decouple phpunit.php from Maintenance

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

Change 672762 merged by jenkins-bot:

[mediawiki/core@master] Simplify / clean up phpunit.php

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

Question from code review:

@Daimona wrote on Change 672777:

If the test classes all subclass a baseclass, there can be one per-backend. Each would look for some an entry in a testing configuration array and skip the test if one is not present for the backend.

Yes, this is also what I proposed on the task. My question is exactly what test classes would be needed, e.g. if some class should be skipped, or if certain classes should also be tested with different configs.

@aaron Could you look into this and maybe tell Daimona what kind of test structure you'd like to see for file backend and job queue?

There could be a FileBackendTestBase with subclasses for each backend. The "proxy" backend classes (FileBackendMultiWrite) could just use MemoryFileBackend instances. The tests for MemoryFileBackend would not need any config. The FSFileBackend subclass could just use the tmp directory. The other FileBackendStore subclass would need site config pointing to a real backend...

If there was a $wgBackendIntegrationTestConfigByClass setting, a map of (full class name => config array), then each test subclass could look for the $wgIntegrationTestBackends entry with the ::class name of the backend subclass. The test would be marked skipped if the config or a driver is missing. The JobQueue could use the same pattern and config variable. Maybe the config variable could go in an IntegrationTestSettings.php file instead of LocalSettings.php, which might avoid some overhead and coupling when running single files in phpunit.

There could be a FileBackendTestBase with subclasses for each backend. The "proxy" backend classes (FileBackendMultiWrite) could just use MemoryFileBackend instances. The tests for MemoryFileBackend would not need any config. The FSFileBackend subclass could just use the tmp directory. The other FileBackendStore subclass would need site config pointing to a real backend...

If there was a $wgBackendIntegrationTestConfigByClass setting, a map of (full class name => config array), then each test subclass could look for the $wgIntegrationTestBackends entry with the ::class name of the backend subclass. The test would be marked skipped if the config or a driver is missing. The JobQueue could use the same pattern and config variable. Maybe the config variable could go in an IntegrationTestSettings.php file instead of LocalSettings.php, which might avoid some overhead and coupling when running single files in phpunit.

This seems like it won't be straightforward. Perhaps we can make move the relevant flags to env variables, and create another task for removing them later?

Change 672777 abandoned by Daimona Eaytoy:

[mediawiki/core@master] Remove three MW-specific PHPUnit options

Reason:

I have changed these options to env variables in I597ed2b5666f4214173609f7e77e23dbc4fd81ae instead. I think this is acceptable as a temporary solution.

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

Using env vars for now seems OK for now.

FTR, next steps:

  • r673074 so we don't have any more custom CLI options
  • Adjust configuration
    • Do we still need two separate files? If not, merge. If yes, perhaps we could add a composer script to switch between configs easily.
    • Note that we also have two different bootstrap files
  • Move everything from phpunit.php to the bootstrap file
  • Delete phpunit.php and fix everything that refers directly to it

Change 673069 merged by jenkins-bot:

[mediawiki/core@master] phpunit.php: Remove DB-related options

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

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

[integration/quibble@master] phpunit: Add support for vendor/bin/phpunit entrypoint

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

Change 673074 merged by jenkins-bot:

[mediawiki/core@master] phpunit.php: Move remaining CLI options to env variables

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

FTR, next steps:

  • r673074 so we don't have any more custom CLI options
  • Adjust configuration
    • Do we still need two separate files? If not, merge. If yes, perhaps we could add a composer script to switch between configs easily.
    • Note that we also have two different bootstrap files
  • Delete phpunit.php and fix everything that refers directly to it

IMO we don't need two config files; we should switch everything to use phpunit.xml.dist in the repo root and get rid of suite.xml. Likewise, bootstrap.maintenance.php could be removed.

  • Move everything from phpunit.php to the bootstrap file

We should be careful about what we bring over. Running integration tests works with vendor/bin/phpunit + phpunit.xml.dist + the bootstrap.php file, FWIW.

As requested in the email on wikitech-l:

  • CirrusSearch integration tests rely on --wiki=wiki in vagrant.
  • Some of the CirrusSearch integration tests also rely on LocalSettings.php being set up right. If $wgLanguageCode is not set to "en" some of the tests will fail.
  • Some of the CirrusSearch integration tests also rely on LocalSettings.php being set up right. If $wgLanguageCode is not set to "en" some of the tests will fail.

For that one, see also T277456: Use qqx as content language for PHPUnit tests

IMO we don't need two config files; we should switch everything to use phpunit.xml.dist in the repo root and get rid of suite.xml. Likewise, bootstrap.maintenance.php could be removed.

Having a single file is probably the best choice in terms of usability, yes.

  • Move everything from phpunit.php to the bootstrap file

We should be careful about what we bring over. Running integration tests works with vendor/bin/phpunit + phpunit.xml.dist + the bootstrap.php file, FWIW.

Right, we might want to first ensure that everything is needed, and then find a way to avoid unnecessary setup (e.g. loading LS.php) for unit tests.

I think I'll be working on this at the hackathon, so if you or anybody else is interested, we might pair.

As requested in the email on wikitech-l:

  • CirrusSearch integration tests rely on --wiki=wiki in vagrant.

Right, this thing in vagrant was pointed out earlier. This can be remediated by using an environment variable instead, PHPUNIT_WIKI=wiki.

  • Some of the CirrusSearch integration tests also rely on LocalSettings.php being set up right. If $wgLanguageCode is not set to "en" some of the tests will fail.

While this isn't affected by the changes here, it's actually subpar because tests are less consistent. As an alternative to using qqx, forcing the language to English inside the test class would already mitigate this.

As requested in the email on wikitech-l:

  • CirrusSearch integration tests rely on --wiki=wiki in vagrant.

Right, this thing in vagrant was pointed out earlier. This can be remediated by using an environment variable instead, PHPUNIT_WIKI=wiki.

That doesn't actually work. The issue here is that vagrant requires --wiki for MWMultiVersion. It will not work without it. If you pass it at the moment, the tests do not run and instead show unrecognized option --wiki.

Having phpunit.php remove --wiki or --wiki= from $argv is a workaround, or MWMultiVersion can be changed to accept an environment variable instead perhaps.

That doesn't actually work. The issue here is that vagrant requires --wiki for MWMultiVersion. It will not work without it. If you pass it at the moment, the tests do not run and instead show unrecognized option --wiki.

Having phpunit.php remove --wiki or --wiki= from $argv is a workaround, or MWMultiVersion can be changed to accept an environment variable instead perhaps.

AIUI, this is issue happens because tests are being run as a mwscript. This is going to be impossible anyway once this task is resolved. So another possibility might be to just finish the migration.

That doesn't actually work. The issue here is that vagrant requires --wiki for MWMultiVersion. It will not work without it. If you pass it at the moment, the tests do not run and instead show unrecognized option --wiki.

Having phpunit.php remove --wiki or --wiki= from $argv is a workaround, or MWMultiVersion can be changed to accept an environment variable instead perhaps.

AIUI, this is issue happens because tests are being run as a mwscript.

But, just running php phpunit.php requires a --wiki. Trying to run php /vagrant/mediawiki/tests/phpunit/phpunit.php --help fails because Vagrant's LocalSettings invokes MWMultiVersion.

Following the stacktrace leads to vagrant's CommonSettings calling MWMultiVersion when MW_PHPUNIT_TEST is defined (T130171: Fix fundraising PHPUnit tests under mw-vagrant).

This is going to be impossible anyway once this task is resolved. So another possibility might be to just finish the migration.

That would make running the unit test under Vagrant impossible, unless I'm misunderstanding the end goal here.

That doesn't actually work. The issue here is that vagrant requires --wiki for MWMultiVersion. It will not work without it. If you pass it at the moment, the tests do not run and instead show unrecognized option --wiki.

Having phpunit.php remove --wiki or --wiki= from $argv is a workaround, or MWMultiVersion can be changed to accept an environment variable instead perhaps.

AIUI, this is issue happens because tests are being run as a mwscript.

But, just running php phpunit.php requires a --wiki. Trying to run php /vagrant/mediawiki/tests/phpunit/phpunit.php --help fails because Vagrant's LocalSettings invokes MWMultiVersion.

The point of this task is to remove tests/phpunit/phpunit.php entirely. Can you try with vendor/bin/phpunit?

The point of this task is to remove tests/phpunit/phpunit.php entirely. Can you try with vendor/bin/phpunit?

Is there a specific way to invoke this? Just using vendor/bin/phpunit doesn't work, it doesn't load the required classes. Passing suite.xml through --configuration shows me the message

You are running these tests directly from phpunit. You may not have all globals correctly set.
Running phpunit.php instead is recommended.#!/usr/bin/env php
A copy of your installation's LocalSettings.php

The point of this task is to remove tests/phpunit/phpunit.php entirely. Can you try with vendor/bin/phpunit?

Is there a specific way to invoke this? Just using vendor/bin/phpunit doesn't work, it doesn't load the required classes. Passing suite.xml through --configuration shows me the message

You are running these tests directly from phpunit. You may not have all globals correctly set.
Running phpunit.php instead is recommended.#!/usr/bin/env php
A copy of your installation's LocalSettings.php

@Mainframe98 which test(s) are you trying to run? vendor/bin/phpunit tests/phpunit/{path-to-test-file-or-directory} should work, and it uses the phpunit.xml.dist in the root of the repository by default, not the suite.xml file that was used with the tests/phpunit/phpunit.php wrapper script.

@Mainframe98 which test(s) are you trying to run? vendor/bin/phpunit tests/phpunit/{path-to-test-file-or-directory} should work, and it uses the phpunit.xml.dist in the root of the repository by default, not the suite.xml file that was used with the tests/phpunit/phpunit.php wrapper script.

Odd. I updated MediaWiki to rMWda18f3eea7c2848e2a9e27aeb84e74bc3861ec60, and now it works. The unit test part at least - I can't run the integration part as MWMultiVersion interprets the path it is given as a database name, but that is a MWMultiVersion/Vagrant bug.

So, merging config files is not trivial; the main differences I could find:

  • Different bootstrap. I think this is also not trivial on its own, as unit and integration tests have different setups
  • failOnRisky is unset in the unit tests config (probably unintentional, can be added)
  • Unit tests have a memory limit of 512 MB. I think this can be set in the bootstrap file instead
  • Different suites, of course; I think merging all of them in a single file shouldn't cause too many issues
  • The configuration of addUncoveredFilesFromWhitelist is different, I haven't checked how we could merge them
  • The integration tests config uses two custom extensions, MediaWikiLoggerPHPUnitExtension and MediaWikiHooksPHPUnitExtension. The latter is going to be removed anyway (r673705); the former might be added to the unit xml file, but I don't know if it would be helpful.

So perhaps we might start by having different composer commands and keep two separate files until we can improve this.

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

[mediawiki/core@master] Add a composer command for the custom PHPUnit entry point

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

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

[mediawiki/core@master] [WIP] Delete the custom phpunit.php entry point

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

The point of this task is to remove tests/phpunit/phpunit.php entirely. Can you try with vendor/bin/phpunit?

$ PHPUNIT_WIKI=wiki php /vagrant/mediawiki/vendor/bin/phpunit /vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/
PHP Fatal error:  Class 'ApiTestCase' not found in /vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/integration/ApiHelpPanelQuestionPosterTest.php on line 15

Also

$ PHPUNIT_WIKI=wiki php /vagrant/mediawiki/tests/phpunit/phpunit.php /vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/
Using PHP 7.2.31-1+0~20200514.41+debian9~1.gbpe2a56b+wmf1
PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

[863175c0668e15457765b528] [no req]   Wikimedia\Rdbms\DBConnectionError: Cannot access the database: Unknown database '/vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/' (127.0.0.1) (127.0.0.1)
In T90875#7105008, @Tgr wrote:
$ PHPUNIT_WIKI=wiki php /vagrant/mediawiki/tests/phpunit/phpunit.php /vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/
Using PHP 7.2.31-1+0~20200514.41+debian9~1.gbpe2a56b+wmf1
PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

[863175c0668e15457765b528] [no req]   Wikimedia\Rdbms\DBConnectionError: Cannot access the database: Unknown database '/vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/' (127.0.0.1) (127.0.0.1)

This one might be caused by the change to the DB-related options, see r673069.

The immediate cause is that it tries to use the first parameter as the wiki name. Not sure why, I skimmed the code and nothing seemed to do that.

That is because of MWMultiVersion. Vagrant's CommonSettings sets $wgDBname to what was determined by MWMultiVersion, which for PHPUnit tests sets this to either --wiki, or the first argument given to the script ($argv). It should be updated to use the environment variable.

A quick update. I tried to migrate our custom script to a bootstrap file. This is currently very hard because Setup.php, LocalSettings etc. all use global state a lot. They define global variables, alter them, require more files that alter these variables etc. PHPUnit's bootstrap system, however, includes the bootstrap file from function scope (FileLoader::load), so these variables are not available globally during the bootstrap process. Then PHPUnit will build a list of variables that were created within the bootstrap file and import them into the global scope, but this happens too late for us. Some of the issues can be fixed by manually importing some variables into the global scope if we already know that a file is going to use them, but it's not always possible (e.g. Setup.php declares wgAutoloadClasses and then TestsAutoloader alters it); also, this would break very easily if the process of setting any global is changed.

I think it would be better if we could first improve this global mess in the various setup files, which is probably a good idea regardless of PHPUnit.

Change 693614 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/vagrant@master] Allow wiki ID to be set as an env var for maintenance scripts

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

The patch fixes the DB selection issue. Class autoloading for extensions still seems broken.

In T90875#7105008, @Tgr wrote:
$ PHPUNIT_WIKI=wiki php /vagrant/mediawiki/vendor/bin/phpunit /vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/
PHP Fatal error:  Class 'ApiTestCase' not found in /vagrant/mediawiki/extensions/GrowthExperiments/tests/phpunit/integration/ApiHelpPanelQuestionPosterTest.php on line 15

Weird, seems like the test autoloader is not invoked? The bootstrap file (in this case, tests/phpunit/bootstrap.php) loads it though, so I'm not sure why you're seeing this failure. Could you please verify that the bootstrap file is being executed, and whether something is preventing the test autoloader from being executed, too?

(BTW, apologies if I can't be of much help, but I've never used vagrant)

It turned out to be mostly a PEBKAC issue: phpunit.php could be invoked from anywhere but plain phpunit looks for the suite in the current directory, so you need to cd /vagrant/mediawiki first.

Change 693614 merged by jenkins-bot:

[mediawiki/vagrant@master] Allow wiki ID to be set as an env var for maintenance scripts

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

Change 693561 merged by jenkins-bot:

[mediawiki/core@master] Add a composer command for the custom PHPUnit entry point

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

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

[mediawiki/core@REL1_36] Add a composer command for the custom PHPUnit entry point

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

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

[mediawiki/core@REL1_35] Add a composer command for the custom PHPUnit entry point

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

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

[mediawiki/core@REL1_31] Add a composer command for the custom PHPUnit entry point

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

Change 692372 merged by jenkins-bot:

[integration/quibble@master] phpunit: Use composer phpunit:entrypoint

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

Change 704936 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/quibble@master] Release Quibble 1.0.0

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

Change 704936 merged by jenkins-bot:

[integration/quibble@master] Release Quibble 1.0.0

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