Closed Bug 1258003 Opened 8 years ago Closed 8 years ago

Raise ValueError when empty video manifest is provided

Categories

(Testing Graveyard :: external-media-tests, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: impossibus, Assigned: vakila, Mentored)

Details

(Whiteboard: [good-first-bug][lang=py])

Attachments

(1 file)

The video_urls attribute in MediaTestCase should be an empty list by default, but its default value is instead set to be a boolean:
https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/external_media_harness/testcase.py#21

You can run the affected test-suite locally with |./mach external-media-tests|
(In reply to Maja Frydrychowicz (:maja_zf) from comment #0)
> The video_urls attribute in MediaTestCase should be an empty list by
> default

Even better, let's raise a ValueError in the argument parser if the list of video urls is empty! https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/external_media_harness/runtests.py#34

(Leave the line is testcase.py as is; it prevents us from iterating tests on empty lists. However, that should really be reported as a busted test, not a failed test (hence raising a ValueError in the parser.)
I'm interested in working on this. Just want to check I've understood correctly: the only change needed is in runtests.py, to throw a ValueError if args.video_urls is empty? i.e. the type error in testcase.py is no longer actually the issue?
(In reply to Anjana Vakil (:vakila) from comment #2)
> I'm interested in working on this. Just want to check I've understood
> correctly: the only change needed is in runtests.py, to throw a ValueError
> if args.video_urls is empty? i.e. the type error in testcase.py is no longer
> actually the issue?

Correct. Raising a ValueError in runtests.py will be reported as a problem with the harness (or how it was called), which is preferable to addressing the issue in the testcase and reporting it as a test failure. We also don't want our test cases to be iterating on empty lists, so a fall-back value of False for self.video_urls is actually a good thing.

Bugzilla tip: if your comment is a question, check the "Need more information from ___" box at the bottom of the bug form.
Assignee: nobody → anjanavakil
Summary: Fix type error in definition of self.video_urls → Raise ValueError when empty video maniest is provided
(In reply to Maja Frydrychowicz (:maja_zf) from comment #3)

Thanks for confirming. I've made the simple change to the code, and hopefully submitted it for review using hg/MozReview correctly - just checking the "needs more information" box here in case I haven't done it right and you don't get notified for the review. Looking forward to your feedback!
Flags: needinfo?(mjzffr)
Summary: Raise ValueError when empty video maniest is provided → Raise ValueError when empty video manifest is provided
Yep, the review request was sent correctly so I got a "r?" notification. Thanks!
Flags: needinfo?(mjzffr)
Comment on attachment 8732800 [details]
MozReview Request: Bug 1258003 - Raise ValueError if video_urls empty; r?maja_zf

https://reviewboard.mozilla.org/r/41375/#review37903

Thanks, Anjana!
Attachment #8732800 - Flags: review?(mjzffr) → review+
https://hg.mozilla.org/mozilla-central/rev/3c4c38101215
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.