Page MenuHomePhabricator

Don't use RequestContext::getMain() in MinervaNeue UnitTests
Closed, DeclinedPublic

Description

This problem interferes with development and is a nuisance which makes it harder to write php changes.

RequestContext::getMain() causes a side effect - it creates a static instance of RequestContext inside RequestContext class. When unit tests operates on RequestContext it modifies the cached global instance which may cause bad behaviour.

As an example some of the phpunit tests in MobileFrontend fail when run in isolation. Here's an attempt to fix one of them. We also cannot create a mock RequestContext and pass it to SkinMinerva.

There are at least two ways of fixing it:

  • proper solution: do not use RequestContext::getMain() inside unit tests - as a result we will have to refactor SkinMinerva::__construct() to not call SkinMinerva::getTitle() which calls RequestContext::getMain()
  • quick hack - each unittests which calls RequestContext::getMain() should have defined tearDown() method that calls RequestContest::resetMain()

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 12 2017, 5:03 PM
Jdlrobson triaged this task as Normal priority.May 16 2017, 10:17 AM
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson added a subscriber: Jdlrobson.

Which tests?
What is the proposed solution?
Any risks / unknowns or is the problem clearly defined?

pmiazga renamed this task from Properly fix some phpunit tests to Don't use RequestContext::getMain() in UnitTests.May 16 2017, 6:42 PM
pmiazga updated the task description. (Show Details)

@Jdlrobson I fixed the description. I'm familiar with the problem and I would like to work on it. Please verify if new description fit your needs, If yes please move it to To triage

Looks good @pmiagza. Hopefully during an estimation we can identify how difficult it will be to do the proper solution

Feel free to work on this outside sprint if you're itching to to try and work out which solution we will go with.

Jdlrobson updated the task description. (Show Details)May 17 2017, 6:29 AM

Change 354117 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Do not call RequestContext::getMain() in SkinMinervaConstructor

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

I pushed the proper solution.

Change 354117 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Do not call RequestContext::getMain() in SkinMinervaConstructor

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

Now that the above patch has been merged, we've left 3 more instances to fix:

MobileFrontend.hooksTest.php:123:57:		$mainContext = new DerivativeContext( RequestContext::getMain() );
MobileContextTest.php:46:53:		$context = new DerivativeContext( RequestContext::getMain() );
MobileContextTest.php:560:19:		RequestContext::getMain()->setRequest( $req );

Bringing it to a current sprint board as the task is done (implemented & reviewed). Now it requires technical sign off.

pmiazga removed pmiazga as the assignee of this task.May 23 2017, 9:41 PM
pmiazga moved this task from To Do to Ready for Signoff on the Readers-Web-Kanbanana-Board-Old board.
pmiazga added a subscriber: pmiazga.

@bmansurov This patch fixes only SkinMinerva tests as it required a bit of refactoring the Skin class. Other issues will be fixed in different hygiene patches.

Removing from sprint board as there is more required and I won't be able to fit it into current sprint

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Mass-moving all items tagged for MediaWiki 1.30.0-wmf.3, as that was never released; instead, we're using -wmf.4.

Jdlrobson renamed this task from Don't use RequestContext::getMain() in UnitTests to Don't use RequestContext::getMain() in MinervaNeue UnitTests.Jul 13 2017, 5:47 PM
Jdlrobson edited projects, added MinervaNeue; removed MobileFrontend.
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.Jul 13 2017, 6:04 PM

Can this be please updated to reflect the remaining work? It's tagged MinervaNeue but I don't think there's anything to be done there..?

phuedx removed a subscriber: phuedx.Aug 2 2017, 7:16 AM

@Jdlrobson - I'll check it today.

Ping @pmiazga. Can we move this to "Triaged but Future"?

I have a pretty big problem with this task, I'm still trying to grasp the global state access in MediaWiki unit tests but looks like it's not possible to not use global RequestContext::getMain(). Too many things are using RequestContext::getMain() instead of taking ContextSource as a dependency, Most of MWCore is using RequestContext::getMain() and we're not able to fix it easily. MediaWikiTestCase is calling RequestContext::resetMain() during tearDown() so every test should have new RequestContext and the issue we had previously wasn't related to using global RequestContext instance.
It had to be different issue because the quick hack was already there.

Passing RequestContext as a dependency is already done, there is only one instance where we still call RequestContext::getMain(), refactoring that won't hurt but also will not bring any good. And even if we pass new RequestContext we still should override global RequestContext in case MinervaSkin calls something that is using global state (most of the core).

The best call is probably to reject this task as the proposed solution doesn't bring any good to current codebase.

bmansurov closed this task as Declined.Sep 6 2017, 4:47 PM

Thanks!