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.
Unit test src/renderer.js, which was migrated from the old version of Popups.
It doesn't have any code coverage.
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.
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.
Change 350591 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Bring back renderer#getClosestYPosition tests
Change 350657 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createPokeyMasks
Change 350747 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Bring back renderer#renderExtract tests
Change 351160 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#getClasses
Change 350591 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Bring back renderer#getClosestYPosition tests
Change 351173 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnailElement
Change 350657 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createPokeyMasks
Change 351383 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#SIZES
Change 351384 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createEmptyPreview
Change 351518 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createPreview
Change 351524 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnail
Change 351558 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#hide
Change 351657 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#bindBehavior
Change 351658 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#show
Change 350747 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Bring back renderer#renderExtract tests
Change 351160 merged by Bmansurov:
[mediawiki/extensions/Popups@master] QA: Test renderer#getClasses
Change 351173 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createThumbnailElement
Change 351383 abandoned by Bmansurov:
QA: Test renderer#SIZES
Reason:
OK, np, I'll abandon this patch and rebase others.
Change 351384 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createEmptyPreview
Change 351518 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#createPreview
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
Change 351558 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#hide
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!
Change 351657 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#bindBehavior
Change 352048 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Improve renderer#createThumbnail tests
Change 352184 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] Refactor and test renderer#createLayout
Change 352048 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Improve renderer#createThumbnail tests
Change 352190 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#layoutPreview
Change 352184 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Refactor and test renderer#createLayout
Change 352681 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] QA: Test renderer#show
Change 351658 abandoned by Bmansurov:
QA: Test renderer#show
Reason:
See https://gerrit.wikimedia.org/r/#/c/352681/
Change 352190 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#layoutPreview
Change 352681 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] QA: Test renderer#show
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:
@bmansurov good job!
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.
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.