Tagging Platform Engineering in case they have opinions on this.

I don't know how DonationInterface and Wikibase are using this hook, but Parsoid's use is pretty subtle and not obviously easily replaced: Parsoid is *both* a library *and* and extension, and we need to be able to squirrel away Parsoid's "standalone" tests (run by composer test without access to a MediaWiki instance) somewhere where core's default auto discovery of tests won't find it -- and similarly, we need to be able to write "extension" tests (which require access to a MediaWiki instance) in a way that the standard composer test won't find them.

This is pretty easy to do if extension discovery is done with executable code (or at least a hook to executable code). It seems quite challenging if you're using static phpunit.xml.dist files.

In my personal opinion, rMW1d7221d066c0 was a mistake: as a MediaWiki developer I have just about every extension every authored checked out in an extensions directory, and I enable them on demand via LocalSettings.php. But after rMWcf4654399bd3 phpunit completely ignores my LocalSettings.php and runs tests on every extension it finds in the directory, regardless of whether it is currently enabled/configured/set up, etc. So I have to resort to storing my extensions out of the tree and a hack in my LocalSettings.php that creates symlinks in my extensions/ directory to correspond to the current set of enabled extensions. That's annoying.

On the other hand, I suppose if extension configuration was done declaratively (outside of LocalSettings.php) I wouldn't be as opposed to test discovery using the same mechanism. As long as enabling/disabling an extension can be done in a single place. Blindly runnings tests on anything found in extensions/*/tests is a disaster, though.

In T298509#7609799, @cscott wrote:

In my personal opinion, rMW1d7221d066c0 was a mistake: as a MediaWiki developer I have just about every extension every authored checked out in an extensions directory, and I enable them on demand via LocalSettings.php. But after rMWcf4654399bd3 phpunit completely ignores my LocalSettings.php and runs tests on every extension it finds in the directory, regardless of whether it is currently enabled/configured/set up, etc. So I have to resort to storing my extensions out of the tree and a hack in my LocalSettings.php that creates symlinks in my extensions/ directory to correspond to the current set of enabled extensions. That's annoying. […]

I'm unable to reproduce this nor see how it could happen. I don't doubt you're experiencing it, but I am fairly confident that this is not in any way desirable or intended functionality and either result of a bug or an accident/misunderstanding in configuration.

The linked patch discovers tests/phpunit in enabled extensions. It does not discover extensions. It uses ExtensionRegistry, which iterates over enabled extensions at run-time in the same way as e.g. Special:Version does.

The only code I'm aware of that discovers uninstalled extensions, is the CLI installer where Antoine recently added a way for CI to quickly install MW with all extensions enabled that are present on disk, such that the logic for deciding which extensions to install in CI for a given repo is tied to which extension repos are cloned, and then MW takes it all. That's opt-in though, and afaik not something we expose to PHPUnit.

In T298509#7633606, @Krinkle wrote:

In T298509#7609799, @cscott wrote:

In my personal opinion, rMW1d7221d066c0 was a mistake: as a MediaWiki developer I have just about every extension every authored checked out in an extensions directory, and I enable them on demand via LocalSettings.php. But after rMWcf4654399bd3 phpunit completely ignores my LocalSettings.php and runs tests on every extension it finds in the directory, regardless of whether it is currently enabled/configured/set up, etc.

Can you please share what command(s) you are using to run the tests?

composer phpunit:unit will run unit (not integration) tests for core and all skins/extensions, regardless of whether they're enabled, but those tests don't rely on LocalSettings.php so I don't think that's what you're referring to?

In T298509#7633961, @kostajh wrote:

composer phpunit:unit will run unit (not integration) tests for core and all skins/extensions, regardless of whether they're enabled, but those tests don't rely on LocalSettings.php so I don't think that's what you're referring to?

They may not rely on LocalSettings.php but they do depend on composer update, references to deprecated methods, etc, so they will certainly fail even if LocalSettings.php isn't involved.

For example, composer phpunit:unit at the moment in my tree consisting of only WMF production extensions (but most of them disabled or unconfigured or old checkouts from the last time I had to patch them) fails with 582 errors, ending with:

582) Flow\Tests\Data\ManagerGroupTest::testCachePurgeCallsAppropriateManager
Error: Class 'Pimple\Container' not found

/home/cananian/Projects/Wikimedia/Flow/includes/Container.php:5

Yes, in theory I could write a script to git pull and composer update in each and every checked out extension (282 of them at the moment, and those are just extensions I've personally patched at one point or another), but the point is: phpunit shouldn't be looking at extensions I haven't actually enabled, just because they happen to be present on disk.

In any case, apart from my general griping, this is the actual issue:

In T298509#7609799, @cscott wrote:

I don't know how DonationInterface and Wikibase are using this hook, but Parsoid's use is pretty subtle and not obviously easily replaced: Parsoid is *both* a library *and* and extension, and we need to be able to squirrel away Parsoid's "standalone" tests (run by composer test without access to a MediaWiki instance) somewhere where core's default auto discovery of tests won't find it -- and similarly, we need to be able to write "extension" tests (which require access to a MediaWiki instance) in a way that the standard composer test won't find them.

This is pretty easy to do if extension discovery is done with executable code (or at least a hook to executable code). It seems quite challenging if you're using static phpunit.xml.dist files.

If you allowed extensions to declaratively customize the 'tests/phpunit' subpath, that would probably be sufficient. Parsoid's extension.json could specify that MediaWiki should run the tests in extension/tests/phpunit and that MediaWiki should ignore the tests in tests/phpunit.

In T298509#7640564, @cscott wrote:

In T298509#7633961, @kostajh wrote:

composer phpunit:unit will run unit (not integration) tests for core and all skins/extensions, regardless of whether they're enabled, but those tests don't rely on LocalSettings.php so I don't think that's what you're referring to?

They may not rely on LocalSettings.php but they do depend on composer update, references to deprecated methods, etc, so they will certainly fail even if LocalSettings.php isn't involved.

For example, composer phpunit:unit at the moment in my tree consisting of only WMF production extensions (but most of them disabled or unconfigured or old checkouts from the last time I had to patch them) fails with 582 errors, ending with:

582) Flow\Tests\Data\ManagerGroupTest::testCachePurgeCallsAppropriateManager
Error: Class 'Pimple\Container' not found

/home/cananian/Projects/Wikimedia/Flow/includes/Container.php:5

Yes, in theory I could write a script to git pull and composer update in each and every checked out extension (282 of them at the moment, and those are just extensions I've personally patched at one point or another), but the point is: phpunit shouldn't be looking at extensions I haven't actually enabled, just because they happen to be present on disk.

As an aside, you can use https://www.mediawiki.org/wiki/Composer#Using_composer-merge-plugin so that you just run composer update in the root of the repo, and the merge plugin will handle updates for any extensions/skins.

composer phpunit:unit by default runs unit tests across core/extensions/skins for convenience, but you could also just run vendor/bin/phpunit {pathToExtensionTests} {pathToAnotherExtensionTests} etc, and make an alias or something for convenience. We don't currently have a static listing of enabled extensions/skins, which would be nice IMO, but I don't think it is a good idea to require bootstrapping MediaWiki in order to tell it where tests are. That slows down the overall testing setup and also causes problems for e.g. data providers which don't have any access to the MW anyway in a vanilla PHPUnit setup.

In T298509#7640575, @cscott wrote:

In any case, apart from my general griping, this is the actual issue:

In T298509#7609799, @cscott wrote:

I don't know how DonationInterface and Wikibase are using this hook, but Parsoid's use is pretty subtle and not obviously easily replaced: Parsoid is *both* a library *and* and extension, and we need to be able to squirrel away Parsoid's "standalone" tests (run by composer test without access to a MediaWiki instance) somewhere where core's default auto discovery of tests won't find it -- and similarly, we need to be able to write "extension" tests (which require access to a MediaWiki instance) in a way that the standard composer test won't find them.

This is pretty easy to do if extension discovery is done with executable code (or at least a hook to executable code). It seems quite challenging if you're using static phpunit.xml.dist files.

If you allowed extensions to declaratively customize the 'tests/phpunit' subpath, that would probably be sufficient. Parsoid's extension.json could specify that MediaWiki should run the tests in extension/tests/phpunit and that MediaWiki should ignore the tests in tests/phpunit.

Maybe we could implement the workarounds in Quibble, since it sounds like this is a temporary situation for Parsoid anyway?

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

[mediawiki/extensions/DonationInterface@master] [WIP] Remove UnitTestsList hook

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

Change 761894 abandoned by Kosta Harlan:

[mediawiki/extensions/DonationInterface@master] [WIP] Remove UnitTestsList hook

Reason:

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

Note that the presence of this hook makes it quite difficult to have a standard PHPUnit setup (T227900, T90875), because we need to fully initialize MediaWiki in order to run the hook. For now I'm going to implement a workaround, but I'd really like to see this hook go away.