979636 - Unit tests for video app

Status: RESOLVED WONTFIX

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

Description

[Russ Nicoletti [:russn]]

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

Comment 1

[Russ Nicoletti [:russn]]

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

Comment 2

[Russ Nicoletti [:russn]]

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

Comment 3

[Russ Nicoletti [:russn]]

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

Comment 4

[Russ Nicoletti [:russn]]

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

Comment 5

[Russ Nicoletti [:russn]]

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

Comment 6

[Russ Nicoletti [:russn]]

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

Comment 7

[Russ Nicoletti [:russn]]

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

Comment 8

[Russ Nicoletti [:russn]]

Attachment: Pull request https://github.com/mozilla-b2g/gaia/pull/17142

Hi John,

Can you review my patch for adding some unit tests to video.js? It's a small patch.

Comment 9

[John Hu [:johnhu][: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?

Comment 10

[Russ Nicoletti [:russn]]

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.

Comment 11

[Russ Nicoletti [:russn]]

Hi David,

Can I land these video.js unit tests in 1.4? There is no refactoring associated with these tests.

Comment 12

[Russ Nicoletti [:russn]]

I went ahead and merged this as there was no refactoring to the app code, only unit tests were added.

Comment 13

[Russ Nicoletti [:russn]]

Attachment: Pull request https://github.com/mozilla-b2g/gaia/pull/18546

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.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

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

Comment 15

[Russ Nicoletti [:russn]]

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

Comment 16

[Russ Nicoletti [:russn]]

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

[John Hu [:johnhu][:johu][:醬糊小弟]]

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.

Comment 18

[David Flanagan [:djf]]

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!

Comment 19

[Russ Nicoletti [:russn]]

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

Comment 20

[Russ Nicoletti [:russn]]

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.

Comment 21

[Russ Nicoletti [:russn]]

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

[John Hu [:johnhu][:johu][:醬糊小弟]]

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.

Comment 23

[Russ Nicoletti [:russn]]

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.

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks Russ,

I think this version is cool.

Comment 25

[John Hu [:johnhu][: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.

Comment 26

[Russ Nicoletti [:russn]]

Landed showPlayer tests in master: https://github.com/mozilla-b2g/gaia/commit/101c500903a2477f9de1ea5ce523b9e0be4d45d0

Comment 27

[Russ Nicoletti [:russn]]

Attachment: Pull request: https://github.com/mozilla-b2g/gaia/pull/19338

John, I've created unit tests for showPlayer. Please let me know your comments. Thanks for all your reviews!

Comment 28

[Russ Nicoletti [:russn]]

Correction: in comment #27, 'showPlayer' should be 'hidePlayer'

Comment 29

[John Hu [:johnhu][:johu][:醬糊小弟]]

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.

Comment 30

[Russ Nicoletti [:russn]]

hidePlayer, master: https://github.com/russnicoletti/gaia/commit/480fea09349563fd7a14f687782678b80f6eaa8d

Comment 31

[Russ Nicoletti [:russn]]

Attachment: handleScreenLayoutChange tests PR: https://github.com/mozilla-b2g/gaia/pull/20817

Comment 32

[Russ Nicoletti [:russn]]

John, will you be able to review the handleScreenLayoutChange unit tests? If not, that's ok, I'll ask djf.

Comment 33

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8443750 [details] [review]
handleScreenLayoutChange tests PR: https://github.com/mozilla-b2g/gaia/pull/20817

Thanks. It looks good.

Comment 34

[Russ Nicoletti [:russn]]

Attachment: PR for toggleVideoControls, setControlsVisibility unit tests

Comment 35

[Russ Nicoletti [:russn]]

Master (handleScreenLayoutChange)

https://github.com/mozilla-b2g/gaia/commit/b939eb05e10193b8b867de182fc9b510d91866f0

Comment 36

[Russ Nicoletti [:russn]]

video.js coverage, including showPlayer, hidePlayer, and handleScreenLayoutChange tests:

http://video.gaiamobile.org:8080/js/video.js - 300/607 - 49.42 %

Comment 37

[John Hu [:johnhu][:johu][:醬糊小弟]]

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.

Comment 38

[Russ Nicoletti [:russn]]

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 %

Comment 39

[Russ Nicoletti [:russn]]

Attachment: updateSelection unit tests: https://github.com/mozilla-b2g/gaia/pull/21552

Hi John, more video.js unit tests (updateSelection). Thanks.

Comment 40

[Russ Nicoletti [:russn]]

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.

Comment 41

[Russ Nicoletti [:russn]]

Attachment: PR for handleSliderTouchStart/Move/End unit tests: https://github.com/mozilla-b2g/gaia/pull/21635

John, if you don't have time to review, that's ok. I will ask djf.

Comment 42

[John Hu [:johnhu][: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.

Comment 43

[John Hu [:johnhu][: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.

Comment 44

[John Hu [:johnhu][: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].

Comment 45

[Russ Nicoletti [:russn]]

updateSelection commit:

https://github.com/russnicoletti/gaia/commit/6a55f251655786459d1b41c39fd4fcb94bc12556

Comment 46

[Russ Nicoletti [:russn]]

Thanks for the reviews, John. I've removed the "chaining" of tests and updated the PR.

Comment 47

[Russ Nicoletti [:russn]]

I also addressed your other comment in the review and landed the updated patch:

https://github.com/mozilla-b2g/gaia/commit/898bfe3456b3960bd8d1306650055179d6452d58

Comment 48

[Russ Nicoletti [:russn]]

Attachment: Unit test coverage for gaia apps

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

Comment 49

[Russ Nicoletti [:russn]]

Attachment: Unit Test Coverage for Gaia Apps

Comment 50

[BMO Automation]

Firefox OS is not being worked on