Page MenuHomePhabricator

PHPUnit: Cover all methods in MobileFrontend.skin.hooks.php
Open, NormalPublic

Description

Only getPluralLicenseInfo() has been fully covered in this PHP file. The goal here is to cover all other methods in this file one step at a time. Methods coverage below;

  • interimTogglingSupport() should go up to 100%
  • gradeCImageSupport() should go up to 100%
  • getTermsLink() should go up to 100%
  • getLicense() should go up to 100%
  • prepareFooter() should go up to 100%
  • desktopFooter() should go up to 100%
  • mobileFooter() should go up to 100%

Event Timeline

D3r1ck01 created this task.Jan 27 2019, 4:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 27 2019, 4:18 PM

Change 486811 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test for MobileFrontendSkinHooks::getTermsLink()

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

This task will go through different rounds of TODO, Doing and Needs code review as I'll be writing tests for the methods one at a time.

Change 486811 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test for MobileFrontendSkinHooks::getTermsLink()

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

Change 487074 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test for ::interimTogglingSupport() method

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

D3r1ck01 claimed this task.Jan 29 2019, 7:30 PM
D3r1ck01 triaged this task as Normal priority.

Change 487074 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test for ::interimTogglingSupport() method

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

Change 487475 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test for MobileFrontendSkinHooks::gradeCImageSupport()

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

Change 487475 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test for MobileFrontendSkinHooks::gradeCImageSupport()

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

Jdlrobson added a subscriber: Jdlrobson.

Let me know when you need another round of review by tagging with out sprint board!

Change 490600 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test case for MobileFrontendSkinHooks::getLicense()

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

Change 490600 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit test case for MobileFrontendSkinHooks::getLicense()

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

D3r1ck01 added a comment.EditedFeb 15 2019, 8:18 AM

@Jdlrobson, code coverage https://doc.wikimedia.org/cover-extensions/MobileFrontend/MobileFrontend.skin.hooks.php.html reports 95.56% for ::getLicense() which is not the goal.

One line is not executed (L#153).

Made a follow-up patch: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/490808 to fix this issue.

Thanks @thiemowmde for reviewing and merging the patch that attempts to fix the issue and nicely does so :), per https://doc.wikimedia.org/cover-extensions/MobileFrontend/MobileFrontend.skin.hooks.php.html, we now have the method fully covered to 100%.

nray claimed this task.Feb 18 2019, 6:05 PM
nray added a comment.Feb 19 2019, 7:54 PM

@Jdlrobson should this task be in ready for signoff? According to https://doc.wikimedia.org/cover-extensions/MobileFrontend/MobileFrontend.skin.hooks.php.html, these methods still have 0% coverage:

  • prepareFooter
  • desktopFooter
  • mobileFooter
  • prepareFooter
  • desktopFooter
  • mobileFooter

Matching the task description, yes! I'll have to make 1 more patch to cover these 3 methods at once, so this class is stepped up to 100% once and for all :)

nray removed nray as the assignee of this task.Tue, Feb 26, 7:38 PM
nray added a subscriber: nray.

@Jdlrobson & @pmiazga, in preparation for finishing up the remaining tests, I noticed injecting MobileContext into prepareFooter() is an issue so I wrote up a service wiring for this. See: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/494825. Is this a good idea?

This approach allows one to mock context and inject it into a test when need be but please if there are better ideas, let me know.

Makes sense to me. What do you think @pmiazga ?