Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9290008:
Introduce new basic playback test. (Closed)
|
Created:
8 years, 11 months ago by DaleCurtis Modified:
8 years, 11 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, John Grabowski, Nirnimesh, acolwell+watch_chromium.org, annacc+watch_chromium.org, dennis_jeffrey, anantha, dyu1, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, imasaki1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce new basic playback test.
As titled, this brings back a basic playback test covering theora, h264,
and webm w/ and w/o audio.
BUG=110779
TEST=Ran test. Ran new unittests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119357
Patch Set 1 #Patch Set 2 : Documentation. #
Total comments: 30
Patch Set 3 : Code review fixes. #Patch Set 4 : Remove test matrix for now. #
Total comments: 14
Patch Set 5 : Code review fixes. PYAUTO_TESTS. #
Messages
Total messages: 12 (0 generated)
PTAL. Not completely happy with cohesion of media_test_utils; suggestions welcome.
http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/csv/m... File chrome/test/data/media/csv/media.json (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/csv/m... chrome/test/data/media/csv/media.json:4: "fps": 29.970, s/0// http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_basic_playback.html (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:24: logEvent(event); Log the event *first*, which both removes doubts about races, and the need to duplicate the logEvent() call in the then/else clauses. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:31: // Notify PyAuto that we've completed testing. Send test of currenTime s/nT/ntT/ http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:34: video.currentTime == video.duration); What happens if this fails? http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:43: function logEvent(evt) { Move this above the text that uses it. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:22: var d = function (s) { return decodeURIComponent(s.replace(/\+/g, ' ')); } Why var and not just function d(s) {...} ? http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_basic_playback.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_basic_playback.py:22: """PyAuto test container. See file doc string for more information.""" Does this comment buy anything? http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_basic_playback.py:33: # Find all bear media w/o video. Wat? http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_test_utils.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_test_utils.py:5: import functools missing newline http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_test_utils.py:9: """Given a loosely structured test matrix, finds media matching criteria. This whole thing seems like crazy overkill to me. http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_test_utils_unittest.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_test_utils_unittest.py:10: class MediaTestUtilsFindMediaFiles(unittest.TestCase): ...but nice test coverage, at least ;)
http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_basic_playback.html (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:29: events = events.join(','); How would PyAuto read "events" if some error occurs? http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:9: var QueryString = function () { I am not sure why we need this function? Can't we just call a startTest(videoName) from PyAuto instead of parsing the URL parameters? http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:29: } (); Does this directly call the QueryString function when the file loads? http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_basic_playback.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_basic_playback.py:43: self.assertTrue(self.ExecuteJavascript('true;')) Does this wait for the test to end? (I am just familiar with WaitUntil()...) Is there a time-out?
http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/csv/m... File chrome/test/data/media/csv/media.json (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/csv/m... chrome/test/data/media/csv/media.json:4: "fps": 29.970, On 2012/01/25 17:58:48, Ami Fischman wrote: > s/0// Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_basic_playback.html (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:24: logEvent(event); On 2012/01/25 17:58:48, Ami Fischman wrote: > Log the event *first*, which both removes doubts about races, and the need to > duplicate the logEvent() call in the then/else clauses. Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:29: events = events.join(','); On 2012/01/25 19:30:04, shadi wrote: > How would PyAuto read "events" if some error occurs? It will time out and fail. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:31: // Notify PyAuto that we've completed testing. Send test of currenTime On 2012/01/25 17:58:48, Ami Fischman wrote: > s/nT/ntT/ Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:34: video.currentTime == video.duration); On 2012/01/25 17:58:48, Ami Fischman wrote: > What happens if this fails? You mean the equality statement? The PyAuto portion of the test checks this value and will fail if it is false. If domAutomationController.send fails, we have bigger problems. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/media_basic_playback.html:43: function logEvent(evt) { On 2012/01/25 17:58:48, Ami Fischman wrote: > Move this above the text that uses it. Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... File chrome/test/data/media/html/utils.js (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:9: var QueryString = function () { On 2012/01/25 19:30:04, shadi wrote: > I am not sure why we need this function? Can't we just call a > startTest(videoName) from PyAuto instead of parsing the URL parameters? We could, in this case though this test will be going into the list of tests PyAuto runs on every platform, not just our perf builder. As such, I tried to reduce the number of automation calls to reduce execution time. In any case, this functionality is useful to have. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:22: var d = function (s) { return decodeURIComponent(s.replace(/\+/g, ' ')); } On 2012/01/25 17:58:48, Ami Fischman wrote: > Why var and not just > function d(s) {...} > ? Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/data/media/html/... chrome/test/data/media/html/utils.js:29: } (); On 2012/01/25 19:30:04, shadi wrote: > Does this directly call the QueryString function when the file loads? Yes. http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_basic_playback.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_basic_playback.py:22: """PyAuto test container. See file doc string for more information.""" On 2012/01/25 17:58:48, Ami Fischman wrote: > Does this comment buy anything? Just keeps gpylint from complaining. http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_basic_playback.py:33: # Find all bear media w/o video. On 2012/01/25 17:58:48, Ami Fischman wrote: > Wat? Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_basic_playback.py:43: self.assertTrue(self.ExecuteJavascript('true;')) On 2012/01/25 19:30:04, shadi wrote: > Does this wait for the test to end? (I am just familiar with WaitUntil()...) > Is there a time-out? Yes, the normal PyAuto automation call timeout applies. Every ExecuteJavascript invocation requires a subsequent window.domAutomationController.send() call. Here we're waiting for the test to respond back that it's completed. http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_test_utils.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_test_utils.py:5: import functools On 2012/01/25 17:58:48, Ami Fischman wrote: > missing newline Done. http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_test_utils.py:9: """Given a loosely structured test matrix, finds media matching criteria. On 2012/01/25 17:58:48, Ami Fischman wrote: > This whole thing seems like crazy overkill to me. You should have seen the generic version :) In any case, I wrote it with the intent that we'll eventually have a very large test matrix where metadata filtering will be important.
http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... File chrome/test/functional/media/media_test_utils.py (right): http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... chrome/test/functional/media/media_test_utils.py:9: """Given a loosely structured test matrix, finds media matching criteria. On 2012/01/25 23:59:25, DaleCurtis wrote: > On 2012/01/25 17:58:48, Ami Fischman wrote: > > This whole thing seems like crazy overkill to me. > > You should have seen the generic version :) In any case, I wrote it with the > intent that we'll eventually have a very large test matrix where metadata > filtering will be important. WDYT about keeping this matrix business in your back-pocket for that glorious future, and making this CL just about making the one video play?
On 2012/01/26 00:07:22, Ami Fischman wrote: > http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... > File chrome/test/functional/media/media_test_utils.py (right): > > http://codereview.chromium.org/9290008/diff/2001/chrome/test/functional/media... > chrome/test/functional/media/media_test_utils.py:9: """Given a loosely > structured test matrix, finds media matching criteria. > On 2012/01/25 23:59:25, DaleCurtis wrote: > > On 2012/01/25 17:58:48, Ami Fischman wrote: > > > This whole thing seems like crazy overkill to me. > > > > You should have seen the generic version :) In any case, I wrote it with the > > intent that we'll eventually have a very large test matrix where metadata > > filtering will be important. > > WDYT about keeping this matrix business in your back-pocket for that glorious > future, and making this CL just about making the one video play? Fine with me, I'm all for keeping code bloat down, and easy enough to drop it in later. Done.
lgtm https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... File chrome/test/data/media/html/utils.js (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/utils.js:9: var QueryString = function () { Is this any better than function QueryString() { ... ? https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/function... File chrome/test/functional/media/media_basic_playback.py (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/function... chrome/test/functional/media/media_basic_playback.py:22: # have more test videos in the matrix. Code already written, ping for details. ...or you could point to the PS#3 of this CR :)
drive-by on js code https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... File chrome/test/data/media/html/media_basic_playback.html (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:9: <video autoplay preload controls/> <video> doesn't use /> tags please change to <video></video> https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:17: var events = [] missing ; https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:20: events.push(evt.type) missing ; https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:49: video.play() missing ; https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... File chrome/test/data/media/html/utils.js (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/utils.js:26: params[d(match[1])] = d(match[2]); nit: de-indent by 1 space
but lgtm fwiw!
https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... File chrome/test/data/media/html/media_basic_playback.html (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:9: <video autoplay preload controls/> On 2012/01/26 21:52:16, scherkus wrote: > <video> doesn't use /> tags > > please change to <video></video> Done. https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:17: var events = [] On 2012/01/26 21:52:16, scherkus wrote: > missing ; Done. https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:20: events.push(evt.type) On 2012/01/26 21:52:16, scherkus wrote: > missing ; Done. https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/media_basic_playback.html:49: video.play() On 2012/01/26 21:52:16, scherkus wrote: > missing ; Done. https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... File chrome/test/data/media/html/utils.js (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/utils.js:9: var QueryString = function () { On 2012/01/26 21:40:05, Ami Fischman wrote: > Is this any better than > function QueryString() { ... > ? Yes, as that's not what QueryString contains. QueryString contains the results of the call to the anonymous function. https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/data/med... chrome/test/data/media/html/utils.js:26: params[d(match[1])] = d(match[2]); On 2012/01/26 21:52:16, scherkus wrote: > nit: de-indent by 1 space Done. https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/function... File chrome/test/functional/media/media_basic_playback.py (right): https://chromiumcodereview.appspot.com/9290008/diff/3015/chrome/test/function... chrome/test/functional/media/media_basic_playback.py:22: # have more test videos in the matrix. Code already written, ping for details. On 2012/01/26 21:40:05, Ami Fischman wrote: > ...or you could point to the PS#3 of this CR :) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/9290008/7003
Change committed as 119357 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
