T278003: Running tests on a clean vagrant instance fails is related, but for MediaWiki-Vagrant in particular. One thing I found while investigating for that task was that certain tests fail when $wgUseInstantCommons is enabled, as they now try to access Commons, which is prohibited by NullHttpRequestFactory.

We already have a TestSetup.php file, where many configuration settings are replaced with defaults or values similar to defaults, and this is enforced the MediaWikiIntegrationTestCase via PHPUnit.

We could play whack-a-mole and gradually end up copying almost every setting from DefaultSettings to TestSetup.php, but maybe we can instead ask the question:

What settings from LocalSettings.php do we actually want to apply under test?

As a strawman answer, I pose there are only two things:

• Extension state (which extensions to load).
• Database settings (faccilitate integration tests).

I additionally pose that the objective of local testing is to match a (subset) of CI.

If these three statements are correct, I believe it follows then that there can't be other settings one would "want" or "need" during a test. It seems by design that any other config setting naturally falls into one of these three categories:

• A non-default config is needed for a test to pass pass. Shared need, thus must be set in the test class directly, as enforced by CI which would otherwise fail.
• A personal override exists locally, and it does not affect the test. Neutral either way. Doesn't matter whether PHPUnit uses the MainConfigSchema value, your value, or gets overridden by TestSetup.php.
• A personal override exists and it makes a test fail. You'll either end up hardcoding the working default in either TestSetup.php or the test class so that your tests pass locally.

Tentatively proposing one of these two approaches to consider:

1. Low risk. We take TestSetup to its natural conclusion and automatically reflect a copy of MainConfigSchema for all non-db settings.
2. Higher risk but maybe cleaner. We update the phpunit runner to load LocalSettings.php via a Setup.php callback instead of the normal way. The callback uses $wgSettings or LocalSettingsLoader to essentially require LocalSettings.php but discard all but a handful of its assignments.

In T277470#9405517, @Krinkle wrote:

• Extension state (which extensions to load).

Yes, I think this is the most important setting we really need. And also the main motivation behind r938244.

• Database settings (faccilitate integration tests).

Sounds good. And in general, I agree that this is the right approach: begin with just extensions and DB settings, and consider adding things if really needed, rather than starting with all settings and removing things that are not needed.

2. Higher risk but maybe cleaner. We update the phpunit runner to load LocalSettings.php via a Setup.php callback instead of the normal way. The callback uses $wgSettings or LocalSettingsLoader to essentially require LocalSettings.php but discard all but a handful of its assignments.

This is similar to the approach I've used for bootstrap.php, which shells out to the getPHPUnitExtensionsAndSkins.php script in order to get a list of extensions to load while avoiding any side effects from LS.php.

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

[mediawiki/core@master] phpunit: Fix tests relying on implicit wgScript/wgArticlePath

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

Change #967324 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Fix tests relying on implicit wgScript/wgArticlePath

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

Another example I found is AvailableRightsTest. In my LocalSettings, I have some $wgGroupPermissions assignments for extension rights; I used to do these unconditionally, including if the extension in question isn't loaded (wfLoadExtension commented out), for convenience. This causes test failures because Additional user rights need to be added to $wgAvailableRights or via the "UserGetAllRights" hook.

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

[mediawiki/core@master] tests: Fix tests implicitly relying on config

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

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

[mediawiki/core@master] phpunit: Ensure wgCentralIdLookupProvider defaults to "local"

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

Change #1149694 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Ensure wgCentralIdLookupProvider defaults to "local"

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

Change #1151243 had a related patch set uploaded (by Krinkle; author: Daimona Eaytoy):

[mediawiki/core@master] phpunit: Delay initial TestSetup config to after extension registration

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

Change #1151723 had a related patch set uploaded (by Krinkle; author: Daimona Eaytoy):

[mediawiki/core@REL1_44] phpunit: Delay initial TestSetup config to after extension registration

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

Change #1151723 merged by jenkins-bot:

[mediawiki/core@REL1_44] phpunit: Delay initial TestSetup config to after extension registration

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

Change #1151243 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Delay initial TestSetup config to after extension registration

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

Change #1141974 merged by jenkins-bot:

[mediawiki/core@master] tests: Fix tests implicitly relying on config

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

r1151243 had to be reverted due to T395483. On second thought, it makes sense that we cannot forcefully override some extension-overridden settings, because they might be needed for that extension to work properly. For example, if a test relies on CentralAuth, we need to let CentralAuth setup its auth stack.

@Daimona It failed only on REL1_44, not master. This is because I forgot to backport https://gerrit.wikimedia.org/r/1149694 and https://gerrit.wikimedia.org/r/1139822.

It passing on master is a very strong signal that there are not common cases where such implicit dependencies exist. There was one previous-case we know of that existing (which was identified and fixed beforehand in master) which is CentralAuth.

Today seems like a good time to enforce this, rather than allow it to regress again.

In summary: In MediaWiki core, we use TestSetup to set configuration in Config that PHPUnit effectively restores at every setUp/tearDown, as a reliable starting point. Any configuration values not set by TestSetup are free game for extension hooks to change the defaults of, just like how any other MediaWiki core or extension configuration variable can change its default any time, and those will be reflected in tests.

The issue above is much narrower than it seems: It is specifically when there is a conflict between the defaults we configure for PHPUnit, and what an extension does via an early hook. Anything else they set is applied fine either way. We didn't disable that hook, only moved it so that PHPUnit's defaults reliably apply as a starting point. Individual tests should set any non-default settings they need, and it passing on the master branch tells me we're in a good position to enforce that.

I'm netural on whether we bother with it for REL1_44, it seemed worth a try, but I'm happy to let that go.

In T277470#10865562, @Krinkle wrote:

@Daimona It failed only on REL1_44, not master.

I don't think that's true. The original report is for https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ConfirmEdit/+/1151737 against master. I saw it on other repos, also against master.

Yeah, tests were failing on master.

It is specifically when there is a conflict between the defaults we configure for PHPUnit, and what an extension does via an early hook.

Nit: not an early hook, but extension.json. In practice it's basically the same though.

The thing is: for as much as I'd like to use stable and predictable config in tests, I'm not sure if that's something we can achieve for extension defaults. If an extension overrides a core setting's default, then presumably the overridden value is needed for the extension to work properly. Using CentralAuth as an example, the SessionProviders override (strictly speaking, array_merge) is a requirement. I'd assume that the extension isn't meant to operate in an environment where that default did not get applied, and whatever happen in that case is either undefined behaviour or crashes.

But perhaps the most egregious example is extensions defining handlers for core hooks. It is well known that they can cause unexpected test failures; I also fixed some in the above patch. But would it make sense to outright disable all extension hook handlers in tests? I think this would be problematic, especially because it isn't clear how to revert that for an individual test that needs a certain hook handler, without duplicating code. This also hints at another problem of the above approach. Again using the example of a test that was failing earlier today: a CheckUser test needs CentralAuth to be fully setup. How is that test in CheckUser meant to fully (re-)enable CentralAuth? If not by duplicating and hard-coding the CA SessionProviders override inside the CU test, which is undesirable for obvious reasons.

Maybe a more generalized idea would be to fully disable all extensions when running tests (effectively ignore wfLoadExtension calls), and have individual tests explicitly enable them. This would be an inversion of the current paradigm where we actively disable hook handlers where we don't want interferences. But, I'm not sure how to implement this properly, i.e., in a way that doesn't require people to write tons of boilerplate cruft. For example, I imagine that such a system would, when running any test defined in extension X, enable extension X and all of its hard dependencies, by default. But this is well beyond the scope of this task.

In T277470#10866290, @Daimona wrote:

[…] for as much as I'd like to use stable and predictable config in tests, I'm not sure if that's something we can achieve for extension defaults. If an extension overrides a core setting's default, then presumably the overridden value is needed for the extension to work properly.
[…] But perhaps the most egregious example is extensions defining handlers for core hooks. […]

Maybe a more generalized idea would be to fully disable all extensions […], and have individual tests explicitly enable them. […] But this is well beyond the scope of this task.

Agreed. That is an interesting future direction to explore, and may or may not be worth it. TBD.

For this task, I'd like to get back to the original scope, which upon reflection, seems compatible with everything we've learned so far?

The keypoint here is to remove sources of variability and works-for-me-not-for-you issues from LocalSettings.php. Default core settings, default extension settings, and whatever extensions do consistently and deterministically by default (through overrides, callbacks, hooks, etc) all seems fair game here.

In previous comments, we mentally approaching from the perspective of clearing all config state (thus losing what hooks did initially to override settings), and then loading only core+extension defaults + DB settings. That seems way too much indeed, and breaks all the above.

But.. we don't have to approach it that way. Can we approach it as-it-happens by not loading the settings we don't want, and then going forward as usual? Our PHPUnit setup already deals with restoring state to the initial snapshots, so we only need to focus on how we initialise our state. Rather than focussing on "un-doing" things long after the fact (e.g. during the first test), could can address it more locally between loading LocalSettings and the rest of init?

With our modern bootstrap.php file, I think we have the ability to do this at the "right" time, such that whatever other init logic happens on top of "the config state" can just happen. It'd be similar to where we apply TestSetup.php, which we appear to also apply between defaults+LocalSettings and runtime (e.g. hooks/overrides).

In T277470#11071209, @Krinkle wrote:

For this task, I'd like to get back to the original scope, which upon reflection, seems compatible with everything we've learned so far?

Yep, sorry about the OT :)

Can we approach it as-it-happens by not loading the settings we don't want, and then going forward as usual?

It might take a few hacks, but it should be doable. I suppose the solution today would look something like this:

• Snapshot the relevant state before loading LocalSettings.php
• require LS.php
• Restore previous snapshot except for things we want to keep

A few additional notes/caveats:

• What is "state" in the context of the above? Definitely global variables, but what else? For example, static singletons like SettingsBuilder? Also other setup hook points like MW_CONFIG_CALLBACK, MW_SETUP_CALLBACK, MW_FINAL_SETUP_CALLBACK?
• What do we want to keep? I agree with T277470#9405517 that DB settings and extensions/skins to load should be sufficient. Or at least I hope so. Maybe it's a case of "try and see".

One thing to note about extensions is that it would generally be nicer if we had a way to specify which extensions to load without involving LocalSettings. For example, an extension.yaml file. Reading extensions from LS.php is one of the two reasons why the getPHPUnitExtensionsAndSkins hack exists (the other reason being the UnitTestsList hook, T298509). A few years ago there was an attempt to migrate MW configuration to YAML, but it doesn't look like it's had much success, or follow-up work after the initial version. Maybe that could help if we made it the recommended/only way to specify MW configuration. (Assuming that dynamic config needs could be addressed with a config builder that runs outside of MW and generates a static config file). At any rate, this would be much more work.