Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
499513002:
Add layout test and test media for infinite duration tests. (Closed)
|
Created:
6 years, 4 months ago by DaleCurtis Modified:
6 years, 4 months ago Reviewers:
scherkus (not reviewing) CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionAdd layout test and test media for infinite duration tests.
Adds a test for the corresponding Chrome side change:
http://crrev.com/291504
BUG=400442
TEST=new test!
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180867
Patch Set 1 #
Total comments: 24
Patch Set 2 : Rename. #
Total comments: 6
Patch Set 3 : Comments. #
Messages
Total messages: 15 (0 generated)
I know you haven't written tons of layout tests so apologies in advance for the flood of comments also you don't have to wait for one side to land vs. the other ... if you want to go hardcore test-driven-development stylez you can land this test and have an entry in TestExpectations that marks it as expecting to fail, then update the expectations when the chromium-side lands (it's probably not worth the trouble TBH, but wanted to highlight that it is an option) https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... File LayoutTests/http/tests/media/video-buffered-unknown-duration.html (right): https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:1: <html> add <!DOCTYPE html> -- the official doctype of html5! https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:2: <head> nit: drop empty head https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:5: <p>Load a video via preloading; verify infinite duration. Start playback and Prehaps rewords this to say "loading a file with unknown duration to verify that it contains an infinite duration blah blah blah" (the loading via preloading bit confuses me) https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:7: <video id="video" preload="auto"></video> nit: default preload attribute is auto https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:7: <video id="video" preload="auto"></video> nit: no need for an id https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:8: <script src=../../media-resources/video-test.js></script> nit: "" all attribute strings https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:14: testExpected("video.duration", Infinity, "=="); nit: javascript uses ' over " for all strings https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:17: // 1000 arbitrarily chosen, just some value > duration for testing for would duration + 1 suffice? or just compare > duration? https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:20: test("video.currentTime <= video.buffered.end(0)", true); boy oh boy does video-test.js contain some real gems -- it's not widely used in .html files and it certainly confused me because I didn't understand how this test was going to end in other words, I'd rather see explicit testExpected() code followed by a call to endTest() https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:32: function start () { nit: remove space before () https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:33: video = document.getElementById('video'); funny enough by loading the video-test.js script it does this for you (check out findMediaElement()) https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:34: video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?nph=1&name=resources/test-live.webm&throttle=100&type=video/webm"; do you need to run this through the cgi? would pointing at "/media/resources/test-live.webm" suffice? also does loading over HTTP make a difference? if not, we can plop this file into LayoutTests/media/ instead of http/tests/media/
lol, just copy pasted another layout test :) I choose poorly!
On 2014/08/22 01:17:06, DaleCurtis wrote: > lol, just copy pasted another layout test :) I choose poorly! yeah the quality of layout tests are all over the map :\
https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... File LayoutTests/http/tests/media/video-buffered-unknown-duration.html (right): https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:1: <html> On 2014/08/22 01:07:34, scherkus wrote: > add <!DOCTYPE html> -- the official doctype of html5! Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:2: <head> On 2014/08/22 01:07:33, scherkus wrote: > nit: drop empty head Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:5: <p>Load a video via preloading; verify infinite duration. Start playback and On 2014/08/22 01:07:33, scherkus wrote: > Prehaps rewords this to say "loading a file with unknown duration to verify that > it contains an infinite duration blah blah blah" > > (the loading via preloading bit confuses me) Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:7: <video id="video" preload="auto"></video> On 2014/08/22 01:07:34, scherkus wrote: > nit: default preload attribute is auto Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:7: <video id="video" preload="auto"></video> On 2014/08/22 01:07:34, scherkus wrote: > nit: no need for an id Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:8: <script src=../../media-resources/video-test.js></script> On 2014/08/22 01:07:33, scherkus wrote: > nit: "" all attribute strings Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:14: testExpected("video.duration", Infinity, "=="); On 2014/08/22 01:07:34, scherkus wrote: > nit: javascript uses ' over " for all strings Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:17: // 1000 arbitrarily chosen, just some value > duration for testing for On 2014/08/22 01:07:34, scherkus wrote: > would duration + 1 suffice? > > or just compare > duration? I want it to be immune to any future minor changes in duration parsing, so I'll just use 10 seconds. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:20: test("video.currentTime <= video.buffered.end(0)", true); On 2014/08/22 01:07:34, scherkus wrote: > boy oh boy does video-test.js contain some real gems -- it's not widely used in > .html files and it certainly confused me because I didn't understand how this > test was going to end > > in other words, I'd rather see explicit testExpected() code followed by a call > to endTest() Don't want to use testExpected() since it will print the actual value of video.buffered.end() into the layout test. If timeupdates fire differently on bots etc, that would lead to flaky failures. I did ditch the magic test ending parameter :) https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:32: function start () { On 2014/08/22 01:07:33, scherkus wrote: > nit: remove space before () Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:33: video = document.getElementById('video'); On 2014/08/22 01:07:34, scherkus wrote: > funny enough by loading the video-test.js script it does this for you (check out > findMediaElement()) Done. https://codereview.chromium.org/499513002/diff/1/LayoutTests/http/tests/media... LayoutTests/http/tests/media/video-buffered-unknown-duration.html:34: video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?nph=1&name=resources/test-live.webm&throttle=100&type=video/webm"; On 2014/08/22 01:07:33, scherkus wrote: > do you need to run this through the cgi? > > would pointing at "/media/resources/test-live.webm" suffice? > > also does loading over HTTP make a difference? if not, we can plop this file > into LayoutTests/media/ instead of http/tests/media/ Looks like it works either way, moved.
lgtm with nits! don't forget CL description prior to landing https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-buffered-unknown-duration.html (right): https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-buffered-unknown-duration.html:6: <video preload></video> nit: drop preload attribute altogether (absence is still treated as auto) https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-buffered-unknown-duration.html:19: testExpected('video.buffered.end(0)', 10, '<'); good test future proofing thing: add a testExpected() / check that verifies 10 is indeed larger than the duration if anyone ever swaps the test file it'll catch it https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-buffered-unknown-duration.html:34: video.src = 'resources/test-live.webm'; you should use findMediaFile() (check out other tests)
Other change didn't land last night, so still waiting. This will cause a DCHECK failure crash w/o it. https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... File LayoutTests/media/video-buffered-unknown-duration.html (right): https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-buffered-unknown-duration.html:6: <video preload></video> On 2014/08/22 02:38:41, scherkus wrote: > nit: drop preload attribute altogether (absence is still treated as auto) Done. https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-buffered-unknown-duration.html:19: testExpected('video.buffered.end(0)', 10, '<'); On 2014/08/22 02:38:41, scherkus wrote: > good test future proofing thing: add a testExpected() / check that verifies 10 > is indeed larger than the duration > > if anyone ever swaps the test file it'll catch it duration == infinite, so I can't actually check that :) See the check for infinite duration above. https://codereview.chromium.org/499513002/diff/20001/LayoutTests/media/video-... LayoutTests/media/video-buffered-unknown-duration.html:34: video.src = 'resources/test-live.webm'; On 2014/08/22 02:38:41, scherkus wrote: > you should use findMediaFile() (check out other tests) findMediaFile won't work here since it doesn't understand webm.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/499513002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24242)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24247)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/499513002/40001
Message was sent while issue was closed.
Committed patchset #3 (40001) as 180867 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
