Page MenuHomePhabricator
Create Task
Maniphest T202641

Allow NameTableStores to be reset in sync with their associated database tables in unit tests.
Closed, ResolvedPublic
Actions

Description

The purpose of NameTableStores is to provide aggressive caching for certain database tables. These caches need to be reset when the corresponding table is reset between unit tests. NameTableStore has a reloadMap() method for that purpose, but there is no easy way to get hold of all the NameTableStore instances. In particular, RevisionStoreFactory presently constructs NameTableStores for content_models and slot_roles that are untracked, it does not use the instances provided MediaWikiServices::getContentModelStore() and MediaWikiServices::getSlotRoleStore(), in order to be able to cosntruct NameTableStore for an arbitrary target database.

We needs to either:

  • reset ALL services between tests
  • or track all NameTableStore and reset them when the tables get reset
  • or at least make RevisionStoreFactory use the same instances we return from getContentModelStore, getSlotRoleStore, etc. A NameTableStoreFactory may be used to achieve this.

Note: there are several mechanisms that may trigger a table to be re-set:

  1. MediaWikiTestCase::tablesUsed
  2. MediaWikiTestCase::truncateTable
  3. MediaWikiTestCase::getSchemaOverrides

Details

SubjectRepoBranchLines +/-
DNM: Fail when accessing the live DB during tests.mediawiki/coremaster+88 -1
Introduce NameTableStoreFactorymediawiki/coremaster+329 -109
Avoid constructing Title objects in data providersmediawiki/coremaster+152 -62
DNM Add tracking to NameTableStore to find bad state change.mediawiki/coremaster+41 -3
Fix tests that access the database in data providers.mediawiki/extensions/Flowmaster+132 -123
Go back to resetting name tables during tests.mediawiki/coremaster+40 -18
Customize query in gerrit

Related Objects

Event Timeline

daniel created this task.Aug 23 2018, 4:17 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 23 2018, 4:17 PM
daniel mentioned this in P7479 Allow NameTabelStores to be reset in sync with their associated database tables in unit tests..Aug 23 2018, 4:18 PM
daniel updated the task description. (Show Details)Aug 23 2018, 5:10 PM
daniel updated the task description. (Show Details)
daniel added subscribers: • tstarling, Addshore.
Anomie renamed this task from Allow NameTabelStores to be reset in sync with their associated database tables in unit tests. to Allow NameTableStores to be reset in sync with their associated database tables in unit tests..Aug 23 2018, 7:12 PM
Anomie updated the task description. (Show Details)
WMDE-leszek subscribed.Aug 24 2018, 3:01 PM
gerritbot subscribed.Aug 27 2018, 4:46 AM

Change 455487 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/core@master] Introduce NameTableStoreFactory

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

gerritbot added a project: Patch-For-Review.Aug 27 2018, 4:46 AM
• tstarling added a subscriber: CCicalese_WMF.Aug 27 2018, 5:38 AM

@CCicalese_WMF asked me to look at this. Tracking all NameTableStore objects looked easy enough, and is beneficial for other reasons, so I did that in the patch linked above. However, without existing calling code, I'm not sure exactly what @Addshore needs for a reset/reload interface and where it would need to be called, so maybe he could add that in a dependent patch.

daniel added a comment.Aug 27 2018, 9:28 AM

Thanks Tim! I commented on the ticket.

Regarding the reset/reload interface - we may not need one as part of the factory. Resetting a NameTabelStore is needed when the corresponding database table is directly manipulated (typically, truncated during tests). So we want to reset the NameTableStore for a specific table on a specific wiki, which means we could just call $factory->getNameTableStore( $table, $wiki )->reloadMap().

Tgr added a comment.Aug 27 2018, 9:32 AM

Isn't it enough to reset the service itself (as in, resetServiceForTesting)? That takes care of in-process caches and the WAN cache probably gets cleared between tests anyway.

daniel added a comment.Aug 27 2018, 9:46 AM
In T202641#4533822, @Tgr wrote:

Isn't it enough to reset the service itself (as in, resetServiceForTesting)? That takes care of in-process caches and the WAN cache probably gets cleared between tests anyway.

No, that will not work, because the NameTableStore instances are used inside other services, most notably inside RevisionStore. We'd have to reset all services that use NamedTableStores as well, and then all services that use these services, and... we'd probably best just reset all services between tests, always. I'm actually considering that, but the overhead seems too big. We could also declare and track dependencies between services, but that gets us further away from the "plain php" approach to DI, and would add quite a bit of a maintenance burden.

gerritbot added a comment.Aug 27 2018, 11:08 AM

Change 455540 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Go back to resetting name tables during tests.

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

gerritbot added a comment.Aug 28 2018, 9:18 AM

Change 455540 abandoned by Daniel Kinzler:
Go back to resetting name tables during tests.

Reason:
Squashed into Ic0f2d1d94bad9d

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

gerritbot added a comment.Aug 28 2018, 2:09 PM

Change 455843 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/Flow@master] Fix tests that access the database in data providers.

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

gerritbot added a comment.Aug 28 2018, 5:45 PM

Change 455633 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DNM: Fail when accessing the live DB during tests.

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

gerritbot added a comment.Aug 28 2018, 10:12 PM

Change 455843 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Fix tests that access the database in data providers.

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-09-04 (1.32.0-wmf.20)).Aug 28 2018, 11:00 PM
gerritbot added a comment.Aug 29 2018, 4:17 PM

Change 456172 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DNM Add tracking to NameTableStore to find bad state change.

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

gerritbot added a comment.Aug 31 2018, 5:15 AM

Change 456542 had a related patch set uploaded (by Tim Starling; owner: Daniel Kinzler):
[mediawiki/core@master] Avoid constructing Title objects in data providers

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

gerritbot added a comment.Aug 31 2018, 1:35 PM

Change 456172 abandoned by Daniel Kinzler:
DNM Add tracking to NameTableStore to find bad state change.

Reason:
obsolete

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

gerritbot added a comment.Sep 4 2018, 12:23 AM

Change 456542 merged by jenkins-bot:
[mediawiki/core@master] Avoid constructing Title objects in data providers

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

gerritbot added a comment.Sep 4 2018, 8:57 AM

Change 455487 merged by jenkins-bot:
[mediawiki/core@master] Introduce NameTableStoreFactory

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

• tstarling closed this task as Resolved.Sep 4 2018, 11:25 PM
• tstarling claimed this task.
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:38 PM
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