Page MenuHomePhabricator

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

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

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 Readers-Web-Backlog board.

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

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

ovasileva set the point value for this task to 8.Sep 12 2018, 4:38 PM
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

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

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

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

  • 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.

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")?

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

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

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

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

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).

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 removed Jdrewniak as the assignee of this task.

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

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

Jdlrobson updated the task description. (Show Details)Oct 5 2018, 5:52 PM
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)

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

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

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 claimed this task.Oct 8 2018, 5:54 PM

I'll review this today

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

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

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 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

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

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

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

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

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

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