Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196673002:
Convert video-buffer* http tests to testharness.js (Closed)
|
Created:
4 years, 4 months ago by Srirama Modified:
4 years, 4 months ago CC:
blink-reviews, chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert video-buffer* http tests to testharness.js
Cleaning up video-buffer* http tests to
use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.
BUG=588956
Committed: https://crrev.com/7afc1aa63258c6765c67e82b77578c0713a5b507
Cr-Commit-Position: refs/heads/master@{#411796}
Patch Set 1 : fix #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 8
Patch Set 3 : manual rebaseline for video-buffered-range-contains-currentTime.html #Patch Set 4 : add comment #
Messages
Total messages: 22 (10 generated)
Description was changed from ========== WIP fix BUG= ========== to ========== WIP ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP ========== to ========== Convert video-buffer* http tests to testharness.js Cleaning up video-buffer* http tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
srirama.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com
PTAL
https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html (right): https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html:3: <script src="../resources/testharness.js"></script> Not really using this? https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html (right): https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:12: assert_equals(video.buffered.length, 1); Please expand on the changes done to this test (1 vs 2; video-file vs audio-file)
https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html (right): https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html:3: <script src="../resources/testharness.js"></script> On 2016/08/11 16:53:20, fs (OoO until Aug 15) wrote: > Not really using this? Yes, not using. I have experimented so much and finally forgot to remove these. https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html (right): https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:12: assert_equals(video.buffered.length, 1); On 2016/08/11 16:53:20, fs (OoO until Aug 15) wrote: > Please expand on the changes done to this test (1 vs 2; video-file vs > audio-file) Changed it because the buffered length is always 1. Or should we keep the fail message in expected file and investigate why the buffered length is always 1? And using audio or video isn't affecting the test result, so i have changed it to video.
https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html (right): https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:12: assert_equals(video.buffered.length, 1); On 2016/08/11 at 18:17:38, Srirama wrote: > On 2016/08/11 16:53:20, fs (OoO until Aug 15) wrote: > > Please expand on the changes done to this test (1 vs 2; video-file vs > > audio-file) > > Changed it because the buffered length is always 1. Or should we keep the fail message in expected file and investigate why the buffered length is always 1? > And using audio or video isn't affecting the test result, so i have changed it to video. To be on the safe side, it's probably better to leave it as-is - i.e with a fail and expecting 2 (and see if there's a bug on file for it, or file a new one et.c.)
https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html (right): https://codereview.chromium.org/2196673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:12: assert_equals(video.buffered.length, 1); On 2016/08/11 20:41:03, fs (OoO until Aug 15) wrote: > On 2016/08/11 at 18:17:38, Srirama wrote: > > On 2016/08/11 16:53:20, fs (OoO until Aug 15) wrote: > > > Please expand on the changes done to this test (1 vs 2; video-file vs > > > audio-file) > > > > Changed it because the buffered length is always 1. Or should we keep the fail > message in expected file and investigate why the buffered length is always 1? > > And using audio or video isn't affecting the test result, so i have changed it > to video. > > To be on the safe side, it's probably better to leave it as-is - i.e with a fail > and expecting 2 (and see if there's a bug on file for it, or file a new one > et.c.) Looking at the history (https://codereview.chromium.org/14631011/diff/1/LayoutTests/TestExpectations) this needs a big test file. https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html (right): https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html:23: video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?name=" + mediaFile + "&throttle=5000&nph=1&type=" + type; changed it, to improve the speed of the test. https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html (right): https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:21: var mediaFile = "../../../media/resources/frame_size_change.webm"; This is a big file which i found working. Is this fine or do we need to create a new one for this test? https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:23: video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?nph=1&name=" + mediaFile + "&throttle=100000&type=" + type; This is also to improve the speed of the test.
lgtm https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1292: crbug.com/487344 http/tests/media/video-buffered-range-contains-currentTime.html [ Failure ] Since this exists, you should be aware that it might show up again after the rebaseline operations completes. Another way to do this would be to skip the test, rebaseline it manually (using Tools/Scripts/webkit-patch) and then restore to previous state. https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html (right): https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered-range-contains-currentTime.html:23: video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?name=" + mediaFile + "&throttle=5000&nph=1&type=" + type; On 2016/08/12 at 14:17:48, Srirama wrote: > changed it, to improve the speed of the test. Acknowledged. https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html (right): https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:21: var mediaFile = "../../../media/resources/frame_size_change.webm"; On 2016/08/12 at 14:17:48, Srirama wrote: > This is a big file which i found working. Is this fine or do we need to create a new one for this test? Using this WFM. https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-buffered.html:23: video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?nph=1&name=" + mediaFile + "&throttle=100000&type=" + type; On 2016/08/12 at 14:17:48, Srirama wrote: > This is also to improve the speed of the test. Acknowledged.
Patchset #3 (id:60001) has been deleted
Request you to review the changes in testExpectations. https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/2196673002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1292: crbug.com/487344 http/tests/media/video-buffered-range-contains-currentTime.html [ Failure ] On 2016/08/12 15:48:48, fs (OoO until Aug 15) wrote: > Since this exists, you should be aware that it might show up again after the > rebaseline operations completes. Another way to do this would be to skip the > test, rebaseline it manually (using Tools/Scripts/webkit-patch) and then restore > to previous state. Acknowledged.
On 2016/08/12 at 18:35:08, srirama.m wrote: > Request you to review the changes in testExpectations. LGTM. It can also be good to add comment, before the commented out line, about why it's commented out, and when it's expected to be restored.
Patchset #4 (id:100001) has been deleted
On 2016/08/12 18:37:23, fs (OoO until Aug 15) wrote: > On 2016/08/12 at 18:35:08, srirama.m wrote: > > Request you to review the changes in testExpectations. > > LGTM. It can also be good to add comment, before the commented out line, about > why it's commented out, and when it's expected to be restored. Thanks, added a comment.
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/2196673002/#ps120001 (title: "add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Convert video-buffer* http tests to testharness.js Cleaning up video-buffer* http tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert video-buffer* http tests to testharness.js Cleaning up video-buffer* http tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Convert video-buffer* http tests to testharness.js Cleaning up video-buffer* http tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert video-buffer* http tests to testharness.js Cleaning up video-buffer* http tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 Committed: https://crrev.com/7afc1aa63258c6765c67e82b77578c0713a5b507 Cr-Commit-Position: refs/heads/master@{#411796} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7afc1aa63258c6765c67e82b77578c0713a5b507 Cr-Commit-Position: refs/heads/master@{#411796} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
