Bug 1122079 - Test the detection and skip of mp4 videos. #28413
Conversation
|
|
Hubert Figuière (hfiguiere)
| { | ||
| type: 'music', | ||
| filePath: 'test_media/Movies/gizmo2.mp4' | ||
| } |
There was a problem hiding this comment.
With this selection of files, the test would succeed even if the app didn't support any .mp4 formats. Please include a .mp4 audio file and verify that it does get listed. I think you can just copy the existing .m4a file and rename it to .mp4.
Even better would be if you could find a test .mp4 audio file that was not from apple, so it had "ftypmp42" just like the movie file does. That would test even deeper in the metadata parsing code. But I'm not even sure that such things exist.
There was a problem hiding this comment.
In general, this is a hard test to do exhaustively because mp4 is such a generic container and there are so many codecs that get included within it. Ideally, we'd have a comprehensive library of mp4 audio and video samples and could write a test to verify that the app reads all of the audio files and none of the video files. If you know of any such mp4 sample library, feel free to import a copy into test-data/mp4/ or something and expand this test. If not, then just make sure that the test is written to be extensible so that we can easily add other .mp4 files to it in the future.
Speaking of a test-data/mp4/ directory. Consider creating one, and copying the gizmo file and test-data/*.m4a file into it so that there is one central location for the test media used here.
There was a problem hiding this comment.
test-data is in general the various test files. It is not specific to any test.
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
5dce0f9
to
ddf4a15
Compare
|
|
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
ddf4a15
to
69e47ea
Compare
|
|
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
69e47ea
to
10b8725
Compare
|
|
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
10b8725
to
50c6fac
Compare
|
|
|
|
||
| music.launch(); | ||
| music.waitForFirstTile(); | ||
| music.waitFinishedScanning(); |
There was a problem hiding this comment.
I'm not convinced by the implementation of waitFinishedScanning(). It will return if the musicdb is not scanning. So if scanning has not started yet, it will also return immediately. Ideally, music.launch() would inject an event handler that listens for the scanend event, and that event handler would set some kind of _initialScanComplete flag on the musicdb. Then, waitFinishedScanning() can wait for that flag to be set.
I'd even be okay adding this line to shared/js/mediadb.js in the endscan() method at 1696:
media.initialScanComplete = true;
There was a problem hiding this comment.
If you are OK with doing this in mediadb.js then I will. It seems to be the simple and reliable solution to this. Will update the PR.
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
50c6fac
to
f87eb9f
Compare
|
Updated PR now. |
|
|
||
| music.waitForSongsInList(function(songs) { | ||
| return songs.length >= 2; | ||
| }); |
There was a problem hiding this comment.
I don't like this because you're waiting for there two be two songs, then testing that there are only two songs. If a third song is added to the list later, then the test could pass when it should have failed.
Given the music app's current architecture, I don't see an easy way to know when the songs view is fully populated. Maybe you could modify switchToSongsView() so that it patches App.showCorrectOverlay() and use that to detect when the songs view is full loaded.
If you don't do that, I'd like you to at least add a comment here about the issue and wait for an extra second to make sure that no more songs are added.
Actually, here's a better way: when list_view.js enumerates the db, it sets this.handle to the return value of the enumerate() call. this.handle is an object with a state property. If you wait for this.handle.state === 'complete' then you'll know that the enumeration is complete.
There was a problem hiding this comment.
will update PR with that.
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
f87eb9f
to
e791264
Compare
hfiguiere
force-pushed
the
bug1122079-test-video-detection
branch
from
a031a32
to
d027717
Compare
…t-video-detection to mozilla-b2g:master
No description provided.