Closed
Bug 979636
Opened 8 years ago
Closed 4 years ago
Unit tests for video app
Categories
(Firefox OS Graveyard :: Gaia::Video, defect)
Firefox OS Graveyard
Gaia::Video
Tracking
(Not tracked)
RESOLVED
WONTFIX
1.4 S3 (14mar)
People
(Reporter: rnicoletti, Assigned: rnicoletti)
References
Details
Attachments
(7 files, 2 obsolete files)
46 bytes,
text/x-github-pull-request
|
johnhu
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
johnhu
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
johnhu
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
johnhu
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
johnhu
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
83.50 KB,
application/vnd.ms-excel
|
Details |
The following list shows the js files for video app and whether they have unit tests: db.js --> no metadata.js --> no thumbnail_date_group.js --> yes thumbnail_item.js --> yes thumbnail_list.js --> yes video.js --> yes (only one function: showInfoView) video_utils.js --> yes view.js --> no Of the files with no unit tests, the following is the order of importance in terms of having unit tests (ranking based on responsibility and amount of logic) 1) video.js 2) metadata.js 3) view.js 4) db.js
Assignee | ||
Comment 1•8 years ago
|
||
Adding unit tests to video.js. Prioritization: focus on the functions with the most responsibility and logic (conditional expressions) first. The top 5, not necessarily in this order, are: 1) showPlayer 2) hidePlayer 3) handleScreenLayoutChange 4) toggleVideoControls 5) updateSelection
Assignee | ||
Comment 2•8 years ago
|
||
Unit testing showPlayer function. + In order to more easily test the logic in the anonymous, inline callback function declared as a parameter to setVideoUrl, move the logic to a separate function and unit test it separately + In order to test the logic in the private function 'doneSeeking' without having to mimic the user seeking within a video, move it out of showPlayer so it is no longer a private function and unit test it separately
Assignee | ||
Comment 3•8 years ago
|
||
High-level test-cases for showPlayer function: * when device is tablet, changing screen orientation to landscape + show, do not play, current video + do not show fullscreen + do not auto-hide controls * when device is tablet, screen orientation is landscape, cancelling "thumbnail select view" + show, do not play, current video + do not show fullscreen + do not auto-hide controls * when parsing metadata for new files is complete and there is at least one video + show, do not play, current video + do not show fullscreen + do not auto-hide controls * clicking on thumbnail + play or show video - during pick activity: show - not during a pick activity: play + show fullscreen or not - during pick activity or on phone or on tablet in portrait orientation: fullscreen - not during pick activity or not on table in portrait orientation: not fullscreen + auto-hide controls - during pick activity: don't auto-hide controls - not during a pick activity: auto-hide controls * when device is tablet in landscape orientation, after deleting the video being shown in the player, display video next in thumbnail list + don't play + show fullscreen + don't auto-hide controls
Assignee | ||
Comment 4•8 years ago
|
||
High-level test-cases for hidePlayer * on tablet, changing screen orientation to portrait + hide player and update metadata of current video - update bookmark, poster, currentTime, watched attributes - update poster and watched attribute of thumbnail + no callback * entering select view on tablet in landscape orientation + hide player and update metadata of current video - update bookmark, poster, currentTime, watched attributes - update poster and watched attribute of thumbnail + call callback - switch layout to select mode - set thumbnail list select mode - un-select any selections * on tablet, clicking on thumbnail + hide player and update metadata of current video - update bookmark, poster, currentTime, watched attributes - update poster and watched attribute of thumbnail + call callback - wait for metadata parsing to finish - show player * on phone or on tablet in portrait orientation, after deleting the video being shown in the player + hide player, do not update metadata of current video + no callback * on phone or on tablet in portrait orientation, user clicks on close button (is close button only shown on tablet in landscape?) + hide player and update metadata of current video - update bookmark, poster, currentTime, watched attributes - update poster and watched attribute of thumbnail + no callback * after user selects video during a pick activity (attach video to email, sms, etc) OR user cancels a video pick activity + hide player, do not update metadata of current video + no callback * when user cancels a video pick activity where an overlay is displayed and the user clicks on overlay cancel button + hide player, do not update metadata of current video + no callback
Assignee | ||
Comment 5•8 years ago
|
||
handleScreenLayoutChange test-cases * if on tablet, rotating to landscape and file system scanning/metadata parsing is in progress + show spinnerOverlay + disable playerView * if on tablet, rotating to portrait or rotating to landscape and file system scanning/metadata parsing is finished + hide spinnerOverlay + enable playerView * if on tablet, rotating to portrait and in thumbnail list layout + hidePlayer() * if on tablet, rotating to landscape and in thumbnail list layout + showPlayer() * if on tablet rotating to portrait + set max thumbnail title lines to 4 * if on tablet rotating to landscape + set max thumbnail title lines to 2 * when screen orientation changes and current layout mode is thumbnail list of selection + update thumbnail titles based on max title lines and length of title * when screen orientation changes and current layout mode is fullscreen + schedule update of thumbnail titles when layout mode is changed away from fullscreen * when screen orientation changes and video has at least started to load 'if (dom.player.readyState !== HAVE_NOTHING)' + adjust video to fit the current screen orientation
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
toggleVideoControls test-cases * when a 'videoControls' event is fired and an auto-hide timeout is pending + clear the auto-hide timeout * when a 'videoControls' event is fired while in the middle of a pick activity + do nothing * when a 'videoControls' event is fired while not in the middle of a pick activity and the video controls are not shown + show video controls + cancel bubbling of event * when a 'videoControls' event is fired while not in the middle of a pick activity and the video controls are shown + hide video controls
Assignee | ||
Comment 7•8 years ago
|
||
updateSelection test-cases * When in selection view, no thumbnails are currently selected, clicking on a thumbnail + select thumbnail (add 'selected' class to the thumbnail element) + add name of video corresponding to thumbnail to the list of selected thumbnails (should be one name in list) + get video blob from db and associate it with the name of the selected video (should be one entry in map) + Set the text content of the thumbnails-number-selected element to the number of selected thumbnails (1) + enable the thumbnails delete button + enable the thumbnails share button * When in selection view, one thumbnail is currently selected, clicking on the selected thumbnail + de-select clicked onthumbnail (remove 'selected' class from the thumbnail element) + remove the name of the video corresponding to thumbnail from the list of selected thumbnails (now the list should be empty) + remove the video blob corresponding to the selected video from the map (map should now be empty) + Set the text content of the thumbnails-number-selected element to the number of selected thumbnails (0) + disable the thumbnails delete button + disable the thumbnails share button * When in selection view, one thumbnail is currently selected, clicking on another thumbnail + select clicked on thumbnail that is clicked on (add 'selected' class to the thumbnail element) + add name of video corresponding to thumbnail to the list of selected thumbnails (should be two names in list) + get video blob from db and associate it with the name of the selected video (should be two entries in map) + Set the text content of the thumbnails-number-selected element to the number of selected thumbnails (2) + enable the thumbnails delete button + enable the thumbnails share button * When in selection view, two thumbnails are currently selected, clicking on one of the selected thumbnails + de-select clicked on thumbnail (remove 'selected' class from the thumbnail element) + remove the name of the video corresponding to the clicked on thumbnail from the list of selected thumbnails (should now be one name in list) + remove the video blob corresponding to the selected video from the map (should be one entry in map) + Set the text content of the thumbnails-number-selected element to the number of selected thumbnails (1) + enable the thumbnails delete button + enable the thumbnails share button
Assignee | ||
Comment 8•8 years ago
|
||
Hi John, Can you review my patch for adding some unit tests to video.js? It's a small patch.
Attachment #8390278 -
Flags: review?(johu)
Comment 9•8 years ago
|
||
Comment on attachment 8390278 [details] [review] Pull request https://github.com/mozilla-b2g/gaia/pull/17142 Thanks for doing this. It is fine to land this part. BTW, you only write the test case for updateDialog. But you said much about others. How about the others?
Attachment #8390278 -
Flags: review?(johu) → review+
Assignee | ||
Comment 10•8 years ago
|
||
John, I am starting with showPlayer. updateDialog and showOverlay are called by showPlayer so I wrote tests for those functions to ensure good coverage for showPlayer.
Assignee | ||
Comment 11•8 years ago
|
||
Hi David, Can I land these video.js unit tests in 1.4? There is no refactoring associated with these tests.
Flags: needinfo?(dflanagan)
Assignee | ||
Comment 12•8 years ago
|
||
I went ahead and merged this as there was no refactoring to the app code, only unit tests were added.
Flags: needinfo?(dflanagan)
Assignee | ||
Comment 13•8 years ago
|
||
Please see comment on PR for an explanation regarding an oddity of the unit tests due to dom.player.seeking always being true. David, if John can review I'm thinking your review would be optional. If you would like to review even if John also reviews, please let me know. Thanks.
Attachment #8410668 -
Flags: review?(johu)
Attachment #8410668 -
Flags: review?(dflanagan)
Comment 14•8 years ago
|
||
Comment on attachment 8410668 [details] [review] Pull request https://github.com/mozilla-b2g/gaia/pull/18546 I feel we have some space to improve these unit tests. Please check the comments at PR. For the question about seeking, I don't know why it is always seeking. But we may need to create a mocked video element and wire it as "dom.player" so that we can change every state we want. An example can be found at shared/test/unit/mocks/mock_audio.js[1] and its usage can be found here[2]. [1] https://github.com/mozilla-b2g/gaia/blob/4da57191b7e3ca153a22d017068f3332f9dcf580/shared/test/unit/mocks/mock_audio.js [2] https://github.com/mozilla-b2g/gaia/blob/4da57191b7e3ca153a22d017068f3332f9dcf580/apps/sms/test/unit/notify_test.js
Attachment #8410668 -
Flags: review?(johu) → review-
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #14) > Comment on attachment 8410668 [details] [review] > Pull request https://github.com/mozilla-b2g/gaia/pull/18546 > > I feel we have some space to improve these unit tests. Please check the > comments at PR. > > For the question about seeking, I don't know why it is always seeking. But > we may need to create a mocked video element and wire it as "dom.player" so > that we can change every state we want. An example can be found at > shared/test/unit/mocks/mock_audio.js[1] and its usage can be found here[2]. > > [1] > https://github.com/mozilla-b2g/gaia/blob/ > 4da57191b7e3ca153a22d017068f3332f9dcf580/shared/test/unit/mocks/mock_audio.js > [2] > https://github.com/mozilla-b2g/gaia/blob/ > 4da57191b7e3ca153a22d017068f3332f9dcf580/apps/sms/test/unit/notify_test.js Thanks for the review, John. I've refactored the code to address most of your comments, and I've updated the PR to reflect what I've done. Regarding creating a MockVideo similar to MockAudio, I'm not sure how to do that because my understanding is window.Video needs to be an HTML Video Element. If I simply create a MockVideo object, it will only be an Object, not an HTML Video Element, correct? I'm not sure how to create a mock HTML Video Element. I would appreciate your thoughts/advice on that. Thanks.
Flags: needinfo?(johu)
Assignee | ||
Comment 16•8 years ago
|
||
Correction. Regarding comment #14, what I meant to say was that my understand is dom.player, not window.Video, has be be an HTML Video Element.
Comment 17•8 years ago
|
||
The dom.player variable is assigned by document.getElementById() at video.js. We may modify it when video.js is loaded where is the callback of requireApp, like: requireApp('video.js', function callbacked(){ ... }); After that, we may replace dom.player as an instance of MockVideo and we can control everything. It isn't matter about HTML Video Element or JS mocked object because we use API calls to dom.player. It is viewed as an object with HTML Video Element API[1] implemented. In this case, we don't need to implement all APIs but APIs used by video.js. So, we don't need to implement the APIs, like audioTracks, textTracks, videoTracks, mozLoadFrom, canPlayType (only used at metadata.js), mozGetMetadata (only used at metadata.js), etc. What we need to do is to give those unused API a skeleton. That may consume times to type those APIs but it will be very useful at the future testing case. [1] https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement BTW, I had checked some of your replies at PR and put my comments there, too.
Flags: needinfo?(johu)
Comment 18•8 years ago
|
||
Comment on attachment 8410668 [details] [review] Pull request https://github.com/mozilla-b2g/gaia/pull/18546 Removing the review flag for myself. John's review will be sufficient. Thanks, John!
Attachment #8410668 -
Flags: review?(dflanagan)
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] OOO from 5/1~5/4 from comment #17) > The dom.player variable is assigned by document.getElementById() at > video.js. We may modify it when video.js is loaded where is the callback of > requireApp, like: requireApp('video.js', function callbacked(){ ... }); > After that, we may replace dom.player as an instance of MockVideo and we can > control everything. > > It isn't matter about HTML Video Element or JS mocked object because we use > API calls to dom.player. It is viewed as an object with HTML Video Element > API[1] implemented. In this case, we don't need to implement all APIs but > APIs used by video.js. So, we don't need to implement the APIs, like > audioTracks, textTracks, videoTracks, mozLoadFrom, canPlayType (only used at > metadata.js), mozGetMetadata (only used at metadata.js), etc. What we need > to do is to give those unused API a skeleton. That may consume times to type > those APIs but it will be very useful at the future testing case. > > [1] https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement > > BTW, I had checked some of your replies at PR and put my comments there, too. I updated the PR to include changes to address some of your review comments. I have kept the second commit so you can see my recent changes. Regarding dom.player being an HTMLVideoElement or JS mocked object, I understand the concept and benefits of mocking the video element. I should have been more specific with my previous concern/question. My concern/question is regarding setVideoUrl where it sets 'dom.player.onloadedmetadata' to the anonymous callback function defined in showPlayer and then sets 'dom.player.src' to the video blob url. As I understand it, setting dom.player.src causes the video to be loaded which will result in the onloadedmetadata function being called. And I believe this will only occur if dom.player is an HTMLVideoElement. I could be completely wrong about this, but this is what I observed when I set 'dom.player' to my MockVideo object: the setVideoUrl callback function was never invoked during the tests. I will continue to review your comments and make changes accordingly.
Flags: needinfo?(johu)
Assignee | ||
Comment 20•8 years ago
|
||
What I've done as a POC for one of the tests is to assign dom.player to MockVideo and then after invoking showPlayer the test now explicitly calls dom.player.onloadmetadata() to simulate the video being loaded, which invokes the setVideoUrl callback in showPlayer. John, I'm assuming this is what you had in mind when you suggested using MockVideo. I will proceed with this strategy for the other tests. John, leaving NI for you to confirm this strategy. No rush.
Assignee | ||
Comment 21•8 years ago
|
||
John, I have updated the PR: https://github.com/mozilla-b2g/gaia/pull/18546. I believe these changes address your review comments. Please have a look. Thanks.
Comment 22•8 years ago
|
||
Russ, Sorry for late reviewing. Please find the comment at PR. Most of them are cool. We may need to change the existing tests to use MockMediaDB.
Flags: needinfo?(johu)
Assignee | ||
Comment 23•8 years ago
|
||
John, I've addressed your latest review comments. I added this latest commit separately to the PR so you can review only the latest changes. I will squash before landing. Thanks in advance for another review.
Flags: needinfo?(johu)
Comment 25•8 years ago
|
||
Comment on attachment 8410668 [details] [review] Pull request https://github.com/mozilla-b2g/gaia/pull/18546 Thanks. Please squash and rebase the code before landing.
Attachment #8410668 -
Flags: review- → review+
Assignee | ||
Comment 26•8 years ago
|
||
Landed showPlayer tests in master: https://github.com/mozilla-b2g/gaia/commit/101c500903a2477f9de1ea5ce523b9e0be4d45d0
Assignee | ||
Comment 27•8 years ago
|
||
John, I've created unit tests for showPlayer. Please let me know your comments. Thanks for all your reviews!
Attachment #8424226 -
Flags: review?(johu)
Assignee | ||
Comment 28•8 years ago
|
||
Correction: in comment #27, 'showPlayer' should be 'hidePlayer'
Comment 29•8 years ago
|
||
Comment on attachment 8424226 [details] [review] Pull request: https://github.com/mozilla-b2g/gaia/pull/19338 Thanks for this patch. It looks good. Please find a nit at the PR.
Attachment #8424226 -
Flags: review?(johu) → review+
Assignee | ||
Comment 30•8 years ago
|
||
hidePlayer, master: https://github.com/russnicoletti/gaia/commit/480fea09349563fd7a14f687782678b80f6eaa8d
Assignee | ||
Comment 31•8 years ago
|
||
Attachment #8443750 -
Flags: review?(johu)
Assignee | ||
Comment 32•8 years ago
|
||
John, will you be able to review the handleScreenLayoutChange unit tests? If not, that's ok, I'll ask djf.
Flags: needinfo?(johu)
Comment 33•8 years ago
|
||
Comment on attachment 8443750 [details] [review] handleScreenLayoutChange tests PR: https://github.com/mozilla-b2g/gaia/pull/20817 Thanks. It looks good.
Attachment #8443750 -
Flags: review?(johu) → review+
Flags: needinfo?(johu)
Assignee | ||
Comment 34•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8451918 -
Flags: review?(johu)
Assignee | ||
Comment 35•8 years ago
|
||
Master (handleScreenLayoutChange) https://github.com/mozilla-b2g/gaia/commit/b939eb05e10193b8b867de182fc9b510d91866f0
Assignee | ||
Comment 36•8 years ago
|
||
video.js coverage, including showPlayer, hidePlayer, and handleScreenLayoutChange tests: http://video.gaiamobile.org:8080/js/video.js - 300/607 - 49.42 %
Comment 37•8 years ago
|
||
Comment on attachment 8451918 [details] [review] PR for toggleVideoControls, setControlsVisibility unit tests Looks good to me. I found two nits in PR. Please check them before merge.
Attachment #8451918 -
Flags: review?(johu) → review+
Assignee | ||
Comment 38•8 years ago
|
||
toggleVideoControls, setControlsVisibility unit tests: https://github.com/mozilla-b2g/gaia/commit/754630de7f45c537dd37c425e4c9e4ce78f10ca8 video.js coverage: http://video.gaiamobile.org:8080/js/video.js - 309/607 - 50.91 %
Assignee | ||
Comment 39•8 years ago
|
||
Hi John, more video.js unit tests (updateSelection). Thanks.
Attachment #8453266 -
Flags: review?(johu)
Assignee | ||
Comment 40•8 years ago
|
||
Unit test coverage for video app: http://video.gaiamobile.org:8080/js/thumbnail_date_group.js - 52/52 - 100 % http://video.gaiamobile.org:8080/js/thumbnail_item.js - 76/77 - 98.7 % http://video.gaiamobile.org:8080/js/thumbnail_list.js - 71/89 - 79.78 % http://video.gaiamobile.org:8080/js/forward_rewind_controller.js - 56/57 - 98.25 % http://video.gaiamobile.org:8080/shared/js/lazy_loader.js - 6/51 - 11.76 % http://video.gaiamobile.org:8080/js/video_utils.js - 86/87 - 98.85 % http://video.gaiamobile.org:8080/js/video.js - 365/607 - 60.13 % Note: there are two patches pending review for video.js unit tests. The coverage above for video.js includes these two patches.
Assignee | ||
Comment 41•8 years ago
|
||
John, if you don't have time to review, that's ok. I will ask djf.
Attachment #8454639 -
Flags: review?(johu)
Flags: needinfo?(johu)
Comment 42•8 years ago
|
||
Comment on attachment 8453266 [details] [review] updateSelection unit tests: https://github.com/mozilla-b2g/gaia/pull/21552 Looks good to me. Sorry for late reviewing.
Attachment #8453266 -
Flags: review?(johu) → review+
Flags: needinfo?(johu)
Comment 43•8 years ago
|
||
Comment on attachment 8454639 [details] [review] PR for handleSliderTouchStart/Move/End unit tests: https://github.com/mozilla-b2g/gaia/pull/21635 Russ, I would like to see unchained unit tests. I think other parts are ok to me.
Attachment #8454639 -
Flags: review?(johu)
Comment 44•8 years ago
|
||
Comment on attachment 8453266 [details] [review] updateSelection unit tests: https://github.com/mozilla-b2g/gaia/pull/21552 Make this patch obsoleted because it is merged to the attachment 8454639 [details] [review].
Attachment #8453266 -
Attachment is obsolete: true
Assignee | ||
Comment 45•8 years ago
|
||
updateSelection commit: https://github.com/russnicoletti/gaia/commit/6a55f251655786459d1b41c39fd4fcb94bc12556
Assignee | ||
Comment 46•8 years ago
|
||
Thanks for the reviews, John. I've removed the "chaining" of tests and updated the PR.
Assignee | ||
Comment 47•8 years ago
|
||
I also addressed your other comment in the review and landed the updated patch: https://github.com/mozilla-b2g/gaia/commit/898bfe3456b3960bd8d1306650055179d6452d58
Assignee | ||
Comment 48•8 years ago
|
||
While gathering unit test coverage for the video app, I decided I would gather unit test coverage for all gaia apps. Attached is a spreadsheet with the pct of unit test coverage at the app level for all apps and the unit test coverage at the script level (per app).
Assignee | ||
Comment 49•8 years ago
|
||
Attachment #8460998 -
Attachment is obsolete: true
Comment 50•4 years ago
|
||
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•