Page MenuHomePhabricator

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

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

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 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)

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

@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.

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.

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.

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

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

Reason:
Squashed into Ic0f2d1d94bad9d

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

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

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

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

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

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

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

Reason:
obsolete

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

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

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

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.