Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2126753002:
Convert video-[default, size, timeupdate]* tests 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, 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-[default, size, timeupdate]* tests to testharness.js
Cleaning up video-[default, size, timeupdate]* tests
in media/ to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.
BUG=588956, 310481
Committed: https://crrev.com/cc0fab89b1307fc83165dde3f7126a33708f85b7
Cr-Commit-Position: refs/heads/master@{#404214}
Patch Set 1 #
Total comments: 13
Patch Set 2 : address comments #
Total comments: 10
Patch Set 3 : address comments #
Total comments: 4
Messages
Total messages: 29 (10 generated)
Description was changed from ========== fix BUG= ========== to ========== Convert video-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests 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/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-defaultmuted.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:17: // After setting url, content attribute should have set IDL attribute. This is actually wrong per spec, can you link to https://bugs.chromium.org/p/chromium/issues/detail?id=350303 here, so that if this test starts failing the author will know the test is wrong? A proper test, which would currently not pass, would do something like this: <video muted> <script> test(function() { assert_true(document.querySelector('video').muted); }); </script> </video> No need to add that to this test now, thought. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:40: assert_equals(video.muted, expectedMuted); Can you inline these two asserts everywhere instead? I think it'll make it easier to see what the test is asserting. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:48: test_muted_content_attribute(true); I think the structure here is not as clear as it could be. A single synchronous test could check that defaultMuted IDL attribute reflects the muted content attribute and that neither affects the muted IDL attribute. Can you do that, then have the async test just to test the m_muted = true code path in HTMLMediaElement::loadResource? https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-size.html:2: <title>Test "video" element size with and without "src" and "poster" attributes.</title> This test is flaky, see https://bugs.chromium.org/p/chromium/issues/detail?id=310481 which I have assigned to you. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-size.html:78: setTimeout(t.step_func_done(function() { This is why it is flaky. As I said on the bug, "The problem is that there's no event for when the poster is loaded." What might work is if you first create an img element with the movie.poster (if not null) and wait for the load event on the img element before doing a setTimeout(..., 0) and running the tests. Run the test 1000 times or so to make sure it's really stable. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:13: video.onerror = t.step_func(function() {}); What's the purpose of all of these? Previously the event order was tested implicitly by the -expected.txt order, but this doesn't really do anything? LayoutTests/media/autoplay.html is one way of testing the event order.
https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:13: video.onerror = t.step_func(function() {}); On 2016/07/06 at 09:22:22, Philip Jägenstedt wrote: > What's the purpose of all of these? Previously the event order was tested implicitly by the -expected.txt order, but this doesn't really do anything? LayoutTests/media/autoplay.html is one way of testing the event order. EventWatcher could be another way. I think I may have been the one leading astray here, seeing these as something that would be good to test (i.e the order and appearance), and this is some form of stop-gap so that it would be easier to revisit and fix that up later.
On 2016/07/06 09:34:47, fs wrote: > https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html > (right): > > https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:13: > video.onerror = t.step_func(function() {}); > On 2016/07/06 at 09:22:22, Philip Jägenstedt wrote: > > What's the purpose of all of these? Previously the event order was tested > implicitly by the -expected.txt order, but this doesn't really do anything? > LayoutTests/media/autoplay.html is one way of testing the event order. > > EventWatcher could be another way. I think I may have been the one leading > astray here, seeing these as something that would be good to test (i.e the order > and appearance), and this is some form of stop-gap so that it would be easier to > revisit and fix that up later. I have noticed during this "rewriting tests", many of the test cases have been capturing and testing the event order through *-expected.txt files. I just kept those events though not tested the event order (as fs mentioned above). So the question is do we really need all these test cases to check the event order? My point is we may end up rebasing many of these test cases every time there is a change in behavior which affects the event order.
On 2016/07/06 09:48:12, Srirama wrote: > On 2016/07/06 09:34:47, fs wrote: > > > https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... > > File > third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html > > (right): > > > > > https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:13: > > video.onerror = t.step_func(function() {}); > > On 2016/07/06 at 09:22:22, Philip Jägenstedt wrote: > > > What's the purpose of all of these? Previously the event order was tested > > implicitly by the -expected.txt order, but this doesn't really do anything? > > LayoutTests/media/autoplay.html is one way of testing the event order. > > > > EventWatcher could be another way. I think I may have been the one leading > > astray here, seeing these as something that would be good to test (i.e the > order > > and appearance), and this is some form of stop-gap so that it would be easier > to > > revisit and fix that up later. > > I have noticed during this "rewriting tests", many of the test cases have been > capturing and testing the event order through *-expected.txt files. I just kept > those events though not tested the event order (as fs mentioned above). So the > question is do we really need all these test cases to check the event order? My > point is we may end up rebasing many of these test cases every time there is a > change in behavior which affects the event order. We already have one test case "event-attributes.html" for event order testing and probably restrict this event ordering to few more test cases where it is necessary and remove everywhere else?
On 2016/07/06 09:51:54, Srirama wrote: > On 2016/07/06 09:48:12, Srirama wrote: > > On 2016/07/06 09:34:47, fs wrote: > > > > > > https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... > > > File > > third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... > > > > third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:13: > > > video.onerror = t.step_func(function() {}); > > > On 2016/07/06 at 09:22:22, Philip Jägenstedt wrote: > > > > What's the purpose of all of these? Previously the event order was tested > > > implicitly by the -expected.txt order, but this doesn't really do anything? > > > LayoutTests/media/autoplay.html is one way of testing the event order. > > > > > > EventWatcher could be another way. I think I may have been the one leading > > > astray here, seeing these as something that would be good to test (i.e the > > order > > > and appearance), and this is some form of stop-gap so that it would be > easier > > to > > > revisit and fix that up later. > > > > I have noticed during this "rewriting tests", many of the test cases have been > > capturing and testing the event order through *-expected.txt files. I just > kept > > those events though not tested the event order (as fs mentioned above). So the > > question is do we really need all these test cases to check the event order? > My > > point is we may end up rebasing many of these test cases every time there is a > > change in behavior which affects the event order. > > We already have one test case "event-attributes.html" for event order testing > and probably restrict this event ordering to few more test cases where it is > necessary and remove everywhere else? For this test, the description says that it's about testing when the timeupdate event is fired, so you could do that without testing all of the event order. In fact, eventCountOnPause isn't needed either, you could just add a t.unreached_func after the pause event. But you should expect a timeupdate event right before the pause event.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-defaultmuted.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:17: // After setting url, content attribute should have set IDL attribute. On 2016/07/06 09:22:22, Philip Jägenstedt wrote: > This is actually wrong per spec, can you link to > https://bugs.chromium.org/p/chromium/issues/detail?id=350303 here, so that if > this test starts failing the author will know the test is wrong? > > A proper test, which would currently not pass, would do something like this: > > <video muted> > <script> > test(function() { > assert_true(document.querySelector('video').muted); > }); > </script> > </video> > > No need to add that to this test now, thought. Done. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:40: assert_equals(video.muted, expectedMuted); On 2016/07/06 09:22:22, Philip Jägenstedt wrote: > Can you inline these two asserts everywhere instead? I think it'll make it > easier to see what the test is asserting. Done. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:48: test_muted_content_attribute(true); On 2016/07/06 09:22:22, Philip Jägenstedt wrote: > I think the structure here is not as clear as it could be. A single synchronous > test could check that defaultMuted IDL attribute reflects the muted content > attribute and that neither affects the muted IDL attribute. Can you do that, > then have the async test just to test the m_muted = true code path in > HTMLMediaElement::loadResource? Done. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-size.html:2: <title>Test "video" element size with and without "src" and "poster" attributes.</title> On 2016/07/06 09:22:22, Philip Jägenstedt wrote: > This test is flaky, see > https://bugs.chromium.org/p/chromium/issues/detail?id=310481 which I have > assigned to you. Acknowledged. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-size.html:78: setTimeout(t.step_func_done(function() { On 2016/07/06 09:22:22, Philip Jägenstedt wrote: > This is why it is flaky. As I said on the bug, "The problem is that there's no > event for when the poster is loaded." > > What might work is if you first create an img element with the movie.poster (if > not null) and wait for the load event on the img element before doing a > setTimeout(..., 0) and running the tests. Run the test 1000 times or so to make > sure it's really stable. Done. https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html (right): https://codereview.chromium.org/2126753002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:13: video.onerror = t.step_func(function() {}); On 2016/07/06 09:34:46, fs wrote: > On 2016/07/06 at 09:22:22, Philip Jägenstedt wrote: > > What's the purpose of all of these? Previously the event order was tested > implicitly by the -expected.txt order, but this doesn't really do anything? > LayoutTests/media/autoplay.html is one way of testing the event order. > > EventWatcher could be another way. I think I may have been the one leading > astray here, seeing these as something that would be good to test (i.e the order > and appearance), and this is some form of stop-gap so that it would be easier to > revisit and fix that up later. Removed all these as discussed.
https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-defaultmuted.html (right): https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:10: // that "muted" IDL attribute is not affected by "content" attribute. "content" makes it look like that's the attribute name, so just "the content attribute" would work. https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:60: var image = document.createElement("img"); A very clever implementation might be able to see that after the load event has been fired, this img element is unreachable and can be GCd. Can you append the image to the document, or does that interfere with the testing somehow? https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:64: setTimeout(runTest, 0); Need to wrap t.step_func(runTest), and since you load the image before any of the test is run, you don't need the setTimeout here. https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:85: testMovieSize(); function(){x()} => x here and below. https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html (right): https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:20: }, 500) ; This seems like the test is still slower than necessary. How about just calling play(), waiting for the playing event, then waiting for a timeupdate event and then calling pause()? A timeout after that is necessary, but 250ms seems sufficient since that's the lowest rate allowed by the spec.
https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-defaultmuted.html (right): https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-defaultmuted.html:10: // that "muted" IDL attribute is not affected by "content" attribute. On 2016/07/07 12:33:08, Philip Jägenstedt wrote: > "content" makes it look like that's the attribute name, so just "the content > attribute" would work. Done. https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:60: var image = document.createElement("img"); On 2016/07/07 12:33:08, Philip Jägenstedt wrote: > A very clever implementation might be able to see that after the load event has > been fired, this img element is unreachable and can be GCd. Can you append the > image to the document, or does that interfere with the testing somehow? Done, it is working https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:64: setTimeout(runTest, 0); On 2016/07/07 12:33:08, Philip Jägenstedt wrote: > Need to wrap t.step_func(runTest), and since you load the image before any of > the test is run, you don't need the setTimeout here. Done. https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:85: testMovieSize(); On 2016/07/07 12:33:08, Philip Jägenstedt wrote: > function(){x()} => x here and below. Done. https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html (right): https://codereview.chromium.org/2126753002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-timeupdate-during-playback.html:20: }, 500) ; On 2016/07/07 12:33:09, Philip Jägenstedt wrote: > This seems like the test is still slower than necessary. How about just calling > play(), waiting for the playing event, then waiting for a timeupdate event and > then calling pause()? A timeout after that is necessary, but 250ms seems > sufficient since that's the lowest rate allowed by the spec. Done.
LGTM if no flakiness https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:85: setTimeout(t.step_func_done(testMovieSize), 0); This is now assuming that the poster image will load on a single spin of the message loop. That's hopefully true, but have you run the test 1000 times to check for flakiness?
https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:85: setTimeout(t.step_func_done(testMovieSize), 0); On 2016/07/07 13:37:32, Philip Jägenstedt wrote: > This is now assuming that the poster image will load on a single spin of the > message loop. That's hopefully true, but have you run the test 1000 times to > check for flakiness? Working fine both in release and debug builds in my local linux system. Should we have some timeout value, to be more safe?
https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:85: setTimeout(t.step_func_done(testMovieSize), 0); On 2016/07/07 13:48:17, Srirama wrote: > On 2016/07/07 13:37:32, Philip Jägenstedt wrote: > > This is now assuming that the poster image will load on a single spin of the > > message loop. That's hopefully true, but have you run the test 1000 times to > > check for flakiness? > > Working fine both in release and debug builds in my local linux system. Should > we have some timeout value, to be more safe? No need I think until it's proven to be flaky. But can you add issue 310481 to the BUG= line and close that bug if there are no flakes on flakiness dashboard after a week or so?
Description was changed from ========== Convert video-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests 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 video-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956, 310481 ==========
https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-size.html (right): https://codereview.chromium.org/2126753002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-size.html:85: setTimeout(t.step_func_done(testMovieSize), 0); On 2016/07/07 13:55:40, Philip Jägenstedt wrote: > On 2016/07/07 13:48:17, Srirama wrote: > > On 2016/07/07 13:37:32, Philip Jägenstedt wrote: > > > This is now assuming that the poster image will load on a single spin of the > > > message loop. That's hopefully true, but have you run the test 1000 times to > > > check for flakiness? > > > > Working fine both in release and debug builds in my local linux system. Should > > we have some timeout value, to be more safe? > > No need I think until it's proven to be flaky. But can you add issue 310481 to > the BUG= line and close that bug if there are no flakes on flakiness dashboard > after a week or so? Acknowledged.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by srirama.m@samsung.com
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-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956, 310481 ========== to ========== Convert video-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956, 310481 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Convert video-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956, 310481 ========== to ========== Convert video-[default, size, timeupdate]* tests to testharness.js Cleaning up video-[default, size, timeupdate]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956, 310481 Committed: https://crrev.com/cc0fab89b1307fc83165dde3f7126a33708f85b7 Cr-Commit-Position: refs/heads/master@{#404214} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc0fab89b1307fc83165dde3f7126a33708f85b7 Cr-Commit-Position: refs/heads/master@{#404214} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
