Page MenuHomePhabricator

Do not initialise the database in tests when not needed
Closed, ResolvedPublic

Description

In the unit tests, the database is initialized in MediaWikiTestCase::run(). Since e638075, it is initialized "once" per class type (whatever is the result of needsDB() because it’s a OR) or when needsDB() is true.

On classes where no database is needed, the database is initialised anyway, it shouldn’t. This makes the tests unnecessarily take a bit longer. The change https://gerrit.wikimedia.org/r/#/c/328718 was meant to fix this thing, but an additional issue appeared: the test database is the object property $db, but its state is controlled by a static variable ($dbSetup) which is not kept in sync with $db, because when the object is destroyed the $db is unset but $dbSetup remains. In the next test, the $db is not initialized (since $dbSetup is true) but the test expects it is and $this->db->whatever() fails.

In MediaWikiTestCase::run() there is a TODO comment indicating the DB setup should be done in setUpBeforeClass. If this is chosen, all DB-related variables will be static and should be consistently initialized and destroyed.

This bug is created to track the small refactoring/rewrite of the part of code used to initialise and destruct the database in the tests, in order to finally submit the proposed patch https://gerrit.wikimedia.org/r/#/c/328718 without any error. It will also improve T93556. Possibly, if the code is moved in setUpBeforeClass, it should be checked that this new setup is supported by major extensions.

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+13 -14
mediawiki/extensions/OAuthmaster+1 -0
mediawiki/coremaster+63 -12
mediawiki/extensions/CirrusSearchmaster+113 -17
mediawiki/coremaster+5 -0
mediawiki/extensions/GrowthExperimentsmaster+33 -7
mediawiki/extensions/GrowthExperimentsmaster+8 -5
mediawiki/coremaster+14 -0
mediawiki/extensions/AbuseFiltermaster+15 -14
mediawiki/coremaster+103 -16
mediawiki/coremaster+44 -20
mediawiki/extensions/GeoDatamaster+37 -14
mediawiki/extensions/Mathmaster+22 -2
mediawiki/skins/MinervaNeuemaster+23 -2
mediawiki/coremaster+116 -5
mediawiki/extensions/Translatemaster+18 -3
mediawiki/coremaster+40 -43
mediawiki/extensions/TimedMediaHandlermaster+2 -0
mediawiki/extensions/Gadgetsmaster+7 -0
mediawiki/coremaster+82 -53
mediawiki/coremaster+13 -5
mediawiki/coremaster+52 -1
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

For the record: as Tim said on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/377041#message-e2b7cc4df2aae9c6b8d307a854e9bd8af7e00352, we need to make sure that no test can ever write to the real DB, and thus need to set up a fake DB before running any test.

It would make sense to set up a dysfunctional DB for tests that don't declare to need a database. One simple way to do that is to set the table prefix to nensense until we hit a test that has needsDb() set to true, at which point a functional prefix with fake tables would be set up.

Change 377041 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] [WIP] Do not setup the test database for tests which don't need it

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

Change 377041 abandoned by Gergő Tisza:

[mediawiki/core@master] [WIP] Do not setup the test database for tests which don't need it

Reason:

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

This really needs to be done. Aside from all the things already mentioned in this task, I tried running paratest on MW core for integration tests (no-database ones!) and I got some random DB-related failures. This is surprising to say the least. I'll take a stab at this as part of the broader PHPUnit refactoring for T227900 etc.

Also, re making sure that no tests access the real database: before running any tests (e.g., in setUpBeforeClass or similar) we could disable the DBLoadBalancerFactory, and perhaps for good measure set the DB credential globals to something random.

Change 377041 restored by Daimona Eaytoy:

[mediawiki/core@master] [WIP] Do not setup the test database for tests which don't need it

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

Change 377041 abandoned by Daimona Eaytoy:

[mediawiki/core@master] [WIP] Do not setup the test database for tests which don't need it

Reason:

Actually, it's probably better to start over and link to this patch instead.

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

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

[mediawiki/core@master] phpunit: Do not setup the test DB for tests that don't need it

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

I was looking at the failures of the patch above. I'm generally not scared when there's a lot of stuff to fix, but here there's really a lot of stuff to fix. I couldn't even find any low hanging fruits to cut down cleverly (i.e., not by just adding @group Database) on the failures. One of the biggest offenders seems to be ResourceLoader (because it has lots of tests, but also an unconditional wfGetDB call). Another one is the service instantiator of ExternalStoreFactory (used indirectly by several services), which calls LoadBalancer::getLocalDomainID(). I'll maybe try and fix something, but fixing all tests is really hard, unless we want to just put all the failing ones in the database group and then have some automatic check to flag database tests that don't actually need the database to move them back when possible.

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

[mediawiki/core@master] tests: Avoid database usage when possible

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

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

[mediawiki/core@master] phpunit: Do not call addCoreDBData if the test doesn't need the DB

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

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

[mediawiki/core@master] Throw exception in getTestUser etc. if the test doesn't need the DB

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

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

[mediawiki/core@master] Deprecate MediaWikiIntegrationTestCase::$users

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

I was thinking about this, and I think we should allow calls to simple methods like LBFactory::getLocalDomainID for now, but mock their return value. As long as the tests don't make any queries, it should be fine. getLocalDomainID is used a lot in ServiceWiring and other places that simply try to figure out what's the local wiki. I think this is fine, and it would avoid the majority of the failures I've seen.

I was thinking about this, and I think we should allow calls to simple methods like LBFactory::getLocalDomainID for now, but mock their return value. As long as the tests don't make any queries, it should be fine. getLocalDomainID is used a lot in ServiceWiring and other places that simply try to figure out what's the local wiki. I think this is fine, and it would avoid the majority of the failures I've seen.

Using getLocalDomainID() to get the logical wiki IDs seems backwards, we should provide a better way to do this.

We currently don't have a great place for this info, but I could live with MediaWikiServices::getWikiID(). This matches the idea that eventually (tm), we want to be able to have multiple instance of MediaWikiServices, each configured for a different wiki, happily coexisting.

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

[mediawiki/core@master] LogFormatterTestCase: avoid database access

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

I was thinking about this, and I think we should allow calls to simple methods like LBFactory::getLocalDomainID for now, but mock their return value. As long as the tests don't make any queries, it should be fine. getLocalDomainID is used a lot in ServiceWiring and other places that simply try to figure out what's the local wiki. I think this is fine, and it would avoid the majority of the failures I've seen.

Using getLocalDomainID() to get the logical wiki IDs seems backwards, we should provide a better way to do this.

We currently don't have a great place for this info, but I could live with MediaWikiServices::getWikiID(). This matches the idea that eventually (tm), we want to be able to have multiple instance of MediaWikiServices, each configured for a different wiki, happily coexisting.

I agree, and more generally, anything related to obtaining a wiki ID seems confusing at the moment. For now though, I'd really just like to get this task done, doing whatever it takes to make everything pass but without bigger refactorings if possible.

Also, FTR: another big offender is getTestUser (& friends). I'm restricting those to DB tests only, and I think r938463 will pass once all the patches I've created for extensions are merged. Then there are many more failures, but I'll get to them eventually.

Change 938440 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Do not call addCoreDBData if the test doesn't need the DB

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

Change 938464 merged by jenkins-bot:

[mediawiki/core@master] Deprecate MediaWikiIntegrationTestCase::$users

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

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

[mediawiki/extensions/Gadgets@master] Avoid loading gadgets if the storage backend is disabled in tests

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

Change 942763 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Avoid loading gadgets if the storage backend is disabled in tests

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

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

[mediawiki/extensions/TimedMediaHandler@master] tests: Add `@group Database` where needed

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

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

[mediawiki/extensions/Translate@master] tests: Add `@group Database` to tests that need it

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

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

[mediawiki/core@master] session: Do not save user token in non-database tests

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

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

[mediawiki/extensions/AbuseFilter@master] tests: Avoid DB access in non-Database tests

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

Change 942818 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] tests: Add `@group Database` where needed

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

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

[mediawiki/core@master] Avoid DB access in non-database tests

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

Change 942819 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] tests: Add `@group Database` to tests that need it

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

Change 938406 merged by jenkins-bot:

[mediawiki/core@master] tests: Avoid database usage when possible

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

Change 938886 merged by jenkins-bot:

[mediawiki/core@master] LogFormatterTestCase: avoid database access

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

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

[mediawiki/skins/MinervaNeue@master] Avoid DB access in non-database tests

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

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

[mediawiki/extensions/CirrusSearch@master] Avoid DB access in non-database tests

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

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

[mediawiki/extensions/GrowthExperiments@master] Make WikiPageConfigLoader a no-op in non-database integration tests

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

Change 944301 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Avoid DB access in non-database tests

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

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

[mediawiki/extensions/GeoData@master] Avoid DB access in non-database tests

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

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

[mediawiki/extensions/Math@master] Avoid DB access in non-Database tests

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

This is relatively close to completion now. With all the patches currently in review (probably around 15-20), there should be <50 failures in core + gated extensions and skins. One thing to note is that tests for extensions not in the gate might be broken by the core patch. The quickest fix is to simply add @group Database, although in some cases it can be avoided in favour of mocks.

Change 945814 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Avoid DB access in non-Database tests

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

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

[mediawiki/core@master] Avoid DB access in more non-Database tests

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

Change 945695 merged by jenkins-bot:

[mediawiki/extensions/GeoData@master] Avoid DB access in non-database tests

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

Change 945918 merged by jenkins-bot:

[mediawiki/core@master] Avoid DB access in more non-Database tests

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

Change 943649 merged by jenkins-bot:

[mediawiki/core@master] Avoid DB access in non-database tests

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

I ran

composer phpunit:entrypoint -- --exclude-group Broken,ParserFuzz,Stub,Database,Standalone

(same command used in CI) in my MediaWiki-Docker setup with a bunch of extensions enabled, first with r942820 checked out (patch immediately before the ones that removes the redundant DB setup), and then with r938387 (patch that removes the redundant setup). Here are the results; you can ignore the failure, it's caused by T342193.

Before:

Time: 02:47.812, Memory: 927.50 MB

FAILURES!
Tests: 28483, Assertions: 283078, Failures: 1, Skipped: 172.

After:

Time: 02:27.406, Memory: 935.50 MB

FAILURES!
Tests: 28483, Assertions: 283078, Failures: 1, Skipped: 172.

Which means about 20 seconds saved (YMMV, of course). I should also note that saving time was not the main goal of this task though, so I would consider this a nice bonus.

Change 942821 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] tests: Avoid DB access in non-Database tests

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

Change 938463 merged by jenkins-bot:

[mediawiki/core@master] Throw exception in getTestUser etc. if the test doesn't need the DB

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

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

[mediawiki/extensions/GrowthExperiments@master] tests: Fix invalid user in HomepageModuleRegistryTest

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

Change 948223 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] tests: Fix invalid user in HomepageModuleRegistryTest

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

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

[mediawiki/core@master] phpunit: Use StaticUserOptionsLookup when storage is disabled

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

Change 944970 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Make WikiPageConfigLoader a no-op in non-database integration tests

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

Change 942820 merged by jenkins-bot:

[mediawiki/core@master] session: Do not save user token in non-database tests

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

Change 944356 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] Avoid DB access in non-database tests

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

Change 938387 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Do not setup the test DB for tests that don't need it

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

Change 954987 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/OAuth@master] Mark AccessTokenEndpointTest as needing the DB

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

Change 954987 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Mark AccessTokenEndpointTest as needing the DB

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

Change #948194 merged by jenkins-bot:

[mediawiki/core@master] phpunit: Use StaticUserOptionsLookup when storage is disabled

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