Page MenuHomePhabricator

[EPIC] Split MobileContext god object apart
Open, LowPublic

Description

The MobileContext and MobileFrontendHooks classes are fundamental to how MobileFrontend works but they do a lot of things, which makes them expensive to exercise and to maintain. By breaking these classes apart and testing those parts in isolation with a comprehensive suite of unit tests, we can reduce this cost and make changes with increased confidence.

Why are they expensive to exercise and maintain?

Both classes access and manipulate global state, which make them hard to isolate and, consequently, unit test. Instead, we test these classes via integration and acceptance tests, automating a UA browsing a running instance of MediaWiki, which are sluggish and, in my experience, flaky. Moreover, we don't run our entire suite of acceptance tests against each commit and I'd be willing to bet that our acceptance tests don't all of the above.

What will this cost?

I expect that the many steps of this migration will be hard to review and validate. Designing our code so that it can be unit tested will require increased discipline while making changes (and reviewing changes!). We'll have to get better at communicating and critiquing our designs. If I'm correct and there are methods of these classes that aren't covered by our acceptance tests, then there'll likely be regressions. We'll have to be disciplined in making changes small enough to balance minimising risk and maximising feedback so that we can move forward.

An often unexpected side effect of rigorously breaking apart a large object into a system of increasingly smaller objects is that, unlike a 20 line function in $yourFavouriteImperativeLanguage, it's hard to immediately see how those objects interact. High-level documentation, small interfaces, and unit tests help with this.


TODONE

  • Introduce MediaWikiServices to MobileFrontend (819064d)

Nebulous thoughts that may eventually become sub-tasks (read: TODO)

  • Extract:
    • the MobileFrontendRequest object
  • Discuss action/controller terminology with the team (@Jhernandez)

Event Timeline

phuedx created this task.Aug 17 2016, 8:19 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 17 2016, 8:19 AM
This comment was removed by phuedx.
phuedx updated the task description. (Show Details)Aug 17 2016, 8:39 AM
Jhernandez updated the task description. (Show Details)Aug 17 2016, 9:10 AM

Change 305211 had a related patch set uploaded (by Phuedx):
Introduce MediaWikiServices

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

Change 305212 had a related patch set uploaded (by Phuedx):
Add MobileFrontend.Context service

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

Jhernandez updated the task description. (Show Details)Aug 17 2016, 10:13 AM
phuedx updated the task description. (Show Details)Aug 17 2016, 10:15 AM
Jhernandez updated the task description. (Show Details)Aug 17 2016, 10:15 AM
phuedx updated the task description. (Show Details)Aug 17 2016, 10:33 AM

Did a work breakdown with @phuedx. @phuedx to follow up with refined value statements and costs/risks, and re-engage with team.

phuedx claimed this task.Aug 23 2016, 3:47 PM
phuedx updated the task description. (Show Details)Aug 25 2016, 8:22 AM
phuedx updated the task description. (Show Details)Aug 25 2016, 9:29 AM
phuedx updated the task description. (Show Details)Aug 25 2016, 1:49 PM

Change 305211 merged by jenkins-bot:
Introduce MediaWikiServices

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

phuedx updated the task description. (Show Details)Aug 26 2016, 7:20 AM
phuedx updated the task description. (Show Details)Aug 27 2016, 10:41 AM

My original email:

After some discussion of the Dependency Injection RFC at this year's MediaWiki Dev Summit, a top-level service locator/dependency injection container, the MediaWikiServices class, was added to the MediaWiki codebase in April, along with with a helpful note about migrating hook handlers to allow unit testing (testing in isolation). That MediaWikiServices is a service locator provides us the path to migrate from what we have to loosely-coupled, more readily testable objects.

The classes that would benefit from this the most are: the MobileContext singleton and god object; and the MobileFrontendHooks class, which relies on global state, by way of MobileContext, and groups together many disparate hook handlers that often do far too much. These two classes include a lot of application and business – non-profit? – logic, which is mostly exercised in our acceptance tests and by hand, with the former doubling as integration tests. That isn't to downplay our suite of acceptance tests. However, they're slow, expensive, and unreliable. While we should be able to depend on our acceptance tests, we shouldn't have to depend on them to test business logic when we have the facility to write fast, cheap unit tests.

As for MobileContext and MobileFrontendHooks: I'd like to propose a goal and a plan for migrating each to make use of MediaWikiServices wherever necessary.

In no particular order, MobileContext should be split into:

  • a DeviceService class, which provides information about the UA
  • a FeaturesService class, which provides information about the current features enabled in MobileFrontend
  • a MobileFrontendRequest, which proxies the main Request object, providing useful accessor methods (e.g. MobileFrontendRequest#getMobileAction)
  • a PageBlacklistService class, which tests whether or not a title or category is blacklisted
  • a number of actions – controllers, I guess – which correspond to the setting or unsetting of the mf_useformat, stopMobileRedirect, and disableImages cookies
  • a number of functions for manipulating URLs so that they conform with $wgMobileUrlTemplate

For each of the above, we'd:

  • make an exhaustive list of the clients of the methods
  • cover the associated methods with tests
  • extract the methods and their dependencies, providing a compatibility layer so that the tests still pass
  • migrate the tests to unit tests
  • migrate the clients of the methods

For MobileFrontendHooks, there are some hook handlers than can be migrated by following the helpful note above verbatim, creating a class per hook, obeying the single responsibility principle (SRP). For those hook handlers that do too much, they can be broken into separate classes, i.e. creating multiple classes per hook, which are then grouped by namespace, e.g. MobileFrontend\Images\MakeGlobalVariablesScriptHookHandler and MobileFrontend\Wikibase\MakeGlobalVariablesScriptHookHandler. Regardless, to migrate a hook we'd:

  • cover the hook with tests
  • extract the hook, following the helpful note, and providing a compatibility layer so that the tests still pass
  • migrate the tests to unit tests

It's important to remember that both classes can be migrated method by method. I'll be writing up epics for the high-level goal of migrating these classes, including examples, and I imagine we'll create task for each method (or couple of methods) as we investigate them.

Change 307225 had a related patch set uploaded (by Florianschmidtwelzow):
Re-Apply Introduce MediaWikiServices

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

Jhernandez triaged this task as Normal priority.Aug 29 2016, 5:13 PM

Change 307225 merged by jenkins-bot:
Re-Apply Introduce MediaWikiServices

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

Change 305212 merged by jenkins-bot:
Add MobileFrontend.Context service

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

Jdlrobson lowered the priority of this task from Normal to Low.Jun 5 2017, 11:23 PM
Jdlrobson added a subscriber: Jdlrobson.
D3r1ck01 added a subscriber: D3r1ck01.