Page MenuHomePhabricator

PHPUnit: Cover methods in content-providers/McsContentProvider.php with unit tests
Closed, ResolvedPublic

Description

Code coverage for the content-providers/McsContentProvider.php is 0% and unit tests should be written to cover all it's code.

Acceptance Criteria

Event Timeline

Change 475730 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit tests for methods in McsContentProvider::class

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

D3r1ck01 moved this task from Backlog to Doing [WIP] on the User-D3r1ck01 board.
D3r1ck01 triaged this task as Normal priority.Nov 26 2018, 1:57 PM

This will definitely go in for Needs more work but let it undergo Round 1 (R1) review to get some feedback on how to proceed. Thanks :)

Again, a request for a gentle review :), may likely move back to "Needs more work" but would like to know if this is the right direction :)

pmiazga closed this task as Resolved.Dec 5 2018, 9:29 PM
pmiazga removed a project: Patch-For-Review.
pmiazga updated the task description. (Show Details)

Change 475730 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit tests for methods in McsContentProvider::class

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

Change 479246 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Reset default mobilefrontend provider

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

Change 479250 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[operations/mediawiki-config@master] Define the default mobile content provider

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

Change 479246 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Reset default mobilefrontend provider

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

Change 479265 had a related patch set uploaded (by Jdlrobson; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@wmf/1.33.0-wmf.8] Reset default mobilefrontend provider

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

Change 479265 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.33.0-wmf.8] Reset default mobilefrontend provider

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

Mentioned in SAL (#wikimedia-operations) [2018-12-13T00:22:36Z] <dcausse@deploy1001> Synchronized php-1.33.0-wmf.8/extensions/MobileFrontend/extension.json: T210390: Reset default mobilefrontend provider (duration: 00m 53s)

Jdlrobson reopened this task as Open.Dec 13 2018, 5:46 PM

Looks like the units are failing in core (T211903) so I'm going to do the easiest thing and revert these tests. Please don't be too dispirited - we'll attempt these again!

Looks like the units are failing in core (T211903) so I'm going to do the easiest thing and revert these tests. Please don't be too dispirited - we'll attempt these again!

Sure, no problem :)

D3r1ck01 updated the task description. (Show Details)Dec 13 2018, 8:18 PM
D3r1ck01 moved this task from Reviewed/Resolved to Backlog on the User-D3r1ck01 board.

Looking forward to round 2 of review!

The game continues :)

D3r1ck01 reassigned this task from D3r1ck01 to pmiazga.Dec 17 2018, 11:34 AM
D3r1ck01 moved this task from Backlog to Reviewed/Resolved on the User-D3r1ck01 board.

Per https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/479757, @pmiazga submitted a PS for review, though it needs a rebase, so adjusting this appropriately.

Change 479250 abandoned by Pmiazga:
Define the default mobile content provider

Reason:
per Jdlrobson comment

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

Change 479757 had a related patch set uploaded (by Jdlrobson; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Revert and fix "Revert "Add PHPUnit tests for methods in McsContentProvider::class""

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

Change 479757 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Revert and fix "Revert "Add PHPUnit tests for methods in McsContentProvider::class""

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

phuedx added a subscriber: phuedx.Dec 18 2018, 11:00 AM

Code Coverage has gone up to 100%

From https://integration.wikimedia.org/ci/job/mwext-phpunit-coverage-patch-docker/6645/console, run against PS5 of https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/479757/:

+------------------------------------------+-------+--------+
| Filename                                 | Old % | New %  |
+------------------------------------------+-------+--------+
| content-providers/McsContentProvider.php | 0     |  97.30 |
+------------------------------------------+-------+--------+

While code coverage is an important metric, 100% should be an aim and not a strong requirement. 97.3% is ๐Ÿ‘Œ

phuedx closed this task as Resolved.Dec 18 2018, 11:01 AM
phuedx updated the task description. (Show Details)

Being bold.

Change 482778 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/extensions/MobileFrontend@master] Follow-up on Ie224b89787b631bd597273c20cae4

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

While code coverage is an important metric, 100% should be an aim and not a strong requirement. 97.3% is ๐Ÿ‘Œ

@phuedx: So if it wasn't for @Jdlrobson, we would have let this be but yes, thanks a lot Jon for the extra push. Generally, I love 100% coverage though it's not a guarantee but I think it's always better than < 100%. In that case, ๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰, with the follow up patch, the one left additional code path has been covered, now we have 100% coverage for this class. ๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰๐ŸŽ‰

D3r1ck01 claimed this task.Jan 8 2019, 11:41 AM
D3r1ck01 updated the task description. (Show Details)

Change 482797 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/MobileFrontend@master] Streamline McsContentProvider constructor and tests

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

Change 482778 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Follow-up on Ie224b89787b631bd597273c20cae4

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

Change 482797 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Streamline McsContentProvider constructor and tests

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

nray claimed this task.Jan 8 2019, 6:02 PM
nray added a comment.Jan 8 2019, 8:43 PM

100% code coverage now. Good job all!

nray closed this task as Resolved.Jan 8 2019, 8:54 PM
nray removed nray as the assignee of this task.
nray updated the task description. (Show Details)
nray added a subscriber: nray.