Closed Bug 979636 Opened 7 years ago Closed 3 years ago

Unit tests for video app

Categories

(Firefox OS Graveyard :: Gaia::Video, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
1.4 S3 (14mar)

People

(Reporter: rnicoletti, Assigned: rnicoletti)

References

Details

Attachments

(7 files, 2 obsolete files)

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
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
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
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
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
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
Status: NEW → ASSIGNED
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
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
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 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+
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.
Hi David,

Can I land these video.js unit tests in 1.4? There is no refactoring associated with these tests.
Flags: needinfo?(dflanagan)
I went ahead and merged this as there was no refactoring to the app code, only unit tests were added.
Flags: needinfo?(dflanagan)
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 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-
(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)
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.
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 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)
(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)
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.
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.
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)
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)
Thanks Russ,

I think this version is cool.
Flags: needinfo?(johu)
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+
John, I've created unit tests for showPlayer. Please let me know your comments. Thanks for all your reviews!
Attachment #8424226 - Flags: review?(johu)
Correction: in comment #27, 'showPlayer' should be 'hidePlayer'
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+
Depends on: 1013464
John, will you be able to review the handleScreenLayoutChange unit tests? If not, that's ok, I'll ask djf.
Flags: needinfo?(johu)
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)
Attachment #8451918 - Flags: review?(johu)
video.js coverage, including showPlayer, hidePlayer, and handleScreenLayoutChange tests:

http://video.gaiamobile.org:8080/js/video.js - 300/607 - 49.42 %
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+
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 %
Hi John, more video.js unit tests (updateSelection). Thanks.
Attachment #8453266 - Flags: review?(johu)
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.
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 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 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 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
Thanks for the reviews, John. I've removed the "chaining" of tests and updated the PR.
I also addressed your other comment in the review and landed the updated patch:

https://github.com/mozilla-b2g/gaia/commit/898bfe3456b3960bd8d1306650055179d6452d58
Attached file Unit test coverage for gaia apps (obsolete) —
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).
Attachment #8460998 - Attachment is obsolete: true
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.