Page MenuHomePhabricator
Create Task
Maniphest T266441

MediaWikiUnitTestCase should prevent access to MediaWikiServices
Closed, ResolvedPublic
Actions

Description

MediaWikiUnitTestCase is intended to be used for "pure" unit tests, which mock dependencies in classes under test. To enforce that, we can block classes extending MediaWikiUnitTestCase from accessing MediaWikiServices. That goes a long way towards ensuring database, filesystem, network access etc are prevented in those types of tests.

Original report:

See T266389 for context.

For example, with AbuseFilter, there are tests in tests/phpunit/unit which access MediaWiki services and globals which are otherwise prohibited in MediaWikiUnitTestCase. If you try to run these locally with vendor/bin/phpunit they fail:

composer phpunit:unit -- --exclude-group Broken,ParserFuzz,Stub --stop-on-failure
There was 1 error:

1) AbuseFilterParserTest::testDUNDEFINED with data set #6 ('norm(user_name) !== rmspecials('')')
Error: Class 'Wikimedia\Equivset\Equivset' not found

/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:1610
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:1677
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:1219
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:1016
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:938
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:880
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:869
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:825
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:815
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:780
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:756
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:727
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:695
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:661
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:607
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:533
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:442
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:423
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:404
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:226
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:244
/home/kostajh/src/mediawiki/core/extensions/AbuseFilter/tests/phpunit/unit/AbuseFilterParserTest.php:978
/home/kostajh/src/mediawiki/core/tests/phpunit/MediaWikiUnitTestCase.php:112

But, in CI no error is thrown with the same command: https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php72-noselenium-docker/42693/consoleFull#console-section-9

I don't understand how or why that is happening, but it's supposed to throw an error. IIRC there was a task around this although the only thing I find at the moment is T229220: Unit tests are not being run for extensions under PHPUnit 4.x (HHVM) which is tangential. Pinging @hashar for ideas or clues.

Details

SubjectRepoBranchLines +/-
phpunit: Prevent access to services in UnitTestCasemediawiki/coremaster+53 -8
phpunit: Unit tests may not access MediaWikiServicesmediawiki/extensions/Wikibasemaster+64 -46
Convert PHPUnit\Framework\TestCase to MediaWikiUnitTestCasemediawiki/extensions/Wikibasemaster+214 -230
WikiPageConfigLoaderTest: Don't access MW services from unit testmediawiki/extensions/GrowthExperimentsmaster+333 -240
phpunit: Convert Version/ElasticsearchIntermediary/UserTestingEngine to integration testsmediawiki/extensions/CirrusSearchmaster+14 -54
[WIP] phpunit: Unit tests may not access MW servicesmediawiki/extensions/CirrusSearchmaster+4 -4
phpunit: Unit tests may not access MW servicesmediawiki/extensions/CirrusSearchmaster+53 -13
phpunit: Unit tests may not access MW servicesmediawiki/extensions/Citemaster+6 -6
SiteLinksForDisplayLookupTest: Don't access MW services in unit testmediawiki/extensions/Wikibasemaster+2 -1
phpunit: Block calls to MW services in data providers and in unit testsmediawiki/coremaster+79 -1
Release Quibble 0.0.46integration/quibblemaster+74 -2
Run phpunit-unit before MediaWiki installintegration/quibblemaster+18 -13
Move LocalSettings.php out of the way before running unit testsintegration/quibblemaster+10 -0
phpunit: Switch SpecialPageAliasTest to integration testsmediawiki/coremaster+1 -1
phpunit: Use a closure for services to enable unit testingmediawiki/coremaster+15 -12
[WIP] Prevent access to MediaWikiServices in unit testsmediawiki/coremaster+7 -0
[WIP] Prevent access to MediaWikiServices in integration and unit testsmediawiki/coremaster+2 -6
Show related patches Customize query in gerrit

Related Objects
Search...

Event Timeline

kostajh created this task.Oct 26 2020, 8:53 AM
Daimona added a comment.Oct 26 2020, 10:05 AM

Note, as I wrote in the other task: interestingly, codehealth jobs are not affected, in that these tests fail correctly there, see https://integration.wikimedia.org/ci/job/mwext-codehealth-patch/25987/console.

kostajh added a comment.Oct 26 2020, 11:23 AM
In T266441#6577664, @Daimona wrote:

Note, as I wrote in the other task: interestingly, codehealth jobs are not affected, in that these tests fail correctly there, see https://integration.wikimedia.org/ci/job/mwext-codehealth-patch/25987/console.

Yes, good addition, thanks for flagging. The differences with that one are:

  1. it's using mwext-phpunit-coverage script, which in turn executes:
php -d zend_extension=xdebug.so \
       vendor/bin/phpunit \
       --testsuite=extensions:unit \
       --exclude-group Dump,Broken,ParserFuzz,Stub \
       --coverage-clover "$LOG_DIR"/clover.xml \
       --log-junit "$LOG_DIR"/junit.xml &

That should be just about equivalent to composer phpunit:unit, since the composer script evaluates to vendor/bin/phpunit --colors=always --testsuite=core:unit,extensions:unit,skins:unit.

  1. In the codehealth job, only the AbuseFilter repository is cloned.

The lack of errors in the description could make sense if the AbuseFilter repository wasn't cloned at the time the tests were run, but it doesn't look like that is the case.

kostajh added a comment.Oct 26 2020, 11:48 AM

Hmm, so:

vendor/bin/phpunit --testsuite=core:unit,extensions:unit -- no errors

vendor/bin/phpunit --testsuite=skins:unit,extensions:unit -- errors

vendor/bin/phpunit --testsuite=extensions:unit -- errors

`vendor/bin/phpunit --testsuite=core:unit,skins:unit,extensions:unit -- no errors

kostajh added a comment.Oct 26 2020, 1:24 PM

OK, it looks like if you remove tests/phpunit/unit/includes/language/SpecialPageAliasTest.php then the AbuseFilter unit tests fail as they should.

I don't yet know why that test is bootstrapping MediaWiki nor why calls to MediaWikiServices are allowed inside a test that extends MediaWikiUnitTestCase.

kostajh added a comment.Oct 26 2020, 1:25 PM

One idea is to adjust quibble so that LocalSettings.php is moved out of the way before the unit tests run.

gerritbot added a comment.Oct 26 2020, 2:45 PM

Change 636430 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/quibble@master] Move LocalSettings.php out of the way before running unit tests

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

gerritbot added a project: Patch-For-Review.Oct 26 2020, 2:45 PM
Daimona added a comment.Oct 26 2020, 2:48 PM
In T266441#6578243, @kostajh wrote:

OK, it looks like if you remove tests/phpunit/unit/includes/language/SpecialPageAliasTest.php then the AbuseFilter unit tests fail as they should.

I don't yet know why that test is bootstrapping MediaWiki nor why calls to MediaWikiServices are allowed inside a test that extends MediaWikiUnitTestCase.

validSpecialPageAliasesProvider, which is a data provider, is calling Language::fetchLanguageNames(), which calls MediaWikiServices::getInstance() etc. Might this be the cause? It seems a bit weak though, that a single test doing that will bootstrap MediaWiki and prevent further failures later.

kostajh added a subscriber: awight.Oct 26 2020, 2:56 PM
In T266441#6578491, @Daimona wrote:
In T266441#6578243, @kostajh wrote:

OK, it looks like if you remove tests/phpunit/unit/includes/language/SpecialPageAliasTest.php then the AbuseFilter unit tests fail as they should.

I don't yet know why that test is bootstrapping MediaWiki nor why calls to MediaWikiServices are allowed inside a test that extends MediaWikiUnitTestCase.

validSpecialPageAliasesProvider, which is a data provider, is calling Language::fetchLanguageNames(), which calls MediaWikiServices::getInstance() etc. Might this be the cause? It seems a bit weak though, that a single test doing that will bootstrap MediaWiki and prevent further failures later.

Yeah, apparently that's all you need. :(

I think when this went live, we explicitly ran composer phpunit:unit without LocalSettings.php in place. It seems that this got revised at some point (possibly in the plan execution patches from a few months back?) since the MediaWiki install is happening before the unit tests are run in https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php72-noselenium-docker/42693/consoleFull#console-section-6 . cc @awight

Krinkle subscribed.Oct 27 2020, 4:19 PM

I think we may want to avoid having to move the file in CI, so that out-of–the-box things are locally more similar to CI.

If use of MediaWikiServices is the issue, can we plant some kind of explosive there during the unit test run? It's not clear to me how calling MediaWikiServices would lead to inclusion of LocalSettings.php. I assume this isn't what's happening, but that it is included ahead of time, in which case we may want to fix that so that the composer entry point bootstrapping isn't including any LocalSettings.php file, regardless of whether it exists or not.

kostajh added a comment.Oct 28 2020, 9:56 AM
In T266441#6581966, @Krinkle wrote:

I think we may want to avoid having to move the file in CI, so that out-of–the-box things are locally more similar to CI.

Yeah. Even so, I think it would still be useful to have in quibble, but yes we want this to work locally.

If use of MediaWikiServices is the issue, can we plant some kind of explosive there during the unit test run? It's not clear to me how calling MediaWikiServices would lead to inclusion of LocalSettings.php. I assume this isn't what's happening, but that it is included ahead of time, in which case we may want to fix that so that the composer entry point bootstrapping isn't including any LocalSettings.php file, regardless of whether it exists or not.

Having looked at this more closely, it doesn't seem like LocalSettings.php is the culprit after all. I'm not sure why I thought that 😕

I think the main problem is that we have too much magic going on in our data providers, which are harder to intercept since they are evaluated independently of the MediaWikiUnitTestCase setup process.

gerritbot added a comment.Oct 28 2020, 12:56 PM

Change 636913 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] [WIP] Prevent access to MediaWikiServices in unit tests

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

gerritbot added a comment.Oct 28 2020, 1:01 PM

Change 636915 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] [WIP] Prevent access to MediaWikiServices in integration and unit tests

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

gerritbot added a comment.Oct 29 2020, 7:56 AM

Change 637280 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] phpunit: Block calls to MW services in data providers and in unit tests

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

gerritbot added a comment.Oct 29 2020, 7:57 AM

Change 636915 abandoned by Kosta Harlan:
[mediawiki/core@master] [WIP] Prevent access to MediaWikiServices in integration and unit tests

Reason:
See I5913839d049916cf569efddb250644a47f3206d7

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

gerritbot added a comment.Oct 29 2020, 7:57 AM

Change 636913 abandoned by Kosta Harlan:
[mediawiki/core@master] [WIP] Prevent access to MediaWikiServices in unit tests

Reason:
See I5913839d049916cf569efddb250644a47f3206d7

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

gerritbot added a comment.Nov 19 2020, 10:36 AM

Change 641941 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] phpunit: Use a closure for services to enable unit testing

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

gerritbot added a comment.Nov 19 2020, 10:44 AM

Change 641944 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] phpunit: Switch SpecialPageAliasTest to integration tests

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

gerritbot added a comment.Nov 19 2020, 1:15 PM

Change 641941 merged by jenkins-bot:
[mediawiki/core@master] phpunit: Use a closure for services to enable unit testing

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.20; 2020-12-01).Nov 19 2020, 2:00 PM
gerritbot added a comment.Nov 19 2020, 2:48 PM

Change 641944 merged by jenkins-bot:
[mediawiki/core@master] phpunit: Switch SpecialPageAliasTest to integration tests

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

gerritbot added a comment.Nov 30 2020, 9:18 AM

Change 644174 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/quibble@master] Run phpunit-unit before MediaWiki install

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

gerritbot added a comment.Nov 30 2020, 9:19 AM

Change 636430 abandoned by Kosta Harlan:
[integration/quibble@master] Move LocalSettings.php out of the way before running unit tests

Reason:
We need to fix the bug referenced here in core; that said I took the idea to run phpunit-unit earlier and have a patch for that in I935004d8ed51d1c4a11c6db863ac182bd4d14b34

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

gerritbot added a comment.Nov 30 2020, 10:04 AM

Change 644174 merged by jenkins-bot:
[integration/quibble@master] Run phpunit-unit before MediaWiki install

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

Krinkle unsubscribed.Dec 1 2020, 2:28 AM
kostajh mentioned this in T258089: Errors running `composer phpunit` and warnings running `composer phpunit:integration` on a clean install of mediawiki-core.Dec 4 2020, 2:09 PM
gerritbot added a comment.Jan 7 2021, 9:41 AM

Change 654804 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/quibble@master] Release Quibble 0.0.46

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

gerritbot added a comment.Jan 7 2021, 3:35 PM

Change 654804 merged by jenkins-bot:
[integration/quibble@master] Release Quibble 0.0.46

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

Stashbot added a comment.Jan 7 2021, 4:01 PM

Mentioned in SAL (#wikimedia-releng) [2021-01-07T16:01:18Z] <hashar> Tag Quibble 0.0.46 @ df9e75329ab # T225218 T266441 T263500

Stashbot mentioned this in T263500: phan-taint-check-plugin: Undefined constant 'ast\AST_LIST'.Jan 7 2021, 4:01 PM
Stashbot mentioned this in T225218: Consider httpd for quibble instead of php built-in server.
hashar mentioned this in T274590: Upgrade CI jobs to use Quibble 0.0.46.Feb 11 2021, 8:59 PM
hashar mentioned this in T255793: Run unit tests before installing MediaWiki?.Oct 27 2021, 10:19 AM
kostajh claimed this task.Jul 12 2022, 9:50 AM
gerritbot added a comment.Jul 12 2022, 9:54 AM

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

[mediawiki/core@master] phpunit: Prevent access to MW services in unit test case

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

gerritbot added a comment.Jul 12 2022, 8:11 PM

Change 637280 abandoned by Kosta Harlan:

[mediawiki/core@master] phpunit: Block calls to MW services in data providers and in unit tests

Reason:

See I1ae61d085cf038b68e27ea30c5db7a37c713f9ed

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

gerritbot added a comment.Jul 13 2022, 7:54 AM

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

[mediawiki/extensions/GrowthExperiments@master] WikiPageConfigLoaderTest: Don't access MW services from unit test

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

gerritbot added a comment.Jul 13 2022, 7:59 AM

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

[mediawiki/extensions/Wikibase@master] SiteLinksForDisplayLookupTest: Don't access MW services in unit test

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

gerritbot added a comment.Jul 13 2022, 8:10 AM

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

[mediawiki/extensions/Cite@master] phpunit: Unit tests may not access MW services

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

gerritbot added a comment.Jul 13 2022, 8:23 AM

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

[mediawiki/extensions/CirrusSearch@master] phpunit: Unit tests may not access MW services

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

gerritbot added a comment.Jul 13 2022, 9:55 AM

Change 813585 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SiteLinksForDisplayLookupTest: Don't access MW services in unit test

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.21; 2022-07-18).Jul 13 2022, 10:00 AM
gerritbot added a comment.Jul 13 2022, 1:28 PM

Change 813586 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] phpunit: Unit tests may not access MW services

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

gerritbot added a comment.Jul 13 2022, 1:29 PM

Change 813587 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] phpunit: Unit tests may not access MW services

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

gerritbot added a comment.Jul 13 2022, 6:08 PM

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

[mediawiki/extensions/CirrusSearch@master] [WIP] phpunit: Unit tests may not access MW services

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

gerritbot added a comment.Jul 13 2022, 6:21 PM

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

[mediawiki/extensions/CirrusSearch@master] Revert "phpunit: Unit tests may not access MW services"

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

gerritbot added a comment.Jul 13 2022, 6:21 PM

Change 813664 abandoned by Kosta Harlan:

[mediawiki/extensions/CirrusSearch@master] [WIP] phpunit: Unit tests may not access MW services

Reason:

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

gerritbot added a comment.Jul 13 2022, 9:14 PM

Change 813325 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] phpunit: Convert Version/ElasticsearchIntermediary/UserTestingEngine to integration tests

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

gerritbot added a comment.Jul 14 2022, 9:53 AM

Change 813584 abandoned by Kosta Harlan:

[mediawiki/extensions/GrowthExperiments@master] WikiPageConfigLoaderTest: Don't access MW services from unit test

Reason:

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

gerritbot added a comment.Jul 14 2022, 11:40 AM

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

[mediawiki/extensions/Wikibase@master] phpunit: Unit tests may not access MediaWikiServices

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

gerritbot added a comment.Jul 14 2022, 11:40 AM

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

[mediawiki/extensions/Wikibase@master] Convert PHPUnit\Framework\TestCase to MediaWikiUnitTestCase

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

kostajh renamed this task from Some unit tests are not executed with composer phpunit:unit to MediaWikiUnitTestCase should prevent access to MediaWikiServices.Jul 14 2022, 11:42 AM
kostajh updated the task description. (Show Details)Jul 14 2022, 12:33 PM
• Lucas_Werkmeister_WMDE added a subtask: T277464: [EPIC] Multiple Wikibase service wiring tests fail when run as unit tests.Jul 14 2022, 12:38 PM
• Lucas_Werkmeister_WMDE mentioned this in T277464: [EPIC] Multiple Wikibase service wiring tests fail when run as unit tests.Jul 14 2022, 12:41 PM
kostajh updated the task description. (Show Details)Jul 14 2022, 1:04 PM
ItamarWMDE changed the status of subtask T277464: [EPIC] Multiple Wikibase service wiring tests fail when run as unit tests from Open to Stalled.Jan 31 2023, 3:27 PM
gerritbot added a comment.Mar 16 2023, 8:12 PM

Change 813704 abandoned by Kosta Harlan:

[mediawiki/extensions/Wikibase@master] Convert PHPUnit\Framework\TestCase to MediaWikiUnitTestCase

Reason:

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

gerritbot added a comment.Mar 16 2023, 8:13 PM

Change 813882 abandoned by Kosta Harlan:

[mediawiki/extensions/Wikibase@master] phpunit: Unit tests may not access MediaWikiServices

Reason:

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

gerritbot added a comment.Mar 16 2023, 8:15 PM

Change 813203 abandoned by Kosta Harlan:

[mediawiki/core@master] phpunit: Prevent access to services in UnitTestCase

Reason:

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

Maintenance_bot removed a project: Patch-For-Review.Mar 16 2023, 8:30 PM
Daimona closed this task as Resolved.Aug 19 2023, 4:33 PM

Sorry, I had completely forgotten about this task, but I've been independently working on this and the patch which finally implements this restriction, r938019, was merged yesterday.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and 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