Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155053002:
Convert media-play-promise.html to testharness.js (Closed)
|
Created:
4 years, 5 months ago by Srirama Modified:
4 years, 5 months ago CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert media-play-promise.html to testharness.js
Cleaning up media-play-promise.html in media/
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/b87e3bb23b068f510fcf6bd18635a7a508e5d0d2
Cr-Commit-Position: refs/heads/master@{#406243}
Patch Set 1 #
Total comments: 11
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address nit #Patch Set 4 : add suggested change #
Messages
Total messages: 20 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== fix fix BUG= ========== to ========== Convert media-play-promise.html to testharness.js Cleaning up media-play-promise.html in media/ 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/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); Should we remove these events? Or should we keep them and test with the help of eventwatcher?
https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); On 2016/07/18 at 09:36:10, Srirama wrote: > Should we remove these events? Or should we keep them and test with the help of eventwatcher? I think the interesting cases will be where events should be ordered wrt the play() promise (generally 'playing' before the promise is resolved I believe. not sure that needs to be tested over and over though...) This particular one does not look like one of those. testEventAndPromiseOrdering probably needs some care so that it will notice when the bug it references is fixed. https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:110: // suferring from a not supported error. Nit: suffering https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:268: function play(t, audio, failPromise, error) { Doesn't |failPromise|===true imply |error| is set to something? I.e could we have just |error| and not pass any third argument if it's false (and get undefined as |error|.) Another option would be to split this into two functions (which I think would make the expectations more clear at the call-site.)
https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); On 2016/07/18 10:51:02, fs wrote: > On 2016/07/18 at 09:36:10, Srirama wrote: > > Should we remove these events? Or should we keep them and test with the help > of eventwatcher? > > I think the interesting cases will be where events should be ordered wrt the > play() promise (generally 'playing' before the promise is resolved I believe. > not sure that needs to be tested over and over though...) This particular one > does not look like one of those. testEventAndPromiseOrdering probably needs some > care so that it will notice when the bug it references is fixed. Even the order mentioned in that function isn't correct right now. Updated comment to match that and removed event capturing from all other functions as they are not really checking for event order. https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:110: // suferring from a not supported error. On 2016/07/18 10:51:02, fs wrote: > Nit: suffering Done. https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:268: function play(t, audio, failPromise, error) { On 2016/07/18 10:51:02, fs wrote: > Doesn't |failPromise|===true imply |error| is set to something? I.e could we > have just |error| and not pass any third argument if it's false (and get > undefined as |error|.) > > Another option would be to split this into two functions (which I think would > make the expectations more clear at the call-site.) Done.
LGTM, but I think we need a better story around the ordering test. https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); On 2016/07/18 at 13:58:10, Srirama wrote: > On 2016/07/18 10:51:02, fs wrote: > > On 2016/07/18 at 09:36:10, Srirama wrote: > > > Should we remove these events? Or should we keep them and test with the help > > of eventwatcher? > > > > I think the interesting cases will be where events should be ordered wrt the > > play() promise (generally 'playing' before the promise is resolved I believe. > > not sure that needs to be tested over and over though...) This particular one > > does not look like one of those. testEventAndPromiseOrdering probably needs some > > care so that it will notice when the bug it references is fixed. > > Even the order mentioned in that function isn't correct right now. Updated comment to match that and removed event capturing from all other functions as they are not really checking for event order. unreached_func probably isn't the right thing to do there though? (I.e it looks weird and gives the wrong impression on what will/is expected to happen.) https://codereview.chromium.org/2155053002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:253: function playForResolvedPromise(t, audio) { Nit: Maybe playExpectingResolvedPromise (and similarly below)
https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); On 2016/07/18 14:14:43, fs wrote: > On 2016/07/18 at 13:58:10, Srirama wrote: > > On 2016/07/18 10:51:02, fs wrote: > > > On 2016/07/18 at 09:36:10, Srirama wrote: > > > > Should we remove these events? Or should we keep them and test with the > help > > > of eventwatcher? > > > > > > I think the interesting cases will be where events should be ordered wrt the > > > play() promise (generally 'playing' before the promise is resolved I > believe. > > > not sure that needs to be tested over and over though...) This particular > one > > > does not look like one of those. testEventAndPromiseOrdering probably needs > some > > > care so that it will notice when the bug it references is fixed. > > > > Even the order mentioned in that function isn't correct right now. Updated > comment to match that and removed event capturing from all other functions as > they are not really checking for event order. > > unreached_func probably isn't the right thing to do there though? (I.e it looks > weird and gives the wrong impression on what will/is expected to happen.) You are right, but i couldn't find a way to mix eventwatcher and promise to assert the event ordering. Or should i assert the event ordering with a flag for now? (updated the comment with more information). https://codereview.chromium.org/2155053002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:253: function playForResolvedPromise(t, audio) { On 2016/07/18 14:14:43, fs wrote: > Nit: Maybe playExpectingResolvedPromise (and similarly below) Done.
https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); On 2016/07/19 at 07:15:44, Srirama wrote: > On 2016/07/18 14:14:43, fs wrote: > > On 2016/07/18 at 13:58:10, Srirama wrote: > > > On 2016/07/18 10:51:02, fs wrote: > > > > On 2016/07/18 at 09:36:10, Srirama wrote: > > > > > Should we remove these events? Or should we keep them and test with the > > help > > > > of eventwatcher? > > > > > > > > I think the interesting cases will be where events should be ordered wrt the > > > > play() promise (generally 'playing' before the promise is resolved I > > believe. > > > > not sure that needs to be tested over and over though...) This particular > > one > > > > does not look like one of those. testEventAndPromiseOrdering probably needs > > some > > > > care so that it will notice when the bug it references is fixed. > > > > > > Even the order mentioned in that function isn't correct right now. Updated > > comment to match that and removed event capturing from all other functions as > > they are not really checking for event order. > > > > unreached_func probably isn't the right thing to do there though? (I.e it looks > > weird and gives the wrong impression on what will/is expected to happen.) > > You are right, but i couldn't find a way to mix eventwatcher and promise to assert the event ordering. Or should i assert the event ordering with a flag for now? (updated the comment with more information). Maybe something fairly simple like: var numVolumeChangeEvents = 0; audio.onvolumechange = t.step_func(function() { ++numVolumeChangeEvents }); audio.play().then(..., t.step_func_done(e => { ... like now ... assert_equals(numVolumeChangeEvents, 0); })); (i.e open-coding playExpectingRejectedPromise. Optionally, an additional closure could be passed I guess.)
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2155053002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: audio.onloadedmetadata = t.step_func(function() {}); On 2016/07/19 08:30:11, fs wrote: > On 2016/07/19 at 07:15:44, Srirama wrote: > > On 2016/07/18 14:14:43, fs wrote: > > > On 2016/07/18 at 13:58:10, Srirama wrote: > > > > On 2016/07/18 10:51:02, fs wrote: > > > > > On 2016/07/18 at 09:36:10, Srirama wrote: > > > > > > Should we remove these events? Or should we keep them and test with > the > > > help > > > > > of eventwatcher? > > > > > > > > > > I think the interesting cases will be where events should be ordered wrt > the > > > > > play() promise (generally 'playing' before the promise is resolved I > > > believe. > > > > > not sure that needs to be tested over and over though...) This > particular > > > one > > > > > does not look like one of those. testEventAndPromiseOrdering probably > needs > > > some > > > > > care so that it will notice when the bug it references is fixed. > > > > > > > > Even the order mentioned in that function isn't correct right now. Updated > > > comment to match that and removed event capturing from all other functions > as > > > they are not really checking for event order. > > > > > > unreached_func probably isn't the right thing to do there though? (I.e it > looks > > > weird and gives the wrong impression on what will/is expected to happen.) > > > > You are right, but i couldn't find a way to mix eventwatcher and promise to > assert the event ordering. Or should i assert the event ordering with a flag for > now? (updated the comment with more information). > > Maybe something fairly simple like: > > var numVolumeChangeEvents = 0; > audio.onvolumechange = t.step_func(function() { ++numVolumeChangeEvents }); > audio.play().then(..., t.step_func_done(e => { > ... like now ... > assert_equals(numVolumeChangeEvents, 0); > })); > > (i.e open-coding playExpectingRejectedPromise. Optionally, an additional closure > could be passed I guess.) Done.
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/2155053002/#ps120001 (title: "add suggested change")
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 media-play-promise.html to testharness.js Cleaning up media-play-promise.html in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert media-play-promise.html to testharness.js Cleaning up media-play-promise.html in media/ 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 media-play-promise.html to testharness.js Cleaning up media-play-promise.html in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert media-play-promise.html to testharness.js Cleaning up media-play-promise.html in media/ 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/b87e3bb23b068f510fcf6bd18635a7a508e5d0d2 Cr-Commit-Position: refs/heads/master@{#406243} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b87e3bb23b068f510fcf6bd18635a7a508e5d0d2 Cr-Commit-Position: refs/heads/master@{#406243} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
