Page MenuHomePhabricator

Figure out a system to override default settings when in test context
Closed, DeclinedPublic

Description


TL:DR; I would like a system that would let extensions provide different settings when run in a test context.


The mediawiki-extensions-hhvm job (which is triggered by multiple repositories) has been broken because the SpamBlacklist extension points by default to meta.wikimedia.org which was unreachable (T89052). From extensions/SpamBlackList.php:

$wgBlacklistSettings = array(
    'spam' => array(
        'files' => array( "https://meta.wikimedia.org/w/index.php?title=Spam_blacklist&action=raw&sb_ver=1" )
    )
);

In a test context, the SpamBlacklist would want to set $wgBlacklistSettings = array();.

Another example is ConfirmEdit which enables captcha by default, but that has side effects on tests from other extensions ( T44145 ). @Florian proposed https://gerrit.wikimedia.org/r/#/c/182480/ which turns $wgCaptchaTriggers false. His patch reuses a hack we introduced for the Wikibase extension: Jenkins injects a specific global variable when preparing the environment to test, that let extensions developers reacts differently:

# Injected by Jenkins:
$wgWikimediaJenkinsCI = true;

# Extension
if ( isset( $wgWikimediaJenkinsCI ) && $wgWikimediaJenkinsCI ) {
  // some hacks and conf changes
}

I don't really like this hack, it is asking for more cruft and tech debt to pill up soonish.

We could get either:

  1. a new hook UnitTestsSettings in tests/phpunit/MediaWikiTestCase.php, to let extensions tweak their settings.
  2. a well supported variable $wgTestsSettings , we would use keys as global name to override entries in $_GLOBAL.

Thoughts ideas?

Related Objects

Event Timeline

hashar created this task.Feb 10 2015, 10:03 AM
hashar raised the priority of this task from to Needs Triage.
hashar updated the task description. (Show Details)
hashar added subscribers: hashar, Florian, Addshore, JeroenDeDauw.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 10 2015, 10:03 AM
Florian added a comment.EditedFeb 10 2015, 11:34 AM

+1 for a new hook, thanks for the task @hashar. But the documentation should be really clear, that this hook should be used very rarely and if there is really really no other solution to solve the problem, otherwise we will break our whole testing, if no-one knows anymore when what code is executed where :)

Sorry for being pessimistic, but I'm challenging this to draw out more use cases. As it stands, I don't think we need a system for this.

A project's own tests are responsible for setting up relevant configuration variables (using a LocalSettings appendix, TestCase::setMwGlobal, or mw.config.set for PHPUnit and QUnit respectively). With the added benefit of being inside the test file and being specific to each test. If a project finds they're duplicating a lot of settings, a subclass of MediaWikiTestCase is perfect for this.

Projects depending on other projects should mock/isolate parts accordingly. This seems standard testing practice.

I think such system would be useful, but I'm worried it may be disproportionately prioritised. Or it might distract us from the real problem (needing to find those variables and set them correctly), which having a system isn't going to solve. When people identify the relevant those variables, they might as well set them in PHPUnit setUp directly. It's the same amount of work to call setMwGlobal in setUp as specifying values in this system would be. Or is there an additional benefit?

Do you have an idea how to solve the ConfirmEdit example for when it is installed and the integration tests of another extension are run?

JanZerebecki triaged this task as High priority.Apr 28 2015, 2:37 PM
JanZerebecki set Security to None.

Right now we need this for MobileFrontend and Gather's JJB job for running browser tests per commit.

There are two browser tests that need LocalSettings

  • One browser test runs in MobileFrontend's beta mode, which is only available when $wgMFEnableBeta = true (it defaults to false)
  • Another browser test checks for the existence of a license in the footer. This is important but requires a license to be set like so:
	$wgRightsText = 'Creative Commons Attribution 3.0';
	$wgRightsUrl = "http://creativecommons.org/licenses/by-sa/3.0/";

I think these 2 examples are a clear example of why these might be needed for integration tests.

I agree with @Krinkle's comments regarding the proposed system in the context of unit testing: it doesn't seem necessary as you can just as easily (and with more explicitness) mock/override global settings/state.

Integration/end-to-end (browser) tests, however, are inherently unable to manipulate the application state beyond what is mutable via the UI and API, include MW settings. A simple system for ensuring global settings would help satisfy preconditions for the test cases that @Jdlrobson and @JanZerebecki mentioned.

I'm not sure I like the $wgWikimediaJenkinsCI flag approach, however. For pre-merge CI browser tests where the MW wiki is already being provisioned locally, it might be simpler to just look for an appendix to LocalSettings.php at e.g. tests/browser/LocalSettings.php.

Change 232224 had a related patch set uploaded (by Dduvall):
Close PHP tag when appending LocalSettings.php

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

phuedx added a subscriber: phuedx.Aug 18 2015, 11:22 AM

+1 to @dduvall's suggestion of appending tests/browser/LocalSettings.php to LocalSettings.php. It's simple, powerful, and familiar.

Change 232399 had a related patch set uploaded (by Dduvall):
Append LocalSettings.php for mw-selenium jobs

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

When people are running tests locally, are they also expected to include tests/browser/LocalSettings.php? Will that happen automatically for them? We've been moving in the direction of making our tests run exactly the same in CI as they do on a developer's laptop (e.g. composer test/npm test)...does this method work with that?

dduvall added a comment.EditedAug 19 2015, 5:08 PM

When people are running tests locally, are they also expected to include tests/browser/LocalSettings.php? Will that happen automatically for them? We've been moving in the direction of making our tests run exactly the same in CI as they do on a developer's laptop (e.g. composer test/npm test)...does this method work with that?

It wouldn't, no—I can't think of a way to cleanly implement that behavior locally. I would suggest that anything added to tests/browser/LocalSettings.php also be paired with an assertion of the resulting precondition in tests. For example, if you're enabling beta features, explicitly assert in dependent tests that they are in fact enabled, and override the failure message to make it clear that it's an unsatisfied precondition.

Given(/^beta features are enabled for this wiki$/) do
  visit(MainPage) do |page|
    expect(page.some_manifestation_of_beta_features).to be_visible, "beta features do not appear to be enabled"
  end
end

We already have this situation. We have a README that explains certain config options need to be set in LocalSettings.php before the tests can work so they can be made to run locally. The LocalSettings doesn't interfere with this.

When people are running tests locally, are they also expected to include tests/browser/LocalSettings.php? Will that happen automatically for them? We've been moving in the direction of making our tests run exactly the same in CI as they do on a developer's laptop (e.g. composer test/npm test)...does this method work with that?

It wouldn't, no—I can't think of a way to cleanly implement that behavior locally.

It could be done in MediaWiki-Vagrant, which is the next best thing.

Not everyone uses (or want to use) vagrant, so there should be a solution for these people, too, even if it would be a bit more complicated.

Not everyone uses (or want to use) vagrant, so there should be a solution for these people, too, even if it would be a bit more complicated.

Using a readme like we currently do. You can always tell the user to manually append the LocalSettings in browser folder as part of setup. Thus this is better than the status quo.

Is this stalled or are we gonna give it a go?

To respond to the comment of @Jdlrobson:
Even if it would be nice to have an automatic solution (without vagrant), I'm fine, if it works with a manual adjustment of LocalSettings.php :)

Change 232224 merged by jenkins-bot:
Support additional LocalSettings.php for mw-selenium jobs

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

Change 232399 merged by jenkins-bot:
Append LocalSettings.php for mw-selenium jobs

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

hashar added a subscriber: zeljkofilipin.

So looks like this has been implemented a year ago via rCIJE377e45b17decdc78e2bfa80fd541cf05b9625779

# Append the MW installation's LocalSettings.php with the contents of
# tests/browser/LocalSettings.php. Note that this setup script requires that
# the current working directory already be the appropriate `tests/browser`
# directory.
if [ -f LocalSettings.php ]; then
  cat LocalSettings.php >> "$MW_INSTALL_PATH/LocalSettings.php"
fi

Namely once the Jenkins job has changed dir to tests/browser if a LocalSettings.php file is present its content is injected. So implementation is done.

An example of such usage is MobileFrontend commit 47e5fc6cc9d9a6fbf7f4e5b78455836366db2a93


@zeljkofilipin is there a place where we can document injection of tests/browser/LocalSettings.php? I looked up a bit on mediawiki.org but could not find any obvious place.

In context of browser tests? Some of the pages from Run tests section?

https://www.mediawiki.org/wiki/Selenium

zeljkofilipin closed this task as Declined.Nov 2 2017, 5:37 PM

Unlikely to ever be resolved because of T139740: Port Selenium tests from Ruby to Node.js.