Page MenuHomePhabricator

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

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.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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 Normal.
Jhernandez updated the task description. (Show Details)
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.

Jhernandez removed Jhernandez as the assignee of this task.Mar 7 2017, 9:06 AM
Jdlrobson set the point value for this task to 5.
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
bmansurov moved this task from To Do to Doing on the Reading-Web-Sprint-96 board.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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)

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

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

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

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

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!

💥💥💥

Awesome work, y'all 👊

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

^

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

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!

Jdlrobson closed this task as Resolved.May 10 2017, 12:39 PM

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?

@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 good job!

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

@Jhernandez good idea. Let me think about it.

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

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