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)
Testing Graveyard
external-media-tests
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|
Reporter | ||
Comment 1•8 years ago
|
||
(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.)
Assignee | ||
Comment 2•8 years ago
|
||
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?
Reporter | ||
Comment 3•8 years ago
|
||
(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
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41375/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41375/
Attachment #8732800 -
Flags: review?(mjzffr)
Assignee | ||
Comment 5•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Summary: Raise ValueError when empty video maniest is provided → Raise ValueError when empty video manifest is provided
Reporter | ||
Comment 6•8 years ago
|
||
Yep, the review request was sent correctly so I got a "r?" notification. Thanks!
Flags: needinfo?(mjzffr)
Reporter | ||
Comment 7•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c4c38101215
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•