Closed Bug 1254732 Opened 4 years ago Closed 4 years ago

External Media Tests needs comprehensive API documentation.

Categories

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

defect
Not set

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: nobody → spolk
Summary: docstrings need to be revamped for external_media_tests → External Media Tests need comprehensive API documentation.
Summary: External Media Tests need comprehensive API documentation. → External Media Tests needs comprehensive API documentation.
Attachment #8731839 - Flags: feedback?(dburns)
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:`
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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40867/diff/1-2/
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)
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)
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

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+
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)
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+
https://hg.mozilla.org/mozilla-central/rev/0a35f6c4d11f
Status: NEW → RESOLVED
Closed: 4 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.