Page MenuHomePhabricator

[2hrs] Add missing @covers statements in MobileFrontend and MinervaNeue PHPUnit tests
Closed, ResolvedPublic

Description

We have coverage reports! https://doc.wikimedia.org/cover-extensions/MobileFrontend/
Eventually, we'd like CI to complain when unit test coverage drops. To do this we need to annotate our code so MediaWiki core knows about it.

We're running coverage tests with forceCoversAnnotation=trueoption. It means that all PHPUnit tests require @covers statement, otherwise coverage report will not include those unit tests. Because in near future we will enable code coverage reports for MobileFrontend extension (Task: T151333) it would be nice to fix it.

Additionally we should add PHPCS check that every unit tests must have @covers/@coversNothing/@coversDefaultClass annotation so we do not end up with same issue as we already solved similar problem some time ago (Task: T102006 ).

FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/devices/DeviceDetectorServiceTest.php
-------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------
 47 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 56 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 69 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
-------------------------------------------------------------------------------------------------------


FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/devices/UADeviceDetectorTest.php
-------------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
-------------------------------------------------------------------------------------------------------------
 153 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 163 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 170 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 193 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 203 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
-------------------------------------------------------------------------------------------------------------


FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/devices/AMFDeviceDetectorTest.php
------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------------------
 34 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 58 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 79 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
------------------------------------------------------------------------------------------------------------


FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/devices/CustomHeaderDeviceDetectorTest.php
-----------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------
 40 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
 46 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
-----------------------------------------------------------------------------------------------------------


FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/devices/DeviceDetectorServiceIntegrationTest.php
-----------------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
-----------------------------------------------------------------------------------------------------------------
  55 | ERROR | Missing PHPUnit code coverage annotation.(MissingCovers.Missing)
  64 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
  71 | ERROR | Missing PHPUnit code coverage annotation.(MissingCovers.Missing)
  88 | ERROR | Missing PHPUnit code coverage annotation.(MissingCovers.Missing)
 102 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
-----------------------------------------------------------------------------------------------------------------


FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/MobileContextTest.php
-------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
 558 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
-------------------------------------------------------------------------------------------------------------


FILE: /vagrant/mediawiki/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php
-------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
 188 | ERROR | Missing PHPUnit code coverage annotation. (MissingCovers.Missing)
-------------------------------------------------------------------------------------------------------------

Time: 10.6 secs; Memory: 21.75Mb

In total there are 21 test cases where we need to check which functions are tested and add proper @covers statements.

After fixing those it would be nice to include MissingCoversSniff to phpcs config.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ovasileva triaged this task as Medium priority.Nov 23 2016, 5:11 PM
ovasileva moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.
Jdlrobson moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.

Proposing for next sprint.

Estimations were a little wild here 2-8 so pulling out of the sprint. This task should be broken down into subtasks.

Moving task to "Triaged but future". ~34 test cases are missing @covers statement.

Change 354752 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] [WIP] Hygiene: fix missing @covers annotations in codebase

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

Change 354752 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: fix missing @covers annotations in codebase

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

Jdlrobson renamed this task from Add missing @covers statements in MobileFrontend PHPUnit tests to Add missing @covers statements in MobileFrontend and MinervaNeue PHPUnit tests.Jul 13 2017, 6:06 PM
Jdlrobson added a project: MinervaNeue.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.

This looks good to me as it's written. However, since this task appears to be coming out of the abyss, ping @pmiazga :] just in case he wants to make any changes before it enters the sprint.

This task is ready for dev. There are only 21 occurrences left in MobileFrontend extension, SkinMinerva has all unit tests fixed.

pmiazga renamed this task from Add missing @covers statements in MobileFrontend and MinervaNeue PHPUnit tests to [2hrs] Add missing @covers statements in MobileFrontend and MinervaNeue PHPUnit tests.Nov 9 2017, 12:29 PM

MinervaNeueue is already fixed, there are still some occurrences in MobileFrontend, most of the issues are easy to fix. During sprint prioritization, we decided to timebox this task to 2 hours as it should be enough to fix most (if not) issues.
There is no risk involved with this task, as most of the changes are just adding phpdoc blocks to unit tests. No logic changes.

Change 407010 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Add missing @covers statemenets to Device detection tests

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

Change 407014 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Add missing @covers annotation to phpunit tests

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

pmiazga moved this task from To Do to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.

Bringing it to the current sprint - it's a super small task, has no risk. All changes are only phpunit annotations for code coverage generator. No logic/ui change.

Change 407014 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Add missing @covers annotation to phpunit tests

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

Change 407010 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Add missing @covers statements to Device detection tests

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

Change 407038 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Add missing @covers statements

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

DeviceDetectorService is now showing 100% coverage. I found a few places I would expect coverage and captured them in https://gerrit.wikimedia.org/r/407038

I think this is good enough. A better approach to addressing this problem would be as part of refactoring PHP tasks, we ensure the coverage increases. For example as we refactor MobileFormatter we should aim for 100% coverage.

Change 407038 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add missing @covers statements

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

Change 407124 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Add @covers statements for ParsedMessageModule and MobileLanguages

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

Change 407124 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add @covers statements for ParsedMessageModule and MobileLanguages

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

@Niedzielski given T179094 and our now more fashionable coverage can we sign this off? I think it would be prudent to investigate and fix any more given we have already exhausted the timebox.

Additionally we should add PHPCS check
After fixing those it would be nice to include MissingCoversSniff to phpcs config.

I don't believe this has been done.

I'm not able to quite generate the results on the website but locally I'm seeing 0% coverage in MobileContextTest and MobileFrontend.hooksTest. The device tests are at 100% on the website (and nearly so locally). Is this expected?

Additionally we should add PHPCS check
I don't believe this has been done.

Correct. This is tracked in T179094 and I'd argue out of scope given the time taken so far. We scoped this to 2hrs and it was unplanned sprint work. I personally think we've done more than enough here. If we prefer we can drop the spike and hours, remove this from the sprint and stall this on T179094.

I'm seeing 0% coverage in MobileContextTest and MobileFrontend.hooksTest. The device tests are at 100% on the website (and nearly so locally). Is this expected?

MobileContextTest is in need of a big refactor and and MobileFrontend.hooksTest is mostly static functions some of which are hard to test. Coverage is low (not zero).

It's easy to go down a rabbit hole with this task, I'd recommend waiting for T179094 and improving as we go. I worry if we invest too much time here, while it's not enforced, we'll end up losing it again. We can chip away at it in the mean time as we touch PHP code. We're already seeing good progress in the transform directory given the refactor there.

What do you think?