Page MenuHomePhabricator
Create Task
Maniphest T133022

Unit test src/renderer.js
Closed, ResolvedPublic5 Estimated Story Points
Actions

Description

Unit test src/renderer.js, which was migrated from the old version of Popups.

It doesn't have any code coverage.

Notes

  • Review and merge in, if necessary, the ext.Popups.PreviewBehavior abstraction since it's very closely related to the renderer.

Details

Show related patches Customize query in gerrit

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ovasileva added a parent task: T144522: [EPIC] Hovercards technical debt and quality enhancements.Oct 21 2016, 9:23 AM
ovasileva moved this task from Backlog to Needs Analysis on the Page-Previews board.Dec 20 2016, 12:37 AM
Jdlrobson assigned this task to • Jhernandez.Feb 14 2017, 9:45 PM

I think this is no longer relevant? Can we resolve as invalid @Jhernandez ? #rewrite

Jdlrobson mentioned this in T67103: [EPIC] Hovercards: Test Coverage.Feb 16 2017, 6:49 PM
Jdlrobson moved this task from Triaged but Future to Needs Prioritization on the Web-Team-Backlog board.Feb 16 2017, 6:54 PM
• Jhernandez added a comment.Feb 17 2017, 6:59 PM

This is still valid since the renderer was basically ported without touching it, and not refactored (it was organized a bit, but just that).

See src/renderer.js

I'll update description.

• Jhernandez renamed this task from Unit test ext.popups.renderer.article.js (part 1 EZ) to Unit test src/renderer.js.Feb 17 2017, 7:01 PM
• Jhernandez raised the priority of this task from Low to Medium.
• Jhernandez updated the task description. (Show Details)
• Jhernandez merged a task: T133162: Unit test ext.popups.renderer.article.js (part 2 HARD).
• Jhernandez removed a subscriber: • jhobs.
• Jhernandez added a subscriber: • jhobs.

Changed to normal since it is what Olga set the other one I've just merged to.

phuedx removed a project: Unplanned-Sprint-Work.Feb 23 2017, 6:47 PM
ovasileva moved this task from Needs Prioritization to Triaged but Future on the Web-Team-Backlog board.Mar 6 2017, 4:08 PM
ovasileva moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.Mar 6 2017, 4:17 PM
• Jhernandez removed • Jhernandez as the assignee of this task.Mar 7 2017, 9:06 AM
ovasileva added a project: Reading-Web-Sprint-94.Mar 7 2017, 12:01 PM
Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-94 board.Mar 8 2017, 5:42 PM
Jdlrobson set the point value for this task to 5.
ovasileva removed a project: Reading-Web-Sprint-94.Mar 14 2017, 10:39 PM
phuedx added a comment.EditedApr 6 2017, 10:56 AM

This task could also be extended to include reviewing and merging in, if necessary, the ext.Popups.PreviewBehavior abstraction too, since it's very closely related to the renderer.

phuedx updated the task description. (Show Details)Apr 6 2017, 10:57 AM
ovasileva added a project: Reading-Web-Sprint-96.Apr 13 2017, 6:46 PM
Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-96 board.Apr 19 2017, 5:53 PM
ovasileva moved this task from Upcoming to 2016-17 Q4 on the Web-Team-Backlog board.Apr 21 2017, 3:53 PM
• bmansurov claimed this task.Apr 27 2017, 2:48 PM
• bmansurov moved this task from To Do to Doing on the Reading-Web-Sprint-96 board.
gerritbot subscribed.Apr 27 2017, 4:07 PM

Change 350591 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Bring back renderer#getClosestYPosition tests

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

gerritbot added a project: Patch-For-Review.Apr 27 2017, 4:07 PM
gerritbot added a comment.Apr 27 2017, 7:58 PM

Change 350657 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createPokeyMasks

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

gerritbot added a comment.Apr 27 2017, 9:45 PM

Change 350747 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Bring back renderer#renderExtract tests

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

gerritbot added a comment.May 1 2017, 4:41 PM

Change 351160 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#getClasses

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

gerritbot added a comment.May 1 2017, 5:25 PM

Change 350591 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Bring back renderer#getClosestYPosition tests

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-05-09_(1.30.0-wmf.1)).May 1 2017, 6:00 PM
gerritbot added a comment.May 1 2017, 6:42 PM

Change 351173 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnailElement

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

gerritbot added a comment.May 1 2017, 7:13 PM

Change 350657 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createPokeyMasks

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

gerritbot added a comment.May 2 2017, 8:20 PM

Change 351383 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#SIZES

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

gerritbot added a comment.May 2 2017, 8:20 PM

Change 351384 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createEmptyPreview

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

gerritbot added a comment.May 2 2017, 9:30 PM

Change 351518 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createPreview

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

gerritbot added a comment.May 2 2017, 9:46 PM

Change 351524 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnail

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

gerritbot added a comment.May 3 2017, 12:22 AM

Change 351558 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#hide

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

gerritbot added a comment.May 3 2017, 3:18 PM

Change 351657 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#bindBehavior

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

gerritbot added a comment.May 3 2017, 3:18 PM

Change 351658 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#show

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

gerritbot added a comment.May 3 2017, 4:22 PM

Change 350747 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Bring back renderer#renderExtract tests

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

gerritbot added a comment.May 3 2017, 4:24 PM

Change 351160 merged by Bmansurov:
[mediawiki/extensions/Popups@master] QA: Test renderer#getClasses

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

gerritbot added a comment.May 3 2017, 4:33 PM

Change 351173 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnailElement

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

gerritbot added a comment.May 3 2017, 5:19 PM

Change 351383 abandoned by Bmansurov:
QA: Test renderer#SIZES

Reason:
OK, np, I'll abandon this patch and rebase others.

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

gerritbot added a comment.May 3 2017, 9:27 PM

Change 351384 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createEmptyPreview

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

gerritbot added a comment.May 4 2017, 10:32 AM

Change 351518 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createPreview

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

• Jhernandez added a subscriber: pmiazga.May 4 2017, 10:37 AM

From @pmiazga in QA: Test renderer#createThumbnail

Missing test cases for

  • image too small for landscape/portrait display
  • missing x&y calculation tests (both cases when width/height are bigger or smaller than the one defined in SIZES constant)
gerritbot added a comment.May 4 2017, 10:41 AM

Change 351524 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnail

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

gerritbot added a comment.May 4 2017, 11:05 AM

Change 351558 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#hide

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

• Jhernandez added a comment.May 4 2017, 11:14 AM

Last two patches need some work, and left a comment there T133022#3234696.

Thanks a lot @bmansurov for working on this! With your patches, on the last one on #show, we're at more than 90% of test coverage!

91.11% Statements 564/619 86.1% Branches 322/374 85.12% Functions 103/121 91.11% Lines 564/619

See http://popups-coverage.surge.sh/lcov-report/index.html

This is amazing!

phuedx added a comment.May 4 2017, 12:26 PM

💥💥💥

Awesome work, y'all 👊

Jdlrobson added a project: Readers-Web-Kanbanana-Board-Old.May 4 2017, 5:33 PM
Jdlrobson moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.
gerritbot added a comment.May 4 2017, 8:46 PM

Change 351657 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#bindBehavior

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

gerritbot added a comment.May 4 2017, 10:19 PM

Change 352048 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Improve renderer#createThumbnail tests

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

• bmansurov added a comment.May 4 2017, 10:20 PM

https://gerrit.wikimedia.org/r/#/c/352048/ addresses the comment at T133022#3234696.

gerritbot added a comment.May 5 2017, 6:21 PM

Change 352184 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] Refactor and test renderer#createLayout

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

gerritbot added a comment.May 5 2017, 6:32 PM

Change 352048 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Improve renderer#createThumbnail tests

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

gerritbot added a comment.May 5 2017, 7:38 PM

Change 352190 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#layoutPreview

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

gerritbot added a comment.May 5 2017, 10:46 PM

Change 352184 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Refactor and test renderer#createLayout

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

gerritbot added a comment.May 8 2017, 8:44 PM

Change 352681 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#show

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

• bmansurov moved this task from Doing to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.May 8 2017, 8:45 PM
gerritbot added a comment.May 9 2017, 2:30 PM

Change 351658 abandoned by Bmansurov:
QA: Test renderer#show

Reason:
See https://gerrit.wikimedia.org/r/#/c/352681/

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

gerritbot added a comment.May 9 2017, 9:17 PM

Change 352190 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#layoutPreview

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

gerritbot added a comment.May 9 2017, 9:46 PM

Change 352681 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#show

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

ReleaseTaggerBot edited projects, added MW-1.30-release-notes (WMF-deploy-2017-05-23_(1.30.0-wmf.2)); removed MW-1.30-release-notes (WMF-deploy-2017-05-09_(1.30.0-wmf.1)).May 9 2017, 10:00 PM
phuedx moved this task from Needs Code Review to Ready for Signoff on the Readers-Web-Kanbanana-Board-Old board.May 10 2017, 8:58 AM

^

22:44:30 <bmansurov> you may move the task to sign off if you wish

pmiazga added a comment.May 10 2017, 10:58 AM

We have an excellent coverage for all logic inside renderer.js. only couple one-line-functions are not tested.

Coverage report:

/src/renderer.js
96.53% Statements (139/144 )
94.96% Branches (113/119 )
82.61% Functions (19/23) 
96.53% Lines (139/144)

Would be nice to add just two missing test scenarios:

  • createLayout() when isPreviewTall=false && offsetLeft > ( windowData.width / 2 ) ) -- line 537, branch not covered
  • createThumbnail() when image is tall and thumbHeight is bigger than SIZES.portraitImage.h

@bmansurov good job!

pmiazga moved this task from Ready for Signoff to Needs More Work on the Readers-Web-Kanbanana-Board-Old board.May 10 2017, 10:59 AM
Jdlrobson closed this task as Resolved.May 10 2017, 12:39 PM
Jdlrobson moved this task from Needs More Work to Ready for Signoff on the Readers-Web-Kanbanana-Board-Old board.

Putting my tech lead hat on and considering we originally committed to 5 points and this was one of the lower priorities of the sprint I think Baha has done more than enough here. Test coverage is great and Baha's put in a lot of good work here.

@pmiazga please can you open a new task for the remaining issues?

pmiazga added a comment.May 10 2017, 1:34 PM

@Jdlrobson will do

• Jhernandez added a comment.May 10 2017, 6:13 PM

@bmansurov Besides coverage, (it is just a number), and after this work, are there specific logic or scenarios that would be good to add tests for? Would you mind writing them down if you can think of them so that we can keep the info in a task for future work? It would be good to write those things down now that the information is fresh.

• bmansurov added a comment.May 11 2017, 12:48 AM
In T133022#3251088, @pmiazga wrote:

@bmansurov good job!

Indeed, you did at least half the work. Good job to you, @pmiazga.

@Jhernandez good idea. Let me think about it.

pmiazga mentioned this in T165036: Refactor rendering code in src/renderer.js for readability and testability.May 11 2017, 12:36 PM
• bmansurov added a comment.May 16 2017, 10:09 PM

@Jhernandez upon your request, here are some thoughts:

The show function, for example, is responsible for too many things. It is too complicated as it accepts seemingly unrelated parameters, creates a layout, layouts a preview, binds some behavior, etc. Binding a behavior as part of showing a preview hasn't been tested. In general functions make use of other functions in the file without making them explicit dependencies, unlike data passed to functions. Passing dependent functions to other functions is also not practical as the function signature gets even more complicated.

The createThumbnail function has two many code paths. It takes enourmous effort in order to understand the function correctly and change something in it. Although tests try to cover all cases, we are only creating one case for each code path. getClasses is another such function.

What I'd like to see is we break up functions into small chunks that do one thing well and we test those funtions with random input. Testing monstrous functions with random input will be a major pain. I suggest we use a property-based testing library, perhaps JSVerify, for generating random test data.

• Jhernandez mentioned this in T165573: Improve design and tests on src/renderer.js.May 17 2017, 10:17 AM

Thanks @bmansurov, I see. Lots of ifs and hidden logic in those functions indeed.

I've created T165573: Improve design and tests on src/renderer.js to capture your thoughts so that we get to it some day.

• bmansurov removed • bmansurov as the assignee of this task.Jun 22 2017, 1:06 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