Page MenuHomePhabricator
Create Task
Maniphest T90875

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

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

Details

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
kostajh added a comment.Jul 2 2019, 8:42 PM

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.

Samwilson added a subscriber: Samwilson.Aug 27 2019, 5:19 AM
gerritbot added a comment.Dec 27 2019, 10:55 AM

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

Mainframe98 added a subscriber: Mainframe98.Dec 27 2019, 10:58 AM

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.

Daimona added a subscriber: Daimona.Jan 21 2020, 3:24 PM
gerritbot added a comment.Apr 12 2020, 6:41 PM

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

gerritbot added a comment.Jun 29 2020, 10:08 AM

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

Maintenance_bot removed a project: Patch-For-Review.Jul 1 2020, 8:23 PM
Daimona added a comment.Mar 15 2021, 5:40 PM

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).

In T90875#5301912, @kostajh wrote:

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.

Krinkle added a subscriber: aaron.EditedMar 15 2021, 5:43 PM
In T90875#6914879, @Daimona wrote:
In T90875#5301912, @kostajh wrote:

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).

Daimona added a comment.Mar 15 2021, 7:16 PM
In T90875#6914892, @Krinkle wrote:
In T90875#6914879, @Daimona wrote:
In T90875#5301912, @kostajh wrote:

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.

kostajh added a comment.Mar 15 2021, 8:39 PM
In T90875#6915322, @Daimona wrote:
In T90875#6914892, @Krinkle wrote:
In T90875#6914879, @Daimona wrote:
In T90875#5301912, @kostajh wrote:

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.

gerritbot added a comment.Mar 16 2021, 5:07 PM

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

gerritbot added a project: Patch-For-Review.Mar 16 2021, 5:07 PM
gerritbot added a comment.Mar 16 2021, 6:01 PM

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

Daimona added a comment.Mar 16 2021, 6:07 PM

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
gerritbot added a comment.Mar 16 2021, 6:17 PM

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

Mainframe98 added a comment.Mar 16 2021, 6:23 PM
In T90875#6918669, @Daimona wrote:

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.

Jdforrester-WMF added a comment.Mar 16 2021, 6:57 PM
In T90875#6918669, @Daimona wrote:

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.

Daimona added a comment.Mar 16 2021, 7:35 PM
In T90875#6918766, @Mainframe98 wrote:
In T90875#6918669, @Daimona wrote:

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?

In T90875#6918886, @Jdforrester-WMF wrote:
  • 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.

Daimona added a comment.Mar 16 2021, 7:36 PM
In T90875#6918989, @Daimona wrote:

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.

Jdforrester-WMF added a comment.Mar 16 2021, 7:38 PM
In T90875#6918990, @Daimona wrote:
In T90875#6918989, @Daimona wrote:

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.

Mainframe98 added a comment.Mar 16 2021, 7:40 PM
In T90875#6918989, @Daimona wrote:
In T90875#6918766, @Mainframe98 wrote:

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.

Jdforrester-WMF added a comment.Mar 16 2021, 7:49 PM
In T90875#6918989, @Daimona wrote:
In T90875#6918886, @Jdforrester-WMF wrote:
In T90875#6918669, @Daimona wrote:
  • 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.

Krinkle added a comment.Mar 16 2021, 8:24 PM
In T90875#6918989, @Daimona wrote:

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.

Daimona added a comment.Mar 17 2021, 11:48 AM
In T90875#6918991, @Jdforrester-WMF wrote:

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?

In T90875#6919273, @Krinkle wrote:

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';
}
gerritbot added a comment.Mar 17 2021, 5:59 PM

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

gerritbot added a comment.Mar 17 2021, 6:20 PM

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

Daimona mentioned this in T243600: Preparation for the PHPUnit 9 upgrade.Mar 22 2021, 1:54 PM
gerritbot added a comment.Mar 26 2021, 5:25 PM

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

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.37; 2021-03-30).Mar 26 2021, 6:00 PM
gerritbot added a comment.Apr 6 2021, 10:01 PM

Change 672762 merged by jenkins-bot:

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

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.1; 2021-04-13).Apr 6 2021, 11:00 PM
Krinkle edited projects, added Performance-Team; removed MW-1.37-notes (1.37.0-wmf.1; 2021-04-13), MW-1.36-notes (1.36.0-wmf.37; 2021-03-30), Wikimedia-Hackathon-2019.Apr 13 2021, 1:04 AM

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?

aaron added a comment.Apr 14 2021, 9:50 PM

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.

Daimona added a comment.Apr 15 2021, 11:10 AM
In T90875#7000788, @aaron wrote:

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?

gerritbot added a comment.Apr 17 2021, 11:03 AM

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

aaron added a comment.Apr 19 2021, 6:33 PM

Using env vars for now seems OK for now.

• Gilles moved this task from Inbox to Radar on the Performance-Team board.Apr 20 2021, 6:51 PM
• Gilles edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.Apr 20 2021, 8:05 PM
Daimona mentioned this in T280780: "Language::__destruct() was not expected to be called." failure due to garbage collector brings PHPUnit down.Apr 22 2021, 1:12 PM
Daimona added a comment.Apr 22 2021, 5:06 PM

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
gerritbot added a comment.May 17 2021, 7:15 PM

Change 673069 merged by jenkins-bot:

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

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

gerritbot added a comment.May 17 2021, 7:27 PM

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.6; 2021-05-18).May 17 2021, 8:00 PM
gerritbot added a comment.May 19 2021, 12:31 AM

Change 673074 merged by jenkins-bot:

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

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

Daimona updated the task description. (Show Details)May 19 2021, 4:45 PM
Jdforrester-WMF added a comment.May 19 2021, 7:38 PM

See T283146: Remove --reuse-db option from phpunit.php for one immediate removal.

kostajh added a comment.May 19 2021, 7:57 PM
In T90875#7027935, @Daimona wrote:

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.

TJones added a subscriber: TJones.May 19 2021, 10:43 PM

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.
kostajh added a comment.May 20 2021, 7:41 AM
In T90875#7099558, @TJones wrote:
  • 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

Daimona added a comment.May 20 2021, 8:03 AM
In T90875#7099308, @kostajh wrote:

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.

In T90875#7099558, @TJones wrote:

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.

Mainframe98 added a comment.May 20 2021, 8:58 AM
In T90875#7100088, @Daimona wrote:
In T90875#7099558, @TJones wrote:

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.

Daimona added a comment.May 20 2021, 11:59 AM
In T90875#7100147, @Mainframe98 wrote:

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.

Mainframe98 added a comment.May 20 2021, 12:08 PM
In T90875#7100494, @Daimona wrote:
In T90875#7100147, @Mainframe98 wrote:

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.

kostajh added a comment.May 20 2021, 12:14 PM
In T90875#7100504, @Mainframe98 wrote:
In T90875#7100494, @Daimona wrote:
In T90875#7100147, @Mainframe98 wrote:

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?

Mainframe98 added a comment.May 21 2021, 7:08 AM
In T90875#7100511, @kostajh wrote:

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
kostajh added a comment.May 21 2021, 11:35 AM
In T90875#7102530, @Mainframe98 wrote:
In T90875#7100511, @kostajh wrote:

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.

TJones removed a subscriber: TJones.May 21 2021, 3:12 PM
Mainframe98 added a comment.May 22 2021, 10:05 AM
In T90875#7103078, @kostajh wrote:

@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.

Addshore mentioned this in rMTCLe7f91c0a91b8: mwdd: Fix phpunit running & output (no --wiki).May 22 2021, 11:28 AM
Daimona added a comment.May 22 2021, 1:21 PM

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.

gerritbot added a comment.May 22 2021, 1:28 PM

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

gerritbot added a comment.May 22 2021, 1:52 PM

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

Tgr added a subscriber: Tgr.May 22 2021, 2:51 PM
In T90875#7100511, @kostajh wrote:

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)
Daimona added a comment.May 22 2021, 4:35 PM
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.

Tgr added a comment.May 22 2021, 5:04 PM

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.

Mainframe98 added a comment.May 22 2021, 5:12 PM

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.

Daimona added a comment.May 23 2021, 12:17 PM

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.

gerritbot added a comment.May 23 2021, 5:02 PM

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

Tgr added a comment.May 23 2021, 5:06 PM

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

Daimona added a comment.May 23 2021, 6:10 PM
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)

Tgr added a comment.May 23 2021, 10:07 PM

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.

gerritbot added a comment.Jun 1 2021, 9:57 PM

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

gerritbot added a comment.Jun 7 2021, 7:36 AM

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

ReleaseTaggerBot edited projects, added MW-1.37-notes (1.37.0-wmf.9; 2021-06-07); removed MW-1.37-notes (1.37.0-wmf.6; 2021-05-18).Jun 7 2021, 8:00 AM
gerritbot added a comment.Jul 2 2021, 11:12 AM

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

gerritbot added a comment.Jul 2 2021, 11:12 AM

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

gerritbot added a comment.Jul 2 2021, 11:13 AM

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

gerritbot added a comment.Jul 15 2021, 1:36 PM

Change 692372 merged by jenkins-bot:

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

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

gerritbot added a comment.Jul 16 2021, 10:27 AM

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

gerritbot added a comment.Jul 16 2021, 3:17 PM

Change 704936 merged by jenkins-bot:

[integration/quibble@master] Release Quibble 1.0.0

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

Stashbot mentioned this in T218534: Run Parsoid service in quibble.Jul 16 2021, 5:12 PM
Stashbot mentioned this in T286187: Release Quibble 1.0.0.
Stashbot mentioned this in T280506: Is it intended Quibble does not install the dependencies of the installed skins?.

Mentioned in SAL (#wikimedia-releng) [2021-07-16T17:12:11Z] <hashar> Tag Quibble 1.0.0 @ a13d133f7d1 # T286187 T280506 T90875 T218534 T227352

Stashbot mentioned this in T227352: Set up extension tests for Parsoid repo.Jul 16 2021, 5:12 PM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL