Closed Bug 564720 Opened 10 years ago Closed 10 years ago

Refactor media tests


(Core :: Audio/Video, defect)

Not set





(Reporter: cpearce, Assigned: cpearce)




(1 file, 4 obsolete files)

There's a lot of duplication in the media tests; we repeat many of the same tests for Wav as we do for Ogg. We should refactor the media tests so that they're backend independent.

First observations: it looks like everything we test in test_ended1 and test_wav_ended1 are already tested in the backend independent test_playback. We could easily add the stuff in test_*ended2 to test_play_twice as well.
We can probably fold test_duration1.html into test_playback as well.
Assignee: nobody → chris
Everything in test_[wav_]timeupdate1 is tested in test_playback, so we can just delete that test.

We can check the metadata in the timeupdate handler in test_playback in order to test everything tested in test_[wav_]timeupdate2.

test_timeupdate3 can be implemented as a seek test.
(In reply to comment #2)
> test_timeupdate3 can be implemented as a seek test.

But this has been disabled due to flakiness on WinNT, so I'll not refactor this immediately, I'll enable it and run it through the replay debugger to see if it still fails; it may not fail in the new decoder).
test_bug468190 can be folded into test_onloadedmetadata.
Depends on: 568402
test_progressN can all be folded into a single backend independent test.
Attached patch Patch v1 (obsolete) — Splinter Review
Make tests backend independent where possible. Moved, changed, merged and renamed tests where appropriate.

I didn't make the Ogg X-Content-Duration tests backend independent, as that doesn't really make sense. There's still one WAV test, which tests various misspellings of <source> which I didn't change too.
Attachment #447856 - Flags: review?(roc)
This is really, really nice. Just a few minor comments:

We can make test_bug463162.xhtml backend independent by using <video> elements, and setting 'src' on the source elements to the first file from gSmallTests that we support.

gProgressTests should have a "bogus.duh" file to make sure tests that use it handle unsupported types correctly.

In test_error_on_404.html (and maybe other tests), if we don't manage to play any of the types, the test will never finish because loadError will never be called. Probably worth adding
  if (videos.length == 0) {
    todo(false, "No types supported");
  } else {

test_play_events.html does the right thing, but if videos.length is 0 you should probably do a todo(false) since I think mochitests complain if there are no tests run. Same in test_replay_metadata.html.
test_video_to_canvas.html too.
Attached patch Patch v2 (obsolete) — Splinter Review
Patch v2. Changes from v1:
* Resurrected test_bug476973, it was sitting in the test directory not being tested. I converted it into a test_seek case.
* Added guards before calls to SimpleTest.waitForExplicitFinish() where appropriate so that we won't timeout when there's no media backends, as per review comments.
* Address other review comments.
Attachment #447856 - Attachment is obsolete: true
Attachment #447918 - Flags: review?(roc)
Attachment #447856 - Flags: review?(roc)
Changes between patch v1 and patch v2.
Comment on attachment 447918 [details] [diff] [review]
Patch v2

Just move videoDocumenTitle/audioDocumentTitle to the platform-independent section of the makefile and comment that they need to be made platform independent before they get fixed and reenabled.
Attachment #447918 - Flags: review?(roc) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Patch v2 with comments in as per review comment.
Attachment #447918 - Attachment is obsolete: true
Attachment #447919 - Attachment is obsolete: true
Attachment #447925 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
Rebased on trunk, rather than on top of the webm patches.
Attachment #447925 - Attachment is obsolete: true
Attachment #447930 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]

I'll wait for the tree to go green before closing bug...
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
This seems to have stuck. There were a few new randoms, but nothing major. Some are on Windows, so I can use replay-debugging to catch them, probably next week.
Hey, this patch apparently made the content/media mochitest load all their media files from file:// URIs as opposed to http. What was the reason for that? Afaics, content tests should not care about file:// vs http:// except perhaps for same-origin issues.
No, this only moved some existing code that made a small subset of tests use file:// --- tests that need to explicitly check access to file:// URIs.
On Android, mochitests can't currently access file:// URIs at all. In bug 759221 I'm trying to turn on media mochitest on android, and to that effect, I have a patch here that lets them use http:// instead on Android. Does that sound like the right approach or should I rather completely disable the affected tests? Patch is Attachment 629920 [details] [diff] .
(In reply to Benoit Jacob [:bjacob] from comment #18)
> On Android, mochitests can't currently access file:// URIs at all. 

Is this a feature or a bug? i.e. it seems that mochitests being unable to access file:// URIs is the problem here. Can you fix that instead? Or are we explicitly disallowing file:// URIs on Android?

The tests loading file:// URIs are testing the handling of file:// URIs, so if you start returning non file:// URIs you're breaking the tests assumptions/test conditions. Better of disabling these tests than running the test under invalid assumptions.
Right. If it's going to be a lot of work to give Android mochitests access to file://, and we don't want to prioritize that work now, then just disable those particular tests.
You need to log in before you can comment on or make changes to this bug.