851502 - Create an Endurance test to measure html5 video memory use (re: bug 834667)

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[u279076]

In bug 834667 we regressed memory use while playing HTML5 video resulting in a rather significant memory leak. As per bug 834667 comment 21 we want to create an Endurance test to cover this regression in the future.

This bug will need to be tackled in two phases:
1) Reducing the testcase to use a local html5 video (not YouTube)
2) Developing a Mozmill test which measures memory use when loading the video and skipping ahead to ~30s

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Regression in Firefox. We should give it a higher priority.

Comment 2

[mario garbi]

On mozqa we only have small videos at the moment (10 seconds). For this test we would need an clip at lest 35-40 sec long if we are to skip to 0:30 and play for a short while. Should we create an HTML5 video to be uploaded on mozqa or is there another approach we would want to take?

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Anthony: Is the length of the video important for this test case? If so, could we create a longer video for testcase data? Perhaps the existing video looped several times.

Comment 4

[u279076]

I tested with the sample videos on mozqa.com using Firefox 17.0.2esr and the bug does not reproduce. I'll need to find a source video which reproduces this. I'm not sure the 30s called out in the bug really matters. The source video in that bug is 51 minutes long and I can seek to any arbitrary position to reproduce this bug. I think we'll need to play around with different video lengths until we find the shortest length which still reproduces the bug.

Comment 5

[Andreea Matei [:AndreeaMatei]]

As discussed in Ask an Expert, we still want this test, I recall it was a discussion about the size of this video and how much we can decrease it.

Comment 6

[mario garbi]

I can manually reproduce this with 17.0.2esr but only have the huge increase while using html5 videos on youtube or other remote sources like http://sublimevideo.net/framework that have both video and audio channels. This is covered in Bug 834667 comment 12.
For example using the video at http://sublimevideo.net/framework we get a crash right after skipping ahead after 1 minute mark and the terminal log shows an 'out of memory' output. 

In order to not depend on a remote source we would need to add a HTML5 audio/video file to mozqa.com which we do not have at the moment (we only have short 10 sec nosound files).

I have the test structure build already so as soon as we get a file up on mozqa I will come with a patch.

Comment 7

[mario garbi]

Attachment: HTML5 Video Test v1

Created a new test as requested and had to make a new html file to handle the html5 video player the way we need for this test (skip ahead 5 seconds a few times to see if the regression happens again).

I can reproduce the failure with a Firefox 17.0.2esr and we get a Disconnect Error with "out of memory" in console:
http://mozmill-crowd.blargon7.com/#/endurance/report/bb6e314abd8f1e30e8af61ecd4452fb2

Mac:
http://mozmill-crowd.blargon7.com/#/endurance/report/bb6e314abd8f1e30e8af61ecd4aeab1c
Linux:
http://mozmill-crowd.blargon7.com/#/endurance/report/bb6e314abd8f1e30e8af61ecd4b1500b
Windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/bb6e314abd8f1e30e8af61ecd4b067ad

Comment 8

[mario garbi]

Comment on attachment 820884 [details] [diff] [review]
HTML5 Video Test v1

The patch will be updated after we land the testcase-data file so we can refer to mozqa.com video folder directly without having another file in our local repository. The patch as it is here can be run as it adds the html5 file to the Data folder and is useful for seeing how the test is structured.

I will also fix the extra { assert } that I've missed initially.

Comment 9

[Andrei Eftimie]

Comment on attachment 820884 [details] [diff] [review]
HTML5 Video Test v1

Review of attachment 820884 [details] [diff] [review]:
-----------------------------------------------------------------

Question: would it be better for the testfile HTML5.html to live in the same repository as the actual video file?
I'm thinking either both locally in mozmill-tests or both remote in testcase-data.
Having both parts of the same test in 2 separate repositories might lead to cumbersome updates in the future.

However I wouldn't want a large file in mozmill-tests, if the video file will be small enough it might be doable.
Please see how much you can trim the video you use here and still reproduce the problem.
We might be able to seek() in 2 second increments with a 1 second pause to give it room to buffer, and a 10s clip might be enough.
Also test if we can deliver the video locally as well.

::: data/layout/HTML5.html
@@ +22,5 @@
> +      function skip(value) {
> +        var video = document.getElementById("videoPlayer");
> +        video.currentTime += value;
> +      }
> +    </script>

You can simplify this by moving the script tag at the bottom of the file (right before you close the body tag) and 
save a reference to `videoPlayer` and the `play` once.

**This could be further improved, but I think this suffices for this test.

@@ +30,5 @@
> +  </head>
> +
> +  <body>
> +    <div name="container" style="text-align:center; margin:0 auto 0;
> +              height:100%; width:100%; padding-top:10">

Please move CSS declarations into a <style> tag in <head>

@@ +32,5 @@
> +  <body>
> +    <div name="container" style="text-align:center; margin:0 auto 0;
> +              height:100%; width:100%; padding-top:10">
> +      <div style="color:#06a2c3">
> +        <h1> Desktop Automation - HTML5 Video File </h1>

nit: you can trim the whitespace at the beginning and end of the inside of the tag

@@ +35,5 @@
> +      <div style="color:#06a2c3">
> +        <h1> Desktop Automation - HTML5 Video File </h1>
> +      </div>
> +
> +      <video name="media" id="videoPlayer">

You can remove the name, I don't see it used.

::: tests/endurance/testHTML5VideoMemory/test1.js
@@ +30,5 @@
> +function testHTML5Video() {
> +  enduranceManager.run(function () {
> +    enduranceManager.addCheckpoint("Load a web page with HTML5 Video");
> +    controller.open(TEST_DATA);
> +    controller.waitForPageLoad(TIMEOUT_PAGE);

Do we need to use a custom timeout here?

@@ +39,5 @@
> +    controller.click(playButton);
> +    controller.sleep(1000);
> +
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip ahead 10 second using the Forward button");

This is skipping 5 seconds for each iteration right now.

@@ +40,5 @@
> +    controller.sleep(1000);
> +
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip ahead 10 second using the Forward button");
> +      var fwdButton = new elementslib.ID(controller.window.document, "fastFwd");

Move this declaration out of the loop

Comment 10

[mario garbi]

Attachment: newHTML5VideoTest_v2.patch

Updated the test to use the file on mozqa.com, on Firefox 17.02esr we get a disconnect error that represents the fact that the test is working and it reproduces the failure.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 831567 [details] [diff] [review]
newHTML5VideoTest_v2.patch

Review of attachment 831567 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/endurance/testHTML5_VideoMemory/test1.js
@@ +14,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +
> +  aModule.enduranceManager = new endurance.EnduranceManager(aModule.controller);

No need for that blank line. We use those to separate declarations from calling methods.

@@ +32,5 @@
> +    enduranceManager.addCheckpoint("Starting the video using the play button");
> +    var playButton = new elementslib.ID(controller.window.document, "play");
> +    var skipField = new elementslib.ID(controller.window.document, "skipField");
> +    var skipButton = new elementslib.ID(controller.window.document, "skip");
> +    controller.keypress(skipField, "VK_DELETE", {});

I don't think this works. I got 50 in the skip field. Due to that, the video played since 50 and in the loop, if you have 10 entities as we use, you never get to skip about 7 of them.
You can use dumps in your testing to see what are the outputs of the fields and if you get to skip 5 in each entity.

Comment 12

[mario garbi]

Attachment: newHTML5VideoTest_NIGHTLY.patch

Made a few changes to the patch and this version is for Nightly branch:

Linux:
http://mozmill-crowd.blargon7.com/#/endurance/report/d7f12eb72275b30155ba68b8bcbd53d5

Windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/d7f12eb72275b30155ba68b8bcbd7a06

Mac:
http://mozmill-crowd.blargon7.com/#/endurance/report/d7f12eb72275b30155ba68b8bcbdb63f

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 832934 [details] [diff] [review]
newHTML5VideoTest_NIGHTLY.patch

Review of attachment 832934 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/endurance/testHTML5_VideoMemory/test1.js
@@ +38,5 @@
> +      return skipField.getNode().value === "5";
> +    }, "Value set correctly to 5 seconds");
> +
> +    controller.click(playButton);
> +    controller.sleep(1000);

Is it something specific in the steps to reproduce this 1 second "wait" for the video to play? Or it just has to play for no matter how long?

I don't like using sleep calls, we should use readyState.

@@ +44,5 @@
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip ahead 5 second using the Forward button");
> +      controller.click(skipButton);
> +      // We wait for the video to play for 1 second
> +      controller.sleep(1000);

Same here.

Comment 14

[mario garbi]

Attachment: newHTML5VideoTest_v4.patch

Changed the controller.sleep() into waitFor the readyState of the video. When we reproduce the issue (reproducible with ESR17.02 only) we get a timeOut since the entire browser gets sluggish as memory is being filled and the failures happens at around 200Mb (double from anything else in the testrun).

Nightly testrun:
http://mozmill-crowd.blargon7.com/#/endurance/report/456bebe92845279408c15c03e80db323

ESR17.02 (failing):
http://mozmill-crowd.blargon7.com/#/endurance/report/456bebe92845279408c15c03e8115543

I have changed the assert.waitFor message in the meanwhile so that we get a clear message of why it has failed.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8336049 [details] [diff] [review]
newHTML5VideoTest_v4.patch

Review of attachment 8336049 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/endurance/testHTML5_VideoMemory/test1.js
@@ +45,5 @@
> +      enduranceManager.addCheckpoint("Skip ahead 5 second using the Forward button");
> +      controller.click(skipButton);
> +      // We wait for the video to load before skipping again
> +      assert.waitFor( function () {
> +        return movie.getNode().readyState == 2;

Why are we waiting here only for current data and not for the playing state (4)? 
readyState is 4 after we play, outside the loop and also in the loop before skipping. And since the video is playing when skipping, is automatically playing after each skip as well.

I'm thinking about the reproducibility in manual steps, how a user would experience it.

Comment 16

[mario garbi]

Attachment: newHTML5VideoTest_v5.patch

I have updated the patch as suggested, now checking for readyState == 4 and also added a new logic to rewind the video to 00:01 time if we get to 00:59 in our tests. This is necessary to allow us to use as many iterations as we like, ensuring that the video won't stop once it reaches it's max time.

Comment 17

[mario garbi]

Linux:
http://mozmill-crowd.blargon7.com/#/endurance/report/f8efdd634ceea4902aaa0c29570d1de6

Windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/f8efdd634ceea4902aaa0c2957141d25

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8341040 [details] [diff] [review]
newHTML5VideoTest_v5.patch

Review of attachment 8341040 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/endurance/manifest.ini
@@ +7,5 @@
>  [include:testFlash_SWFVideoEmbed/manifest.ini]
>  [include:testFlash_SWFVideoIframe/manifest.ini]
>  [include:testFlash_SWFVideoObject/manifest.ini]
>  [include:testFlash_SWFVideoURL/manifest.ini]
> +[include:testHTML5_VideoMemory/manifest.ini]

This is not a suitable name for the test file. 'HTML5' is just when the video tag was introduced, and 'Memory' is redundant given that this is an endurance test. Please come up with something more suitable. If you are struggling, please ask for suggestions.

::: firefox/tests/endurance/testHTML5_VideoMemory/test1.js
@@ +21,5 @@
> +function teardownModule(aModule) {
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function testHTML5Video() {

As with the file name, please come up with a more suitable method name.

@@ +27,5 @@
> +    enduranceManager.addCheckpoint("Load a web page with HTML5 Video");
> +    controller.open(TEST_DATA);
> +    controller.waitForPageLoad();
> +
> +    enduranceManager.addCheckpoint("Starting the video using the play button");

This checkpoint does not match what the next action is...

@@ +35,5 @@
> +    var seekField = new elementslib.ID(controller.window.document, "seekField");
> +    var seekButton = new elementslib.ID(controller.window.document, "seek");
> +    var skipField = new elementslib.ID(controller.window.document, "skipField");
> +    var skipButton = new elementslib.ID(controller.window.document, "skip");
> +    controller.doubleClick(skipField);

Why are we double clicking the skip field?

@@ +36,5 @@
> +    var seekButton = new elementslib.ID(controller.window.document, "seek");
> +    var skipField = new elementslib.ID(controller.window.document, "skipField");
> +    var skipButton = new elementslib.ID(controller.window.document, "skip");
> +    controller.doubleClick(skipField);
> +    controller.type(skipField, "5");

I would use a constant for the skip value.

@@ +37,5 @@
> +    var skipField = new elementslib.ID(controller.window.document, "skipField");
> +    var skipButton = new elementslib.ID(controller.window.document, "skip");
> +    controller.doubleClick(skipField);
> +    controller.type(skipField, "5");
> +    assert.waitFor( function () {

Why is this assert/waitFor necessary?

@@ +44,5 @@
> +
> +    controller.click(playButton);
> +
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip ahead 5 second using the Forward button");

Again, this checkpoint could immediately precede a different action to the one it describes. Also, there is no 'Forward' button.

@@ +45,5 @@
> +    controller.click(playButton);
> +
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip ahead 5 second using the Forward button");
> +      if (currentTime.getNode().value >= 59) {

We should define a constant for the video length and use that.

@@ +52,5 @@
> +      }
> +      else {
> +        controller.click(skipButton);
> +      }
> +      // We wait for the video to load before skipping again

The video is already loaded, aren't we waiting for the video to start playing again?

Comment 19

[mario garbi]

Attachment: newHTML5VideoTest_v2.patch

Made the changes requested. We double click the field to select all the text inside it, a single click would simply select the field. This way we replace the text inside with our value (5) instead of adding a char to the string (5+0).

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to mario garbi from comment #19)
> Created attachment 8350055 [details] [diff] [review]
> newHTML5VideoTest_v2.patch
> 
> Made the changes requested. We double click the field to select all the text
> inside it, a single click would simply select the field. This way we replace
> the text inside with our value (5) instead of adding a char to the string
> (5+0).

Why not just directly set the value of the textbox?

Comment 21

[mario garbi]

Attachment: newHTML5VideoTest_v3.patch

I initially wanted to use the UI elements more, that is why I used the input field instead of simply setting the value. I made the change in this patch so that we set it directly instead of using the UI.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8350633 [details] [diff] [review]
newHTML5VideoTest_v3.patch

I'm not going to be available to review this for a while, so I suggest leaving this to Andrei and Andreea now until they're happy with it. Then please request review from myself and Henrik.

Comment 23

[Andrei Eftimie]

Comment on attachment 8350633 [details] [diff] [review]
newHTML5VideoTest_v3.patch

Review of attachment 8350633 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the inline comments this looks fine to me.

::: firefox/tests/endurance/manifest.ini
@@ +16,5 @@
>  [include:testTabbedBrowsing_OpenNewWindow/manifest.ini]
>  disabled = Bug 800872 - Test failure "Window number '1' has been opened"
>  [include:testTabbedBrowsing_PinAndUnpinAppTab/manifest.ini]
> +[include:testVideo_OGVBuffering/manifest.ini]
> +

nit: please remove this extra empty newline

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +10,5 @@
> +var tabs = require("../../../lib/tabs");
> +
> +const TEST_DATA = "http://www.mozqa.com/data/firefox/video/" +
> +                  "test_ogv_video_60s.html";
> +const SKIP_VALUE = "5";

nit: I'd leave this as an integer value

@@ +46,5 @@
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip through the video using the UI ");
> +      // If the video has reached the end go back to start
> +      if (currentTime.getNode().value >= VIDEO_LENGTH) {
> +        controller.type(seekField, "1");

We don't need this at all, seekField is already set to 0, so clicking on the seekButton directly works nicely

Comment 24

[mario garbi]

Attachment: newHTML5VideoTest_v4.patch

Made the changes requested and tested again with latest Nightly on Linux:

http://mozmill-crowd.blargon7.com/#/endurance/report/40742e0746fd767e6d2fd586593d5bda

Comment 25

[Andrei Eftimie]

Comment on attachment 8355511 [details] [diff] [review]
newHTML5VideoTest_v4.patch

Review of attachment 8355511 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Lets get this in.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/fbd27ae80074

Comment 26

[Andrei Eftimie]

I think we're done here.

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

In comment 22 I specifically asked for a final review from either myself or Henrik. Why was this landed without this?

Comment 28

[Andrei Eftimie]

(In reply to Dave Hunt (:davehunt) from comment #27)
> In comment 22 I specifically asked for a final review from either myself or
> Henrik. Why was this landed without this?

Sorry about that. I think I missed this.

Lets get a further review + followup patch then, if necessary.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I would prefer a backout until this has passed review from Henrik. Please take extra care when landing patches for new tests to be sure it has has a final review from myself or Henrik (most likely Henrik, due to the recent organisational changes).

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8355511 [details] [diff] [review]
newHTML5VideoTest_v4.patch

Review of attachment 8355511 [details] [diff] [review]:
-----------------------------------------------------------------

As mentioned before, please request final review from Henrik.

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +30,5 @@
> +    controller.open(TEST_DATA);
> +    controller.waitForPageLoad();
> +
> +    enduranceManager.addCheckpoint("Video is set to autoplay and " +
> +                                   "skip field set to 5 seconds");

Checkpoints add weight to the data so they should be used sparingly, and always precede a user interaction. This checkpoint is inaccurate (it mentions autoplay?), superflous, and also contains a hard-coded value even though we have a constant available.

@@ +42,5 @@
> +    skipField.getNode().value = SKIP_VALUE;
> +    controller.click(playButton);
> +
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip through the video using the UI ");

This checkpoint should clearly be inside the else statement.

@@ +45,5 @@
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Skip through the video using the UI ");
> +      // If the video has reached the end go back to start
> +      if (currentTime.getNode().value >= VIDEO_LENGTH) {
> +        controller.click(seekButton);

We need a checkpoint here to indicate we're jumping to the start of the video.

@@ +51,5 @@
> +      else {
> +        controller.click(skipButton);
> +      }
> +      // We wait for the video to start playing again before skipping
> +      assert.waitFor( function () {

nit: Space after '(' and before 'function'

@@ +53,5 @@
> +      }
> +      // We wait for the video to start playing again before skipping
> +      assert.waitFor( function () {
> +        return movie.getNode().readyState == 4;
> +      }, "Video did not finish loading after skip in time");

This could also be after a seek, so we should update the message to reflect that.

Comment 31

[Andrei Eftimie]

Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/435cdb211085 (default)

Comment 32

[mario garbi]

Attachment: newHTML5VideoTest_v5.patch

Thank you Dave for the review and I hope all is fine now.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8359761 [details] [diff] [review]
newHTML5VideoTest_v5.patch

Review of attachment 8359761 [details] [diff] [review]:
-----------------------------------------------------------------

I think we have to do some improvements to our test given that it doesn't really

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +11,5 @@
> +
> +const TEST_DATA = "http://www.mozqa.com/data/firefox/video/" +
> +                  "test_ogv_video_60s.html";
> +const SKIP_VALUE = 5;
> +const VIDEO_LENGTH = 59;

This is not the video length but the max seek position, right?

@@ +27,5 @@
> +function testVideo_OGVBuffering() {
> +  enduranceManager.run(function () {
> +    enduranceManager.addCheckpoint("Load a web page with HTML5 Video");
> +    controller.open(TEST_DATA);
> +    controller.waitForPageLoad();

The question here is, do we also have to wait for the canplaythrough event for the video? As of now we would start immediately and seek through the stream. What happens if the file hasn't been fully loaded yet? Or is exactly that necessary to reproduce the behavior, means seeking to a position in the video which is not available yet?

@@ +41,5 @@
> +    controller.click(playButton);
> +
> +    enduranceManager.loop(function () {
> +      // If the video has reached the end go back to start
> +      if (currentTime.getNode().value >= VIDEO_LENGTH) {

I would exchange the conditions and check for '< MAX_SEEK_POSITION - SKIP_VALUE'

@@ +52,5 @@
> +      }
> +      // We wait for the video to start playing again before skipping
> +      assert.waitFor(function () {
> +        return movie.getNode().readyState == 4;
> +      }, "Video has loaded and skipped to the expected timeframe");

The original steps are talking about to keep the video playing while the memory increased. I don't see this here given that we immediately continue to seek to the next position. IMO we really don't need the entities in this endurance test, when we would follow the original request which is loading the page, seeking to 30s, and continue playing. Doing it 10 times via the iterations should be totally enough.

Comment 34

[mario garbi]

Attachment: newHTML5VideoTest_v6.patch

Made the changes required in the previous review.

http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be389636

Comment 35

[mario garbi]

(In reply to Henrik Skupin (:whimboo) from comment #33)
> Comment on attachment 8359761 [details] [diff] [review]

> > +    controller.waitForPageLoad();
> 
> The question here is, do we also have to wait for the canplaythrough event
> for the video? As of now we would start immediately and seek through the
> stream. What happens if the file hasn't been fully loaded yet? Or is exactly
> that necessary to reproduce the behavior, means seeking to a position in the
> video which is not available yet?

We need the video to buffer when we skip, therefore we can't let it load completely.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Ok, but shouldn't we wait for the canplay event then for the initial play click? Otherwise it could be that we start seeking while the video is not in playing state.

https://developer.mozilla.org/en-US/docs/Web/Reference/Events/canplay

Comment 37

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8361042 [details] [diff] [review]
newHTML5VideoTest_v6.patch

Review of attachment 8361042 [details] [diff] [review]:
-----------------------------------------------------------------

Besides Henrik's comment above:

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +11,5 @@
> +
> +const TEST_DATA = "http://www.mozqa.com/data/firefox/video/" +
> +                  "test_ogv_video_60s.html";
> +const SKIP_VALUE = 5;
> +const MAX_SEEK_POSITION = 59;

Please order these 2 alphabetically. They can be separate from TEST_DATA.

@@ +39,5 @@
> +
> +    skipField.getNode().value = SKIP_VALUE;
> +    controller.click(playButton);
> +
> +    if (currentTime.getNode().value >= MAX_SEEK_POSITION - SKIP_VALUE) {

This should be currentTime.getNode().value <= MAX_SEEK_POSITION - SKIP_VALUE

@@ +47,5 @@
> +    else {
> +      enduranceManager.addCheckpoint("Jumping to the start of the video");
> +      controller.click(seekButton);
> +    }
> +    // We wait for the video to start playing again before skipping

Blank line before this comment.

Comment 38

[mario garbi]

Attachment: newHTML5VideoTest_v7.patch

http://mozmill-crowd.blargon7.com/#/endurance/report/94e33fe3d7ec0be6dbbfa0a7028a9818

Comment 39

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8362531 [details] [diff] [review]
newHTML5VideoTest_v7.patch

Review of attachment 8362531 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +37,5 @@
> +    var skipField = new elementslib.ID(controller.window.document, "skipField");
> +    var skipButton = new elementslib.ID(controller.window.document, "skip");
> +
> +    // We wait for the video to start playing before skipping
> +    assert.waitFor(function () {

The comment is wrong as this is not autoplay.

@@ +45,5 @@
> +    skipField.getNode().value = SKIP_VALUE;
> +    controller.click(playButton);
> +
> +    if (currentTime.getNode().value <= MAX_SEEK_POSITION - SKIP_VALUE) {
> +      enduranceManager.addCheckpoint("Skip through the video using the UI ");

I don't see why we have to make this difference since there are no entities anymore. At each iteration we open the page and skip only once. Could it take that much to load before that this would make sense?

@@ +54,5 @@
> +      controller.click(seekButton);
> +    }
> +
> +    // We wait for the video to start playing again before skipping
> +    assert.waitFor(function () {

The comment here too since there are no loops anymore.

Comment 40

[mario garbi]

Attachment: newHTML5VideoTest_v8.patch

Thank you Andreea for catching those, I have updated the patch and I hope it is all ok now.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8362531 [details] [diff] [review]
newHTML5VideoTest_v7.patch

Review of attachment 8362531 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +9,5 @@
> +var endurance = require("../../../lib/endurance");
> +var tabs = require("../../../lib/tabs");
> +
> +const MAX_SEEK_POSITION = 59;
> +const SKIP_VALUE = 5;

The TEST_DATA constant should come first as a separate block. Other constants (blocks) will be added later, whereby each section is sorted alphabetical.

@@ +38,5 @@
> +    var skipButton = new elementslib.ID(controller.window.document, "skip");
> +
> +    // We wait for the video to start playing before skipping
> +    assert.waitFor(function () {
> +      return movie.getNode().readyState == 4;

Can we use the public constant for this state? I believe there is one.

@@ +41,5 @@
> +    assert.waitFor(function () {
> +      return movie.getNode().readyState == 4;
> +    }, "Video data is available");
> +
> +    skipField.getNode().value = SKIP_VALUE;

I would move this down to the section where we really need this value set.

@@ +45,5 @@
> +    skipField.getNode().value = SKIP_VALUE;
> +    controller.click(playButton);
> +
> +    if (currentTime.getNode().value <= MAX_SEEK_POSITION - SKIP_VALUE) {
> +      enduranceManager.addCheckpoint("Skip through the video using the UI ");

The comment from Andreea even tells me that we might have to clean the cache before we can do this test, and probably between each iteration too. Otherwise the video will already be buffered in subsequent iterations. I think having a checkpoint would be good to see what happened between the page load the having enough video data for play.

But yes, there is no inner loop anymore. So not sure why this is still in here.

@@ +50,5 @@
> +      controller.click(skipButton);
> +    }
> +    else {
> +      enduranceManager.addCheckpoint("Jumping to the start of the video");
> +      controller.click(seekButton);

Is this really necessary when the page gets reloaded? The video should be at position 0 by default.

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8362572 [details] [diff] [review]
newHTML5VideoTest_v8.patch

Review of attachment 8362572 [details] [diff] [review]:
-----------------------------------------------------------------

Parts of my review of the last patch are still applying to the new patch.

Comment 43

[mario garbi]

Attachment: newHTML5VideoTest_v9.patch

Updated patch as requested.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8363679 [details] [diff] [review]
newHTML5VideoTest_v9.patch

Review of attachment 8363679 [details] [diff] [review]:
-----------------------------------------------------------------

Please get the reviews done internally first. Thanks.

Comment 45

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8363679 [details] [diff] [review]
newHTML5VideoTest_v9.patch

Review of attachment 8363679 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8363679 [details] [diff] [review]
newHTML5VideoTest_v9.patch

Review of attachment 8363679 [details] [diff] [review]:
-----------------------------------------------------------------

Just one more thing to fix. Once done I'm ok with it. Please update as requested inline.

::: firefox/tests/endurance/testVideo_OGVBuffering/test1.js
@@ +53,5 @@
> +
> +    // We wait for the video to start playing again
> +    assert.waitFor(function () {
> +      return movie.getNode().readyState === movie.getNode().HAVE_ENOUGH_DATA;
> +    }, "Video has loaded and skipped to the expected timeframe");

I would have expect a final checkpoint here so we know when we have started playing again.

Comment 47

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v9.1

Updated the patch to add the checkpoint. 
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/305c62836f52 (default)

Thanks Mario for your work here!

Comment 48

[u279076]

Glad to see this has finally been resolved. Any chance this can be uplifted so we can stop running this check manually in Beta?

Comment 49

[Andrei Eftimie]

This is already in Aurora as it has been landed prior to the recent merge.

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/56024e589c42 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/16962ad807b9 (mozilla-release)