|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
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
Created attachment 8732800 [details] MozReview Request: Bug 1258003 - Raise ValueError if video_urls empty; r?maja_zf 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)
(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!
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!
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+
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.