Closed
Bug 1254732
Opened 8 years ago
Closed 8 years ago
External Media Tests needs comprehensive API documentation.
Categories
(Testing Graveyard :: external-media-tests, defect)
Testing Graveyard
external-media-tests
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: sydpolk, Assigned: sydpolk)
Details
Attachments
(1 file)
Per :AutomatedTester, the docstrings currently in the project don't give enough information about how to use what is there to write new tests.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → spolk
Summary: docstrings need to be revamped for external_media_tests → External Media Tests need comprehensive API documentation.
Assignee | ||
Updated•8 years ago
|
Summary: External Media Tests need comprehensive API documentation. → External Media Tests needs comprehensive API documentation.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40867/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40867/
Attachment #8731839 -
Flags: review?(mjzffr)
Attachment #8731839 -
Flags: review?(bvandyk)
Assignee | ||
Updated•8 years ago
|
Attachment #8731839 -
Flags: feedback?(dburns)
Comment 2•8 years ago
|
||
https://reviewboard.mozilla.org/r/40867/#review37479 ::: dom/media/test/external/external_media_harness/testcase.py:20 (Diff revision 1) > VideoException, VideoPuppeteer as VP) > > > class MediaTestCase(FirefoxTestCase): > > + """ Necessary methods for MSE playback Start the words on a new line so it is ``` """ foobar """ ``` ::: dom/media/test/external/external_media_harness/testcase.py:31 (Diff revision 1) > self.video_urls = kwargs.pop('video_urls', False) > FirefoxTestCase.__init__(self, *args, **kwargs) > > def save_screenshot(self): > + """ > + Make a screenshot of the window that is currently playing the video element. Do we need the white space below? or should we add whitespace above? ::: dom/media/test/external/external_media_harness/testcase.py:100 (Diff revision 1) > """ > raise SkipTest(reason) > > > class NetworkBandwidthTestCase(MediaTestCase): > + """ Test MSE playback under different network conditions. Text on a new line ::: dom/media/test/external/external_media_harness/testcase.py:102 (Diff revision 1) > > > class NetworkBandwidthTestCase(MediaTestCase): > + """ Test MSE playback under different network conditions. > + > + Test case for testing playback while network bandwidth is limited. Uses browsermobproxy (https://bmp.lightbody.net/). Please see https://developer.mozilla.org/en-US/docs/Mozilla/QA/external-media-tests for more information on setting up browsermob_proxy. Add line breaks so it is easier to read. Mozreview is wrapping it but no everyone's editor will wrap ::: dom/media/test/external/external_media_harness/testcase.py:145 (Diff revision 1) > - This test both starting videos and performing partial playback at one > + This tests both starting videos and performing partial playback at one > minute each, and is the test that should be run frequently in automation. > """ > > def test_playback_starts(self): > + """ Test to make sure that playback of the video element starts for each video. text on a new line ::: dom/media/test/external/external_media_harness/testcase.py:165 (Diff revision 1) > level='WARNING') > except TimeoutException as e: > raise self.failureException(e) > > def test_video_playback_partial(self): > - """ First 60 seconds of video play well. """ > + """ Test to make sure that playback of 60 seconds works for each video. Text on a new line ::: dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py:264 (Diff revision 1) > def playback_started(video): > + """ > + Determine if video has started > + > + :param video: The video that we are interested in. > + :return: True if is playing; False otherwise I would an empty line between `:param:` and `:return:` ::: dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py:280 (Diff revision 1) > - # we are essentially done. If this happens to be last time we are polled > - # before the video ends, we won't get another chance. > + If we are near the end and there is still a video element, then > + we are essentially done. If this happens to be last time we are polled > + before the video ends, we won't get another chance. > + > + :param video: The video that we are interested in. > + :return: True if we are close enough to the end of playback; False otherwise. I would an empty line between `:param:` and `:return:` ::: dom/media/test/external/external_media_tests/media_utils/youtube_puppeteer.py:119 (Diff revision 1) > YouTube #movie_player element. > + > :param script: script to be executed. > - `arguments[0]` in script refers to video element, `arguments[1]` refers > - to #movie_player element (YouTube). > :return: value returned by script I would an empty line between `:param:` and `:return:`
Assignee | ||
Updated•8 years ago
|
Attachment #8731839 -
Flags: feedback?(dburns)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40867/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8731839 -
Flags: feedback?(dburns)
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf https://reviewboard.mozilla.org/r/40867/#review37797 ::: dom/media/test/external/docs/conf.py:21 (Diff revision 2) > +import os > + > +# If extensions (or modules to document with autodoc) are in another directory, > +# add these directories to sys.path here. If the directory is relative to the > +# documentation root, use os.path.abspath to make it absolute, like shown here. > +#sys.path.insert(0, os.path.abspath('.')) Do we need to setup any paths here? When generating docs locally I'm seeing unsatisfied imports. Is there another way I should be getting these resolved?
Attachment #8731839 -
Flags: review?(bvandyk)
Attachment #8731839 -
Flags: review?(mjzffr) → review+
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf https://reviewboard.mozilla.org/r/40867/#review37807 r+wc - I left several suggestions. I did not look at the documentation generation; will leave that to :SingingTree. ::: dom/media/test/external/external_media_harness/testcase.py:60 (Diff revision 2) > if debug_lines: > self.marionette.log('\n'.join(debug_lines)) > > def run_playback(self, video): > + """ > + Play the video all of the way through. Raises if the video stalls for too long. I don't think this always plays a video all the way through; it just plays it until the "expected_duration", which may be set to before the end of the video. ::: dom/media/test/external/external_media_harness/testcase.py:62 (Diff revision 2) > > def run_playback(self, video): > + """ > + Play the video all of the way through. Raises if the video stalls for too long. > + > + :param video: The video we want played. It would be helpful to specify type information here and elsewhere. The "video" is expected to be an instance of VideoPuppeteer, right? ::: dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py:156 (Diff revision 2) > return self.execute_video_script( > 'return arguments[0].wrappedJSObject.currentTime;') or 0 > > @property > def remaining_time(self): > - # Note that self.current_time could temporarily refer to a > + """ Please don't remove this comment about spliced-in ads. ::: dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py:164 (Diff revision 2) > return self.expected_duration - self.current_time > > @property > def video_src(self): > + """ > + :return: The src DOM of the video element. the 'src attribute'? ::: dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py:263 (Diff revision 2) > > def playback_started(video): > + """ > + Determine if video has started > + > + :param video: The video that we are interested in. Here's another example of where to provide info about expected data type. (VideoPuppeteer) ::: dom/media/test/external/external_media_tests/media_utils/youtube_puppeteer.py:18 (Diff revision 2) > > > class YouTubePuppeteer(VideoPuppeteer): > """ > Wrapper around a YouTube #movie_player element > Suggestion: add a note here that YouTube JS API is useful for site-specific features such interacting with ads, accessing YouTube'a debug data. ::: dom/media/test/external/external_media_tests/utils.py:47 (Diff revision 2) > > def save_memory_report(marionette): > """ > Saves memory report (like about:memory) to current working directory. > + > + :param marionette: marionette runner to use for executing. 'marionette runner' should be 'Marionette instance'. ('marionette runner' would be confused with something like BaseMarionetteTestRunner)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40867/diff/2-3/
Attachment #8731839 -
Flags: feedback?(dburns) → review?(bvandyk)
Assignee | ||
Updated•8 years ago
|
Attachment #8731839 -
Flags: feedback?(dburns)
Comment 7•8 years ago
|
||
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf Offered initial feedback but as I am not a SME in this area will leave it to others to finish off review.
Attachment #8731839 -
Flags: feedback?(dburns) → feedback+
Assignee | ||
Comment 8•8 years ago
|
||
Bryce, could you take a look at this today? Thanks.
Flags: needinfo?(bvandyk)
https://reviewboard.mozilla.org/r/40867/#review37797 > Do we need to setup any paths here? When generating docs locally I'm seeing unsatisfied imports. Is there another way I should be getting these resolved? For reference follwing the docs here remedy this issue: https://developer.mozilla.org/en-US/docs/Mozilla/QA/external-media-tests
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf https://reviewboard.mozilla.org/r/40867/#review38263
Attachment #8731839 -
Flags: review?(bvandyk) → review+
Flags: needinfo?(bvandyk)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8731839 [details] MozReview Request: Bug 1254732 - External Media Tests needs comprehensive API documentation. r?SingingTree, r?maja_zf https://reviewboard.mozilla.org/r/40867/#review38267
Attachment #8731839 -
Flags: review+
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a35f6c4d11f
Status: NEW → RESOLVED
Closed: 8 years ago
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
•