Create simplified video controls in html and javascript and automated/validate their usage with mozmill tests. diff against mozmill-tests repo included.

Mikeal, you forgot the patch! :)

Mikeal, it doesn't work at all for me. Mozmill doesn't start the test due to a possible not running HTTP server? This message I get: HTTP/1.1 200 OK connection: close server: httpd.js date: Thu, 23 Apr 2009 14:48:28 GMT content-length: 0

(In reply to comment #3) > Mikeal, it doesn't work at all for me. Mozmill doesn't start the test due to a > possible not running HTTP server? This message I get: > > HTTP/1.1 200 OK connection: close server: httpd.js date: Thu, 23 Apr 2009 > 14:48:28 GMT content-length: 0 This is filed as bug 489807. Will have a look at your test now.

Comment on attachment 373948 [details] [diff] [review] intial video controls mozmill tests >diff -r fd0cd7b00bdd firefox/testVideo/files/usercontrols.html I think we should better use firefox/video/ here. We don't add a test portion to the folder names. >const ID = elementslib.ID; Can we use Id here? >+seek = function (position) { Please remove the space before the brackets. >+document.getElementById("playDiv").onclick = function(e) {v.play()} >+document.getElementById("pauseDiv").onclick = function(e) {v.pause()} >+document.getElementById("reloadDiv").onclick = function(e) {v.load()} Separate rows for the function body please. >+document.getElementById("seekDiv").onclick = function(e) { >+ seek(document.getElementById('seekField').value); >+ } Please fix the indentation. >\ No newline at end of file *snip* >+ module.d = controller.tabs.activeTab; module.tab? >+ module.v = module.d.getElementById("v1"); module.video? Just to make it better readable. >+ module.timeupdate = module.d.getElementById("timeupdate") timeUpdate >+ controller.sleep(500); >+ module.w = module.d.defaultView; Why we need a sleep here? >+var testPlayPause = function(){ Can you put appropriate comments in-front of the function definitions? And put a space before the bracket. >+ // controller.waitForEval("subject.textContent == 'play'", >+ // undefined, undefined, currentStatus); >+ controller.sleep(1500); >+ controller.click(new ID(d, "pauseDiv")); >+ controller.sleep(1500); >+ // controller.waitForEval("subject.textContent == 'pause'", >+ // undefined, undefined, currentStatus); So you wanna go with sleep instead of waitForEval? If yes, how do you check the current status? >+var testSeekDuringPlay = function () { >+ controller.click(new ID(d, "playDiv")); >+ controller.sleep(4000); Do we really play the movie? You don't check it here too. IMO it would be a neat way to create separate functions for doPlay, doPause, doSeek, and others. >+ controller.type(new ID(d, "seekField"), "2"); >+ controller.click(new ID(d, 'seekDiv')); >+ controller.sleep(1000); >+ controller.waitForEval( >+ "subject[0] == 'seeking' || subject[1] == 'seeked'", >+ undefined, undefined, JSON.parse(allEvents.textContent)); Do we really need the sleep here? >+ controller.click(new ID(d, "playDiv")); >+ if (currentStatus.textContent == 'play') { >+ throw "Video did not keep playing through, status changed"; >+ } I wonder why we have to explicitly hit play again. Is it intended that seeked stays until another user input is fired? >+var testSeekDuringPause = function () { nit: spacing >+var testPlayLinkPauseLink = function(){ >+ controller.click(new ID(d, "playLink")); >+ controller.waitForEval("subject.textContent == 'play'", >+ undefined, undefined, currentStatus); >+ controller.sleep(1000); >+ controller.click(new ID(d, "pauseLink")); >+ controller.waitForEval("subject.textContent == 'pause'", >+ undefined, undefined, currentStatus); >+} It's identical to the div version. We should really try to get this into one function and decide to use which variant by the given id parameter? >+var testSeekLinkDuringPlay = function () { >+ controller.click(new ID(d, "playLink")); >+ controller.sleep(4000); >+ allEvents.textContent = "[]"; >+ controller.type(new ID(d, "seekField"), "2"); >+ controller.click(new ID(d, 'seekLink')); >+ controller.sleep(1000); >+ controller.waitForEval( >+ "subject[0] == 'seeking' || subject[1] == 'seeked'", >+ undefined, undefined, JSON.parse(allEvents.textContent)); >+ if (timeupdate.textContent < 1.98 || timeupdate.textContent > 3) { >+ throw "timeupdate is out of range for seek :: "+timeupdate.textContent; >+ } >+ controller.click(new ID(d, "playLink")); >+ if (currentStatus.textContent == 'play') { >+ throw "Video did not keep playing through, status changed"; >+ } >+ controller.click(new ID(d, "pauseLink")); >+ controller.waitForEval("subject.textContent == 'pause'", >+ undefined, undefined, currentStatus); >+} Duplicated code. I really would like to see it as one function too with appropriate parameters for play and pause. >+var testSeekLinkDuringPause = function () { >+ controller.click(new ID(d, "playLink")); >+ controller.click(new ID(d, "pauseLink")); >+ controller.waitForEval("subject.textContent == 'pause'", >+ undefined, undefined, currentStatus); >+ controller.sleep(1000); >+ allEvents.textContent = "[]"; >+ controller.type(new ID(d, "seekField"), "6"); >+ controller.click(new ID(d, 'seekLink')); >+ controller.sleep(1000); >+ controller.waitForEval( >+ "subject[0] == 'seeking' || subject[1] == 'seeked'", >+ undefined, undefined, JSON.parse(allEvents.textContent)); >+ if (timeupdate.textContent < 5.3 || timeupdate.textContent > 6.9) { >+ throw "timeupdate is out of range for seek :: "+timeupdate.textContent; >+ } >+} dito. >+ var approxEvents = ["emptied","loadstart","durationchange","loadedmetadata","canplay"]; >+ var sEvents = JSON.parse(allEvents.textContent); >+ for (i in approxEvents) { >+ if (approxEvents[i] != sEvents[i]) { >+ throw "allEvents do not match. \n expected :: "+JSON.stringify(approxEvents)+'\n received :: '+allEvents.textContent That fails too. The list you set for approxEvents doesn't match the one of allEvents. For me it's ["emptied","loadstart","durationchange","loadedmetadata","play","waiting","canplay","canplaythrough","load"] >\ No newline at end of file *snip* While running the test for the first time I got several failed tests but those are not reproducible anymore. Mikeal, probably you should wait with the test until canplaythrough has been fired to make sure we have enough data buffered for the test. Otherwise great test. And yeah, please also add the license plates. :)

Fixed issue with unreliable waitFor play/pause state. Added new div to only handle play/pause changes. I'm not doing the formatting changes and the other abstractions, those are all "nice to haves" that we can do later if we wish but we shouldn't be creating this kind of barrier to contribution as we need many many more tests.

Attempted fix for error Clint is seeing waiting for seek. Adding better debugging to sync failure in the case it does fail.

Getting fixes in to every function.

I get a couple of failed results when running the test via jsbridge: est Failed : testSeekDuringPlay in /Volumes/data/projects/mozmill-tests/firefox/testVideo/testVideo.js Test Failed : testSeekDuringPause in /Volumes/data/projects/mozmill-tests/firefox/testVideo/testVideo.js Test Failed : testSeekLinkDuringPlay in /Volumes/data/projects/mozmill-tests/firefox/testVideo/testVideo.js Test Failed : testSeekLinkDuringPause in /Volumes/data/projects/mozmill-tests/firefox/testVideo/testVideo.js Test Failed : testLoad in /Volumes/data/projects/mozmill-tests/firefox/testVideo/testVideo.js Passed 3 :: Failed 5 :: Skipped 0 Mikeal, can you please check this?

Yeah, those a new, i started seeing them when i updated Shiretoko today. The way I'm waiting for a lot of these events is too brittle, I'm going to write a better and more reliable way of waiting for specified events on specific elements.

Ok, i got it all working now. This was a lot harder than it should have been. Accessing functions defined in content windows is a lot harder than it should be, we should write a better abstraction for that.

Much more reliable waiting for multiple events to fire.

Mikeal, I've applied the patch to my local tree and when trying to run the test I get a couple of failures: Error: $ is not defined Source File: http://localhost:43336/d8d177417f79-5048-8686-ec9d6a6fa966/usercontrols.html Line: 100 Error: Error: timeout exceeded for waitForEval('subject.textContent == 'play'') Source File: file:///Volumes/data/build/mozmill/trunk/mozmill/extension/resource/modules/frame.js Line: 411 Error: $ is not defined Source File: http://localhost:43336/d8d177417f79-5048-8686-ec9d6a6fa966/usercontrols.html Line: 89 Even starting the test from the console quits with the following error message which I will add as attachment.

Mikeal, the jquery-1.3.2.min.js file is missing in your patch.

Mikeal, will you have an updated version soon? The latest one which I have fails in a couple of tests.

New patch, working with latest mozmill code. Added tests for 3 video related bugs.

Comment on attachment 387495 [details] [diff] [review] Video Tests Mikeal, those tests looks great! And it's running nearly perfect for me. I have only some small things: * Your video test will fail with the last check if you have cleared the cache before starting the test: "fail :: allEvents do not match. expected :: ". I can clearly reproduce this failure. * The tests you have written for the bugs should really be mochitests and not covered on the Mozmill side. Do you think that you can convert those? Nits and questions: * Why are you using jQuery? You are using getElementById all the time when $(...) could be used. Or get I something wrong? * You should update the license part of the video test file so it matches the other tests. I don't think that Adam is the original author. * Why do you use 'var MODULE_NAME = "testVideo"' in your video test? We don't have such a declaration in all of our tests. Is that needed somehow? * Can you remove the commented out code? That irritates when reading the code. r- due to the breaking test from the first list item.

(In reply to comment #19) > * The tests you have written for the bugs should really be mochitests and not > covered on the Mozmill side. Do you think that you can convert those? They are very synchronous and I don't know enough about mochitest to replicate them. > Nits and questions: > > * Why are you using jQuery? You are using getElementById all the time when > $(...) could be used. Or get I something wrong? getElementById is still faster than jQuery. I'm using jQuery mostly for creating new elements cause it makes that super easy. > * Why do you use 'var MODULE_NAME = "testVideo"' in your video test? We don't > have such a declaration in all of our tests. Is that needed somehow? It's needed if you want to depend on the test. It's how you reference a test by name.

(In reply to comment #21) > (In reply to comment #19) > > * The tests you have written for the bugs should really be mochitests and not > > covered on the Mozmill side. Do you think that you can convert those? > > They are very synchronous and I don't know enough about mochitest to replicate > them. Chris, can we use the created Mozmill test and convert them to Mochitests? Or do you see problems too? > > * Why are you using jQuery? You are using getElementById all the time when > > $(...) could be used. Or get I something wrong? > > getElementById is still faster than jQuery. I'm using jQuery mostly for > creating new elements cause it makes that super easy. Interesting. Will remember that. Thanks. Will test your updated video test shortly and report back.

Comment on attachment 387587 [details] [diff] [review] video tests >+++ b/firefox/testVideo/files/jquery-1.3.2.min.js Wed Jul 08 20:06:24 2009 -0700 >@@ -0,0 +1,19 @@ >+/* >+ * jQuery JavaScript Library v1.3.2 >+ * http://jquery.com/ Mikeal, I wonder if we should place this file under shared modules so other tests can also use the jQuery library. Would that be possible or can httpd only access files located under the given resource? >+++ b/firefox/testVideo/files/usercontrols.html Wed Jul 08 >+// The Original Code is Mozilla Corporation Code. >+// >+// The Initial Developer of the Original Code is >+// Mikeal Rogers. >+// Portions created by the Initial Developer are Copyright (C) 2008 >+// the Initial Developer. All Rights Reserved. The initial developer is Moco. See this test: http://hg.mozilla.org/qa/mozmill-tests/file/d0a067615201/firefox/testFindInPage/testFindInPage.js >+$('<div id="asdfasdfasd"></div>').appendTo(document.body); That was only for debug purposes? I don't see that it is used anywhere. >diff -r 96ec86aadead firefox/testVideo/testBugs.js Can you please update for all the test files the license plate so it matches the example given in the above test? >+var mozmill = {}; Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill); >+var elementslib = {}; Components.utils.import('resource://mozmill/modules/elementslib.js', elementslib); Those two lines are not needed anymore. >+var RELATIVE_ROOT = '.'; >+var MODULE_REQUIRES = ['testVideo']; Why has this test be dependent on testVideo? >+var setupModule = function (module) { >+ module.waitForEvents = module.collector.getModule('testVideo').waitForEvents; >+ module.getWindowForActiveTab = module.collector.getModule('testVideo').getWindowForActiveTab; >+ module.url = module.collector.getModule('testVideo').url; >+ module.controller = mozmill.getBrowserController(); The only variable we use from testVideo is url but we should declare it independently in this test. waitForEvent and getwindowForActiveTab is never used. Please get rid of the dependency. >\ No newline at end of file Nearly each file misses that. >diff -r 96ec86aadead firefox/testVideo/testVideo.js >+var testSeekDuringPlay = function () { [...] >+ controller.click(new ID(d, 'seekDiv')); >+ seekWait.wait(); >+ if (timeupdate.textContent < 1.98 || timeupdate.textContent > 3) { Do we really seek so unreliable? Are this the keyframe positions in this video? >+var testSeekDuringPause = function () { [...] >+ if (timeupdate.textContent < 5.3 || timeupdate.textContent > 6.9) { The timeframe is getting longer. I would really be interested why you are using those values. >+ var seekWait = new mozmill.waitFor(v, ["seeking", "seeked"]); [...] >+ seekWait.wait(); This is something new for me. I've never seen this object. And I really don't have an idea for what I can be used for. It should be renamed to waitForEvents. See bug 503487. >+var testLoad = function () { >+ var validationEvents = ["emptied","loadstart","durationchange","loadedmetadata","canplay"]; >+ var validationWait = new mozmill.waitFor(v, validationEvents); [...] >+ validationWait.wait(); This test works fine now. The mentioned error I cannot see anymore. We should really have it in the next iteration.

Oh and the main reason I forgot. The last test fails on my Windows box: fail :: Reload failed, position too high == 5.999000072479248 Looks like that the checks are starting too early. When is the position reseted to 0? Directly when the load function is called?

Mikeal, shall I take a look at your test and try to fix the remaining failure? Would be nice to have this test in the repository because it is one of the missing smoketest ones.

Comment on attachment 387587 [details] [diff] [review] video tests I have checked those tests again and all are working fine. On 1.9.2 we also have a regression which we catched with your test Mikeal! I'll land it today in the repository! Thanks for your work.

Comment on attachment 387587 [details] [diff] [review] video tests Sorry for the back and forward. While going over the test some parts have really to be updated. Mikeal, I will upload an update soon.

Mikeal, can you please check? It works fine now and surprisingly much faster.

Tony, can you please check which manual tests for user video controls we wanna have on Litmus and add those. Thanks.

Bug 593704 has a nice testcase video file we could use for the tests here.

(In reply to comment #32) > Bug 593704 has a nice testcase video file we could use for the tests here. Do you want this cloned to litmus-data and mozmill-tests/firefox/test-files?

(In reply to comment #33) > Do you want this cloned to litmus-data and mozmill-tests/firefox/test-files? There will be a single patch for any related test data file and all tests. Once the test has been checked in we can update Litmus tests as necessary.

(In reply to comment #34) > (In reply to comment #33) > > Do you want this cloned to litmus-data and mozmill-tests/firefox/test-files? > > There will be a single patch for any related test data file and all tests. Once > the test has been checked in we can update Litmus tests as necessary. I'll create a patch for litmus-data first. Where should this live? I propose litmus-data/firefox/videos. Any objections?

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment on attachment 404706 [details] [diff] [review] Final patch Probably bit-rotted meanwhile. Lets pull review request for now and investigate what else we would have to do.

The following MozTrap Smoketest covers some more video types we probably should cover. I would like to see such a test checked-in until end of this month. Lets get it tracked. https://moztrap.mozilla.org/manage/case/266/

This bug is a quarterly goal so lets try to get it in by end of this week.

This is a wip patch for the video tests. It has the test files attached because the dependency is not yet landed. It is the most complex test, featuring all controls and a plugin check, so i would like to refine this until r+ and then just cut/replace stuff in it for the other 3 tests.

Comment on attachment 668405 [details] [diff] [review] wip v0.1 Review of attachment 668405 [details] [diff] [review]: ----------------------------------------------------------------- I don't think there is a need to include binary files while those are not available yet. We are at a feedback level and do not test the actual code. Beside my given comments I think it's a good start and those tests will become a good proof. ::: data/video/test_mov_video_nosound.html @@ +24,5 @@ > + <a href="#" onclick="stop()" id="stop">stop</a> > + <input type="number" id="seekField" /> > + <a href="#" onclick="seek()" id="seek">seek</a> > + > + <script type="text/javascript"> Please place JS code in the head but not body section. ::: tests/remote/testVideo/manifest.ini @@ +1,1 @@ > +[testVideoMOV.js] I think we want to name it 'testQuicktime.js'. If necessary we can add 'Mov' as suffix but I don't think it's necessary yet. ::: tests/remote/testVideo/testVideoMOV.js @@ +4,5 @@ > + > + /* For Windows and Mac OS X, this test needs the latest QuickTime plugin > + * installed from http://support.apple.com/downloads/#quicktime > + * For Linux this test needs the latest Totem plugin > + * installed from http://projects.gnome.org/totem/ */ Thinking more about it I believe we should put together a list of required external software in the root level readme file. @@ +16,5 @@ > + > +function setupModule() { > + controller = mozmill.getBrowserController(); > + > + tabs.closeAllTabs(controller); Put this last in setupModule(). There is no need to do it, if we can't run the test. @@ +21,5 @@ > + > + var isQuickTimeActive = false; > + var plugins = controller.window.navigator.plugins; > + for (var i = 0; i < plugins.length; i++) { > + if (plugins[i].name.indexOf("QuickTime") !== -1) { I don't think we wanna do that via navigator.plugins. We have the AddonManager for exactly this kind of retrieval. @@ +47,5 @@ > + controller.waitForPageLoad(); > + > + var movie = new elementslib.ID(controller.window.document, "movie"); > + assert.equal(movie.getNode().wrappedJSObject.GetTime(), 0, > + "The movie is not playing"); To make this easier I would suggest that you add a numeric field in the testcase file which retrieves the current time via a timer each 100ms or so. That way we do not have to make such workarounds with wrappedJSObject which is even evil as mentioned in last weeks Ask an Expert meeting. @@ +49,5 @@ > + var movie = new elementslib.ID(controller.window.document, "movie"); > + assert.equal(movie.getNode().wrappedJSObject.GetTime(), 0, > + "The movie is not playing"); > + > + var play = new elementslib.ID(controller.window.document, "play"); I would define all elements at once at the beginning. @@ +62,5 @@ > + var seek = new elementslib.ID(controller.window.document, "seek"); > + controller.click(seek); > + assert.waitFor(function(){ > + return (movie.getNode().wrappedJSObject.GetTime() > 8000); > + }, "The movie is playing beyond the 8 seconds mark"); I would not trust that test. If seek fails the movie is still playing and could reach this time. It's less likely because we jump to 8s but I wouldn't bet for. @@ +67,5 @@ > + > + var pause = new elementslib.ID(controller.window.document, "pause"); > + controller.click(pause); > + assert.equal(movie.getNode().wrappedJSObject.GetRate(), 0, > + "The movie is paused"); Same as for the current time. Create a control which displays that value in the testcase page.

Now that bug 795831 landed, can we agree on a strategy to test video controls, a scenario with clear steps, before writing more code?

(In reply to Alex Lakatos[:AlexLakatos] from comment #42) > Now that bug 795831 landed, can we agree on a strategy to test video > controls, a scenario with clear steps, before writing more code? The MozTrap cases are referenced and it's up to you now to propose steps for a solid test of video controls for different video formats. So please come back to us with your proposal.

(In reply to Henrik Skupin (:whimboo) from comment #43) > (In reply to Alex Lakatos[:AlexLakatos] from comment #42) > > Now that bug 795831 landed, can we agree on a strategy to test video > > controls, a scenario with clear steps, before writing more code? > > The MozTrap cases are referenced and it's up to you now to propose steps for > a solid test of video controls for different video formats. So please come > back to us with your proposal. The moztrap testcases are summary ones that just state "Test playback of videoType" ,there are no clear steps to follow. I have already proposed one scenario in my wip patch, but that seems it could not be trusted. Do we want to reiterate on that scenario or throw it away and come with a new one? Just in case we want to reiterate on it, here are the new proposed steps: 1. Open test page 2. Check the video is not already playing 3. Play the video 4. Pause the vide 5. Stop the video 6. Seek the video. Note that not all the steps are supported by all the video files, so features not available will not be tested. Do these sound ok?

(In reply to Alex Lakatos[:AlexLakatos] from comment #44) > 4. Pause the vide > 5. Stop the video > 6. Seek the video. I would also seek the video while it's playing or in paused state and not only in the stopped state. Otherwise sounds fine with me.

Regarding the talk in Ask an expert about seek behavior, for MOV file: When the movie is playing, if we seek, it will jump to the selected time and immediately start playing from there. If it's paused it will just jump there and remained paused. If it's stopped, seek is not working for Linux at least, for OS X it jumps there and remains stopped/paused. So we can seek for each state of the movie with the correspondent behavior. For WMP seek is not working at all, this is known in bug 795831. For the other 2 formats I'm waiting for the dependency to get fixed. An anomaly on OS X for the MOV file: if I hit stop when the video is playing, it will rewind to the beginning and start playing. If I hit pause first and then stop, it will just stop. So the seek/stop methods depend on the state of the video here, if it's playing, that is what will happen also after.

For OGV and WEBM we don't have stop, but with play/pause + seek is the same (if playing when seek, jumps and plays, for paused when seek, just jumps). At least here we have the time showed in a field, no need for getTime().

Lets ask Dave for his feedback on the last two comments given that he was the one mostly reviewing the code.

I'm not sure what value I can provide here as I'm not familiar with the expected behaviour of video formats on various platforms. We should determine the expected behaviour and write tests to check that. Comment 46 and comment 47 document the current behaviour, but we should determine if this matches expectations.

I checked and is the same behaviour for all platforms. I will add seek after each playing state to check it's respected.

This works fine on OS X and Linux, for Windows I still have some timing issues, could be from my slow VM, I will check more. I wanted to avoid using controller.sleep with those waitFors with smaller timeouts. WEBM on OS X sometimes takes longer to seek to 5 while playing (and after to keep playing), that is why that waitFor does not have smaller timeout. The playstate for paused is 2, but on Linux, for WMV, I get value 10, which is "Ready to be played".

Comment on attachment 708594 [details] [diff] [review] wip patch Review of attachment 708594 [details] [diff] [review]: ----------------------------------------------------------------- This is a great start. Leaving r? for Henrik as I'd also like to see his thoughts. ::: tests/remote/testVideo/testQuicktime.js @@ +35,5 @@ > + > +/** > + * Test video controls for a MOV video file > + */ > +function testVideoControlsMOV() { Should we keep the test file names consistent, and make this testVideoMOV.js rather than testQuicktime.js @@ +56,5 @@ > + }, "The movie is playing"); > + > + controller.type(seekField, "500"); > + controller.click(seek); > + assert.waitFor(function () { If the seek failed then this would ultimately pass anyway as the video is still playing. @@ +85,5 @@ > + controller.type(seekField, "8000"); > + controller.click(seek); > + assert.waitFor(function () { > + return movie.getNode().wrappedJSObject.GetRate() === 0; > + }, "The progress bar jumped to 8 seconds", 1000, 100); Is this the correct message? Would appear to me that the time has not changed.. ::: tests/remote/testVideo/testVideoWMP.js @@ +54,5 @@ > + "The movie is not playing"); > + > + controller.click(play); > + assert.waitFor(function () { > + dump(movie.getNode().wrappedJSObject.PlayState + "\n"); Debug statement? @@ +58,5 @@ > + dump(movie.getNode().wrappedJSObject.PlayState + "\n"); > + return movie.getNode().wrappedJSObject.PlayState === 2; > + }, "The movie is playing"); > + > + seekField.getNode().value = ''; Why are we setting the value to ''? If this is needed then I would suggest setting it directly to the value we want (5). @@ +92,5 @@ > + controller.click(pause); > + assert.waitFor(function () { > + return movie.getNode().wrappedJSObject.playState === 10; > + }, "The movie is paused", 1000, 100); > + } Can we add a comment to explain why we're not testing seek?

(In reply to Dave Hunt (:davehunt) from comment #52) > This is a great start. Leaving r? for Henrik as I'd also like to see his > thoughts. I mean f? :-)

Updated after Dave's feedback. To answer his questions: (In reply to Dave Hunt (:davehunt) from comment #52) > > + assert.waitFor(function () { > > If the seek failed then this would ultimately pass anyway as the video is > still playing. This is why we only wait 1 second (that should be enough to jump there and get time updated), with that timeout it won't get to 5 seconds before failing the waitFor. > > + }, "The progress bar jumped to 8 seconds", 1000, 100); > > Is this the correct message? Would appear to me that the time has not > changed.. I see it changed, the second field with current time changes after seek is pressed. But indeed we don't see a progress bar for all of the files so I changed it to "the movie jumped.." > > + > > + seekField.getNode().value = ''; > > Why are we setting the value to ''? If this is needed then I would suggest > setting it directly to the value we want (5). Setting it directly now. We had to reset it to '' before because we had 0 there after we started to play the video and when typing we got and extra 0. Wanting to type only 5, we ended up with 50.

Comment on attachment 710590 [details] [diff] [review] wip patch Review of attachment 710590 [details] [diff] [review]: ----------------------------------------------------------------- One minor nit, before otherwise this looks good to me. ::: tests/remote/testVideo/testVideoMOV.js @@ +80,5 @@ > + "The movie is not playing"); > + > + if (mozmill.isMac) { > + seekField.getNode().value = ''; > + controller.type(seekField, "8000"); Why are we using type here? We could just set the value directly as we do in other places.

Finally have a working version for all platforms. Notes: * I tested on mm-win-xp-32-3 the WMV page, manually, but it seems it's not working, I rarely see the video and the links won't start/pause it, just the buttons in the video itself. So I've skipped it for XP only. * For Linux, I tested on mm-ub-1204-32-3, because my vm was slower and it was randomly failing to start playing the videos in 5000 timeout. * On OS X, with WMV and Flip4Mac plugin, I sometimes get the page loaded, have the video canvas and the green logo from the plugin and them the canvas disappears. This also happens manually. For that issue, I've added the comment in the test, loading the page again if we fail the first time. Reports - linux, first 20-25 from the remote machine: http://mozmill-crowd.blargon7.com/#/remote/reports?branch=All&platform=Linux&from=2013-02-26&to=2013-02-26 OS X: http://mozmill-crowd.blargon7.com/#/remote/reports?branch=All&platform=Mac&from=2013-02-25&to=2013-02-25 Windows: http://mozmill-crowd.blargon7.com/#/remote/reports?branch=All&platform=Windows NT&from=2013-02-25&to=2013-02-25

Comment on attachment 718369 [details] [diff] [review] patch v1 Review of attachment 718369 [details] [diff] [review]: ----------------------------------------------------------------- Mostly just nits. I'm okay with skipping WMV on Windows XP but would like Henrik's thoughts on that. With regards to the Flip4Mac page reload issue, my concern is whether this is something that could be fixed in Firefox or in the plugin. We should file a bug for further investigation, especially if it can be reproduced manually. ::: tests/remote/testVideo/testVideoMOV.js @@ +54,5 @@ > + "The movie is not playing"); > + > + controller.click(play); > + assert.waitFor(function () { > + dump(movie.getNode().wrappedJSObject.GetTime() + "\n"); Let's remove this debug. @@ +78,5 @@ > + > + controller.click(stop); > + assert.waitFor(function () { > + return movie.getNode().wrappedJSObject.GetTime() === 0; > + }, "The movie is rewinded back to the begining", TIMEOUT); Nit: typo 'beginning' @@ +82,5 @@ > + }, "The movie is rewinded back to the begining", TIMEOUT); > + assert.equal(movie.getNode().wrappedJSObject.GetRate(), 0, > + "The movie is not playing"); > + > + if (mozmill.isMac) { Could we add a comment to explain why we do this for Mac only. @@ +87,5 @@ > + seekField.getNode().value = '8000'; > + controller.click(seek); > + assert.waitFor(function () { > + return movie.getNode().wrappedJSObject.GetRate() === 0; > + }, "The movie jumped to 8 seconds", TIMEOUT); The movie is already not playing, (GetRate is 0) so shouldn't we be waiting for GetTime to be 8000 and then an additional assert to check we're still not playing? ::: tests/remote/testVideo/testVideoOGV.js @@ +14,5 @@ > +const TIMEOUT = 1500; > + > +function setupModule(aModule) { > + controller = mozmill.getBrowserController(); > + addonsManager = new addons.AddonsManager(controller); This doesn't appear to be used. ::: tests/remote/testVideo/testVideoWEBM.js @@ +14,5 @@ > +const TIMEOUT = 1500; > + > +function setupModule(aModule) { > + controller = mozmill.getBrowserController(); > + addonsManager = new addons.AddonsManager(controller); This is not used. @@ +44,5 @@ > + seekField.getNode().value = '5'; > + controller.click(seek); > + assert.waitFor(function () { > + return parseFloat(currentTime.getNode().value) > 5; > + }, "The movie jumped to 5 seconds and is playing", TIMEOUT, 10); We're only asserting the time here, could we split the 'is playing' into a second assert? ::: tests/remote/testVideo/testVideoWMV.js @@ +78,5 @@ > + seekField.getNode().value = '5'; > + controller.click(seek); > + assert.waitFor(function () { > + return movie.getNode().wrappedJSObject.CurrentPosition > 5; > + }, "The movie jumped to 5 seconds and is playing", TIMEOUT, 10); As before, can we split the 'is playing' into a second assert?

Updated as requested, also tested again on OS X regarding the plugin issue and it's gone now, haven't fail in multiple runs (without the reload part): http://mozmill-crowd.blargon7.com/#/remote/reports?branch=All&platform=All&from=2013-04-03&to=2013-04-03

Comment on attachment 732755 [details] [diff] [review] patch v2 Review of attachment 732755 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, but I'd like to defer to Henrik for this review.

Comment on attachment 732755 [details] [diff] [review] patch v2 Review of attachment 732755 [details] [diff] [review]: ----------------------------------------------------------------- With all the plugin dependencies I'm not totally behind getting this in soon. We really might want to wait until PuppetAgain is active for all of our nodes across platforms. The maintenance costs as of now would be too much. Let us revisit the bug once we can continue. Sorry.

We need these tests to cover one of the manual regression tests: https://moztrap.mozilla.org/manage/case/11511/ There are also other formats in the test: HTML5, mp4, VP8 and VP9, and fullscreen on/off, context menu options are would also be nice to have, if possible.

Please do not move mozmill-test bugs to firefox ui tests. What you want is a new bug filed.

We now have bug 1136641 for the Firefox UI test creation. We cannot land this patch for Mozmill anymore. So I'm closing this bug as wontfix.