Page MenuHomePhabricator
Create Task
Maniphest T203818

Add code coverage testing and increase test coverage for some existing mobile.startup files
Closed, ResolvedPublic8 Estimated Story Points
Actions

Description

Our goal is to increase test coverage inside MobileFrontend. We will begin by writing tests for the sources already ported to webpack.

Precursor

  • T203100 has been resolved and original code coverage documented on the wiki

Acceptance criteria

  • increase oo-extend code coverage
  • increase utils code coverage
  • increase browser code coverage

Note: increased but not 100%. Follow up in T206705

  • increase cache code coverage

Set sensible default global threshold for statements, branches, functions and lines (discussed and decided against this)

Developer notes

A patch adding code coverage exists in https://gerrit.wikimedia.org/r/456325

Details

SubjectRepoBranchLines +/-
Fix QUnit appendChild bug with Browser.test.jsmediawiki/extensions/MobileFrontendmaster+40 -20
Fix omitted build files from previous commit.mediawiki/extensions/MobileFrontendmaster+7 -12
Increasing test coverage for mobile.startup/Browser.jsmediawiki/extensions/MobileFrontendmaster+65 -4
Increase test coverage for mobile.startups/util.jsmediawiki/extensions/MobileFrontendmaster+58 -1
Measure code coverage inside MobileFrontendmediawiki/extensions/MobileFrontendmaster+22 -2
Adding unit tests for mobile.startup/mfExtend.jsmediawiki/extensions/MobileFrontendmaster+51 -7
[WIP] Adding tests to mobile.startup/util.jsmediawiki/extensions/MobileFrontendmaster+76 -19
Add unit tests for util.js and mfExtend.jsmediawiki/extensions/MobileFrontendmaster+159 -32
Adds a QUnit test for mobile.startup/cache.jsmediawiki/extensions/MobileFrontendmaster+14 -4
Customize query in gerrit

Related Objects
Search...

Event Timeline

Jdlrobson triaged this task as High priority.Sep 7 2018, 5:09 PM
Jdlrobson created this task.
Jdlrobson moved this task from Incoming to Upcoming on the Web-Team-Backlog board.
Jdlrobson edited projects, added MobileFrontend (MobileFrontend and MinervaNeue architecture); removed MobileFrontend.Sep 7 2018, 5:12 PM
Jdlrobson moved this task from Backlog to Code coverage on the MobileFrontend (MobileFrontend and MinervaNeue architecture) board.
Jdlrobson moved this task from Code coverage to PHP architecture on the MobileFrontend (MobileFrontend and MinervaNeue architecture) board.
Jdlrobson moved this task from PHP architecture to Code coverage on the MobileFrontend (MobileFrontend and MinervaNeue architecture) board.
• Niedzielski added a comment.Sep 7 2018, 7:32 PM

Is this task mostly about enabling coverage with incidental increases or are there tests that should be written as part of this task?

• Niedzielski added a project: Technical-Debt.Sep 7 2018, 7:33 PM
Jdlrobson added a comment.Sep 7 2018, 7:45 PM

This is explicitly about writing missing tests for those files.
Enabling coverage reports is needed to do that.

• Niedzielski updated the task description. (Show Details)Sep 7 2018, 7:49 PM
ovasileva set the point value for this task to 8.Sep 12 2018, 4:38 PM
Jdlrobson moved this task from Upcoming to Readers-Web-Kanbanana-Board-2018-19-Q1 on the Web-Team-Backlog board.Sep 19 2018, 10:32 PM
Jdlrobson edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1); removed Web-Team-Backlog.
Jdlrobson added a comment.EditedSep 19 2018, 10:41 PM

(the todo column was empty, I believe we informally agreed that it should never be empty and this was at the top)

Jdlrobson updated the task description. (Show Details)Sep 19 2018, 11:40 PM
gerritbot subscribed.Sep 20 2018, 10:15 AM

Change 461615 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] [WIP] Adding tests to mobile.startup/util.js

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

gerritbot added a project: Patch-For-Review.Sep 20 2018, 10:15 AM
Jdrewniak claimed this task.Sep 20 2018, 11:45 AM
Jdrewniak moved this task from To Do to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1) board.
gerritbot added a comment.Sep 20 2018, 1:09 PM

Change 461633 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Adds a QUnit test for mobile.startup/cache.js

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

gerritbot added a comment.Sep 20 2018, 8:44 PM

Change 461633 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Adds a QUnit test for mobile.startup/cache.js

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

Jdrewniak added a comment.Sep 24 2018, 10:51 AM
  • increase utils code coverage

Many functions in util.js are wrappers around jQuery methods. Testing these methods basically amounts to testing jQuery, but I don't think that's such a bad thing. util.isNumeric for example, wraps $.isNumeric. This function was changed in jQuery 3.0 to cast strings to numbers, meaning isNumeric("3") now returns true just like isNumeric(3). If this kind of behaviour ever changes in a future version of jQuery, it would still be good to test our assumptions about it in our own code.

Given this kind of tight dependance on jQuery I think it makes sense to use real jQuery in this situation, instead of mock jQuery (as discussed with the team previously).

  • increase oo-extend code coverage

mfExtend.js is also a wrapper, this one around OO.inheritClass OO.initClass and kinda OO.mixinClass. Testing this function also amounts to testing these OO functions, which, like the situation above, would require using the real library (or at least parts of it) instead of a mock, IMO.

gerritbot added a comment.Sep 24 2018, 12:00 PM

Change 462441 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Adding unit tests for mobile.startup/mfExtend.js

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

Jdlrobson added a comment.EditedSep 25 2018, 6:07 PM

Is https://gerrit.wikimedia.org/r/462441 ready for review (or are you still "doing")?

gerritbot added a comment.Oct 1 2018, 12:02 PM

Change 463738 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Add unit tests for util.js

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

Jdrewniak moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1) board.Oct 2 2018, 5:16 PM
ovasileva moved this task from Readers-Web-Kanbanana-Board-2018-19-Q1 to Readers-Web-Kanbanana-Board-2018-19-Q2 on the Web-Team-Backlog board.Oct 2 2018, 5:29 PM
ovasileva edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2); removed Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1).
ovasileva moved this task from To Do to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 2 2018, 5:36 PM
Jdrewniak removed Jdrewniak as the assignee of this task.Oct 2 2018, 6:51 PM
Jdrewniak moved this task from Needs Code Review to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.
Jdrewniak subscribed.
gerritbot added a comment.Oct 2 2018, 7:02 PM

Change 463738 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add unit tests for util.js and mfExtend.js

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

gerritbot added a comment.Oct 2 2018, 8:35 PM

Change 461615 abandoned by Jdrewniak:
[WIP] Adding tests to mobile.startup/util.js

Reason:
superseded by https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/ /463738/

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

gerritbot added a comment.Oct 2 2018, 8:36 PM

Change 462441 abandoned by Jdrewniak:
Adding unit tests for mobile.startup/mfExtend.js

Reason:
superseded by https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/ /463738/

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

Jdlrobson moved this task from Ready for Signoff to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 2 2018, 8:40 PM

Looks like https://gerrit.wikimedia.org/r/456325 hasn't been merged and "Set sensible default global threshold for statements, branches, functions and lines" remains so cannot sign off just yet (feel free to pick up that patch).

Jdlrobson moved this task from Needs More Work to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 3 2018, 5:30 PM
Jdlrobson moved this task from Ready for Signoff to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.
gerritbot added a comment.Oct 3 2018, 5:31 PM

Change 456325 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Measure code coverage inside MobileFrontend

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

Jdrewniak claimed this task.Oct 4 2018, 11:29 AM
Jdrewniak removed Jdrewniak as the assignee of this task.
gerritbot added a comment.Oct 4 2018, 10:33 PM

Change 456325 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Measure code coverage inside MobileFrontend

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

Jdlrobson claimed this task.Oct 4 2018, 10:37 PM
Jdlrobson moved this task from Needs Code Review to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.
Jdlrobson updated the task description. (Show Details)

Will sign off

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-10-16 (1.32.0-wmf.26)).Oct 4 2018, 11:01 PM
Jdlrobson updated the task description. (Show Details)Oct 5 2018, 5:52 PM
Jdlrobson moved this task from Ready for Signoff to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.
File         |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------|----------|----------|----------|----------|-------------------|
All files    |    66.67 |    51.11 |    59.02 |    67.01 |                   |
 Browser.js  |    58.14 |    46.67 |    38.46 |    58.14 |... 61,162,163,165 |
 View.js     |    91.53 |    76.92 |    86.67 |    91.53 |... 05,207,208,367 |
 cache.js    |      100 |      100 |       50 |      100 |                   |
 mfExtend.js |      100 |      100 |      100 |      100 |                   |
 modules.js  |       72 |       50 |    66.67 |       72 |... 48,78,94,96,99 |
 time.js     |    24.24 |       10 |    16.67 |    24.24 |... 34,136,138,139 |
 util.js     |       50 |      100 |    64.29 |    52.63 |... 0,62,71,80,158 |

utils.js does not have 100% and browser does not have 100% per the acceptance criteria.

Jdlrobson removed Jdlrobson as the assignee of this task.Oct 5 2018, 6:10 PM
Jdlrobson updated the task description. (Show Details)
gerritbot added a comment.Oct 8 2018, 11:56 AM

Change 465147 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Increase test coverage for mobile.startups/util.js

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

gerritbot added a comment.Oct 8 2018, 1:59 PM

Change 465147 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Increase test coverage for mobile.startups/util.js

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

Jdrewniak updated the task description. (Show Details)Oct 8 2018, 2:00 PM
gerritbot added a comment.Oct 8 2018, 3:54 PM

Change 465186 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Increasing test coverage for mobile.startup/Browser.js

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

Jdlrobson moved this task from Needs More Work to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 8 2018, 5:11 PM
Jdlrobson claimed this task.Oct 8 2018, 5:54 PM

I'll review this today

Jdlrobson moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 8 2018, 6:35 PM
Jdrewniak moved this task from Needs More Work to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 9 2018, 5:03 PM
gerritbot added a comment.Oct 9 2018, 6:58 PM

Change 465186 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Increasing test coverage for mobile.startup/Browser.js

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

Jdlrobson moved this task from Needs Code Review to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 9 2018, 7:16 PM
Jdlrobson added a comment.Oct 9 2018, 7:18 PM

Seeing the following coverage:
util.js | 100 | 100 | 100 | 100
mfExtend.js | 100 | 100 | 100 | 100 |
cache.js | 100 | 100 | 50 | 100 | |
Browser.js | 79.07 | 70 | 84.62 | 79.07

@Jdrewniak for prosperity, for the latter 2, could you expand on why it's impossible to get 100% here? If we wanted to achieve that what do we need to do?

Jdlrobson moved this task from Ready for Signoff to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 10 2018, 8:03 PM
Jdrewniak added a comment.Oct 10 2018, 8:43 PM

@Jdlrobson well I would hate to say that something is impossible.

I've been exploring the NYC coverage tool and discovered that it can produce pretty nice HTML reports (by changing the npm coverage script to nyc --reporter=html npm -s run test:unit).The output of these reports can give us more insight into what is and isn't being tested.

Here's the output as of the latest patch https://people.wikimedia.org/~jdrewniak/coverage-report/. As you can see in Browser.js, the lines that aren't run relate to: the memoized function, iOS version, and getSingleton method.

Knowing this now, I think we can increase the code-coverage of the Browser.js tests.

In the cache.js example however, the functions that aren't tested are basically noops, so I don't think that would benefit from additional tests.

Jdlrobson closed this task as Resolved.Oct 10 2018, 9:38 PM
Jdlrobson mentioned this in T206705: MFA: Increase test coverage of Browser.js.
Jdlrobson moved this task from Needs More Work to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.

Thanks for the follow up.
Let's address the Browser.js test coverage separately (T206705)
I've made a note of cache.js

Jdlrobson updated the task description. (Show Details)Oct 10 2018, 9:39 PM
gerritbot added a comment.Oct 11 2018, 5:28 PM

Change 466701 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Fix omitted build files from previous commit.

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

Jdlrobson reopened this task as Open.Oct 11 2018, 5:58 PM
gerritbot added a comment.Oct 11 2018, 7:05 PM

Change 466701 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix omitted build files from previous commit.

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

gerritbot added a comment.Oct 11 2018, 9:16 PM

Change 466785 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Fix QUnit appendChild bug with Browser.test.js

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

gerritbot added a comment.Oct 11 2018, 10:25 PM

Change 466785 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix QUnit appendChild bug with Browser.test.js

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

Jdlrobson closed this task as Resolved.Oct 17 2018, 8:03 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits