Page MenuHomePhabricator

Remove phpunit/phpunit dependencies from extensions's composer.json
Closed, ResolvedPublic

Description

Extensions should not be depending upon phpunit/phpunit, instead they should use the version provided by MediaWiki core. While we generally try and keep versions of dependencies in extensions' composer.json, phpunit is difference since it is closely tied to MediaWiki core with stuff like MediaWikiTestCase, the phpunit.php wrapper, and so on., so we keep it centrally managed.

https://codesearch.wmflabs.org/things/?q=phpunit%2Fphpunit&i=nope&files=composer.json&repos=

(MediaWikiFarm does things very differently and should probably be skipped for the purposes of this task)

Patches cleaning this up: https://gerrit.wikimedia.org/r/#/q/topic:rm-phpunit

There was also some discussion on https://gerrit.wikimedia.org/r/#/c/411553/ about how this needed a Phabricator ticket.

Event Timeline

Tests in the WikibaseMediaInfo as well as WikibaseLexeme extensions are intentionally split into two tests/phpunit/mediawiki/ and /tests/phpunit/composer/ directories. The split is a little arbitrary, but allows us to a) identify code that is already independent from MediaWiki, and b) guarantee it stays independent. This will become helpful later when the code bases became more stable, and the teams working on them want to split them into smaller, reusable components. Having classes that are candidates for separate components in their own directory and being able to run their tests separately allows the teams to easily identify possible components that might be worth splitting off. These tests also run much faster.

I created T188011: [Task] Use PHPUnit 6 forward compatibility aliases in all Wikibase code bases to ease the migration to PHPUnit 6. But unfortunately it is not always possible to specify ^4.8.35 || ^6 as a forwards-compatible dependency, because the two versions are incompatible for many more reasons. For example, setExpectedException() as well as getMock() got renamed. What this effectively means is that even when we remove the current PHPUnit dependency from the extensions composer.json, this will not magically make them more compatible with PHPUnit 6. The extensions need an update anyway, and it will be quite big (for example, Wikibase uses getMock() about a thousand times). The composer.json line challenged in this ticket here can easily be updated in these future patches. Only then the tests can be run with PHPUnit 6.

Therefore I suggest to close this ticket as invalid because it does not solve an issue we actually have (or fails to explain it).

Is that the same reason to have wikimedia/testing-access-wrapper in composer.json for the PropertySuggester extension?
When an extension does not work without mediawiki/core it should not define own dependencies, which are already part of mediawiki/core. When it works without core, than it is named wrong.

Also use of wikimedia/assert in WikibaseLexeme can conflict with mediawiki/core on new version in mediawiki/core
Or wikimedia/purtle in Wikibase

Tests in the WikibaseMediaInfo as well as WikibaseLexeme extensions are intentionally split into two tests/phpunit/mediawiki/ and /tests/phpunit/composer/ directories. The split is a little arbitrary, but allows us to a) identify code that is already independent from MediaWiki, and b) guarantee it stays independent. This will become helpful later when the code bases became more stable, and the teams working on them want to split them into smaller, reusable components. Having classes that are candidates for separate components in their own directory and being able to run their tests separately allows the teams to easily identify possible components that might be worth splitting off. These tests also run much faster.

Keeping the tests different is totally fine, MediaWiki will run anything in tests/phpunit. Once the team wants to split them into a separate library, they can do so, and set up the separate phpunit runner at that time using the normal library bootstrap.

I created T188011: [Task] Use PHPUnit 6 forward compatibility aliases in all Wikibase code bases to ease the migration to PHPUnit 6. But unfortunately it is not always possible to specify ^4.8.35 || ^6 as a forwards-compatible dependency, because the two versions are incompatible for many more reasons. For example, setExpectedException() as well as getMock() got renamed. What this effectively means is that even when we remove the current PHPUnit dependency from the extensions composer.json, this will not magically make them more compatible with PHPUnit 6. The extensions need an update anyway, and it will be quite big (for example, Wikibase uses getMock() about a thousand times). The composer.json line challenged in this ticket here can easily be updated in these future patches. Only then the tests can be run with PHPUnit 6.

MediaWiki is going to provide a forwards and backwards-compatibility layer for most of the breakage to make the dual support possible, so it should be possible to run tests written for PHPUnit 4 under PHPUnit 6. Once we've switched over and dropped PHPUnit 4 support, we can slowly deprecate the back-compat we added in for 4 and update extensions.

Therefore I suggest to close this ticket as invalid because it does not solve an issue we actually have (or fails to explain it).

The point general point that I'm trying to make, and what @Umherirrender mentions too, is that these extensions shouldn't/don't depend on a specific version of PHPUnit, they depend upon MediaWiki core, which provides PHPUnit as its dependency, that all extensions should assume core will provide. Just like you don't depend on any of the other libraries that MediaWiki core provides, you just assume they'll be there.

When an extension does not work without mediawiki/core […] When it works without core […]

I'm afraid I did not made my point clear enough. This is not an "either-or" thing. The code bases I talk about are extensions, but contain code that is intentionally independent from MediaWiki core. The teams wish to run these tests independently from core is nothing that is set in stone, and will totally be disputed and changed the moment it creates actual problems. I just want us to a) have a migration path first, and b) let the responsible teams decide before removing code that currently does no actual harm.

Thanks for the reminder for the Assert and Purtle dependencies. We definitely should look into these too.

MediaWiki is going to provide a forwards and backwards-compatibility layer for […] dual support […]

Thats awesome! Thank you. Will this be provided via \MediaWikiTestCase only, or also work for tests that directly extend \PHPUnit\Framework\TestCase?

Keeping the tests different is totally fine, MediaWiki will run anything in tests/phpunit. Once the team wants to split them into a separate library, they can do so, and set up the separate phpunit runner at that time using the normal library bootstrap.

Indeed. And this is what I am expecting us to do at some point.

When an extension does not work without mediawiki/core […] When it works without core […]

I'm afraid I did not made my point clear enough. This is not an "either-or" thing. The code bases I talk about are extensions, but contain code that is intentionally independent from MediaWiki core. The teams wish to run these tests independently from core is nothing that is set in stone, and will totally be disputed and changed the moment it creates actual problems. I just want us to a) have a migration path first, and b) let the responsible teams decide before removing code that currently does no actual harm.

I understand the advantage of keeping the tests separate on the filesystem, but I don't really see much substantial benefit from *running* them separately. We're already running them as part of MediaWiki test suite anyways.

This ticket is about the actual problem it's creating...it's preventing us from upgrading to PHPUnit 6, and will continue to cause problems as we increase the PHPUnit version we use.

MediaWiki is going to provide a forwards and backwards-compatibility layer for […] dual support […]

Thats awesome! Thank you. Will this be provided via \MediaWikiTestCase only, or also work for tests that directly extend \PHPUnit\Framework\TestCase?

getMock()/setExpectedException() are provided via a trait that MediaWikiTestCase will use by default, but any class can use independent of MediaWikiTestCase (similar to the MediaWikiCoversValidator trait) There's also going to be an autoloader to class_alias old non-namespaced names, which will be invoked as long as you use the MediaWiki test runner. See https://gerrit.wikimedia.org/r/#/c/394851/ for the details.

but I don't really see much substantial benefit from *running* them separately.

Running those tests without relying on MediaWiki is pretty helpful when test-driving development of some code there that is MW-independent. That said, it would have been even easier if that code was in the separate code base.
As currently the split in WikibaseLexeme.git between stuff which is the actually MW-related, and the "generic" one is really not clear. Therefore I see it is pretty much as confusing as helpful.

So if you guys consider it makes the PHPUnit update etc easier for you, removing the phpunit dep from composer.json of WikibaseLexeme seems like something we could live for now :)

Change 424846 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/WikibaseLexeme@master] Remove phpunit/phpunit dependency

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

Change 424847 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/WikibaseMediaInfo@master] Remove phpunit/phpunit dependency

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

Change 424846 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Remove phpunit/phpunit dependency

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

Change 424847 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Remove phpunit/phpunit dependency

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

Legoktm claimed this task.