Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1856383002:
Convert css cue track tests from video-test.js to testharness.js based (Closed)
|
Created:
4 years, 8 months ago by Srirama Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert css cue track tests from video-test.js to testharness.js based
Cleaning up css cue tests in media/track 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/d7207b6cd046d757ebd38ef490d51d355dde2dde
Cr-Commit-Position: refs/heads/master@{#385779}
Patch Set 1 : #
Total comments: 19
Patch Set 2 : Address review comments #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== fix BUG= ========== to ========== Convert track tests from video-test.js to testharness.js based Cleaning up tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
Description was changed from ========== Convert track tests from video-test.js to testharness.js based Cleaning up tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert track tests from video-test.js to testharness.js based Cleaning up some tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
Patchset #1 (id:1) has been deleted
srirama.m@samsung.com changed reviewers: + mlamouri@chromium.org, philipj@opera.com
PTAL
Description was changed from ========== Convert track tests from video-test.js to testharness.js based Cleaning up some tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert css cue track tests from video-test.js to testharness.js based Cleaning up css cue tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (left): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:4: <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> Why are you removing all that? Having <html>, <head> and <body> wouldn't hurt. Feel free to keep the <script> after body if you only want to have the script run after all the DOM's <body> is constructed. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:21: video.onseeked = t.step_func_done(function(t) { I would prefer step_func() and call t.done() inside because it is clearer. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:32: }); The test name goes here, not in <title>. ie. "Test that the cue is not styled when video is in a shadow tree and style is in a document."
https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (left): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:4: <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> On 2016/04/06 09:22:58, Mounir Lamouri wrote: > Why are you removing all that? Having <html>, <head> and <body> wouldn't hurt. > Feel free to keep the <script> after body if you only want to have the script > run after all the DOM's <body> is constructed. Even this was suggested by philipj and as per coding guidelines these tags can be omitted (https://www.chromium.org/blink/coding-style/layout-test-style-guidelines), so i am following this style, though keeping them also won't harm as you said. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:21: video.onseeked = t.step_func_done(function(t) { On 2016/04/06 09:22:58, Mounir Lamouri wrote: > I would prefer step_func() and call t.done() inside because it is clearer. Hmm, philip suggested me to use step_func_done and remove t.done(), i think his intention is to keep the test code as compact and simple as possible. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:32: }); On 2016/04/06 09:22:58, Mounir Lamouri wrote: > The test name goes here, not in <title>. > ie. "Test that the cue is not styled when video is in a shadow tree and style is > in a document." Both are same, and having title instead of description would make the test look more simple(even philipj suggested same thing). So what do you suggest?
https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:21: video.onseeked = t.step_func_done(function(t) { On 2016/04/06 10:21:23, Srirama wrote: > On 2016/04/06 09:22:58, Mounir Lamouri wrote: > > I would prefer step_func() and call t.done() inside because it is clearer. > > Hmm, philip suggested me to use step_func_done and remove t.done(), i think his > intention is to keep the test code as compact and simple as possible. Yep, but I'm probably unusually devoted to minimal tests, not surprised to see some disagreement. I'm pretty used to seeing step_func_done by now, but don't feel strongly about it. Mounir? https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:32: }); On 2016/04/06 10:21:24, Srirama wrote: > On 2016/04/06 09:22:58, Mounir Lamouri wrote: > > The test name goes here, not in <title>. > > ie. "Test that the cue is not styled when video is in a shadow tree and style > is > > in a document." > > Both are same, and having title instead of description would make the test look > more simple(even philipj suggested same thing). So what do you suggest? Right, when there's just a single test per file, I really like using <title>, because it's the first thing you see when reading the test. This style is very common in upstream web-platform-tests.
LGTM % nits, but please wait for Mounir's second pass. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2.html:20: assert_equals(getComputedStyle(cueNode).color, "rgb(255, 0, 0)"); Same comments apply to this test. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:23: assert_not_equals(getComputedStyle(cueNode).color, "rgb(255, 0, 0)"); Hmm, the test was already like this, but this looks a bit fragile, the actual color could change without the test failing. If the actual value is reliable, can you use that instead? https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:29: video.oncanplaythrough = t.step_func(function(t) { I think you can not wait for canplaythrough and just set currentTime directly. Originally this didn't work, but it should now, since a few years at least.
Patchset #2 (id:40001) has been deleted
lgtm https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:21: video.onseeked = t.step_func_done(function(t) { On 2016/04/06 at 12:12:55, philipj wrote: > On 2016/04/06 10:21:23, Srirama wrote: > > On 2016/04/06 09:22:58, Mounir Lamouri wrote: > > > I would prefer step_func() and call t.done() inside because it is clearer. > > > > Hmm, philip suggested me to use step_func_done and remove t.done(), i think his > > intention is to keep the test code as compact and simple as possible. > > Yep, but I'm probably unusually devoted to minimal tests, not surprised to see some disagreement. I'm pretty used to seeing step_func_done by now, but don't feel strongly about it. Mounir? Sure. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:32: }); On 2016/04/06 at 12:12:55, philipj wrote: > On 2016/04/06 10:21:24, Srirama wrote: > > On 2016/04/06 09:22:58, Mounir Lamouri wrote: > > > The test name goes here, not in <title>. > > > ie. "Test that the cue is not styled when video is in a shadow tree and style > > is > > > in a document." > > > > Both are same, and having title instead of description would make the test look > > more simple(even philipj suggested same thing). So what do you suggest? > > Right, when there's just a single test per file, I really like using <title>, because it's the first thing you see when reading the test. This style is very common in upstream web-platform-tests. What happens if the test fails? ie. what is printed?
https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:32: }); On 2016/04/07 13:37:17, Mounir Lamouri wrote: > On 2016/04/06 at 12:12:55, philipj wrote: > > On 2016/04/06 10:21:24, Srirama wrote: > > > On 2016/04/06 09:22:58, Mounir Lamouri wrote: > > > > The test name goes here, not in <title>. > > > > ie. "Test that the cue is not styled when video is in a shadow tree and > style > > > is > > > > in a document." > > > > > > Both are same, and having title instead of description would make the test > look > > > more simple(even philipj suggested same thing). So what do you suggest? > > > > Right, when there's just a single test per file, I really like using <title>, > because it's the first thing you see when reading the test. This style is very > common in upstream web-platform-tests. > > What happens if the test fails? ie. what is printed? It's the same as if the title is given as an argument to async_test. The title isn't changed by the test failing, just the things that are printed to the right of the title. Results or whatever it may be labeled.
https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2.html:20: assert_equals(getComputedStyle(cueNode).color, "rgb(255, 0, 0)"); On 2016/04/06 12:17:46, philipj wrote: > Same comments apply to this test. Aren't we already checking the actual value here? Here it is "assert_equals". https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:23: assert_not_equals(getComputedStyle(cueNode).color, "rgb(255, 0, 0)"); On 2016/04/06 12:17:46, philipj wrote: > Hmm, the test was already like this, but this looks a bit fragile, the actual > color could change without the test failing. If the actual value is reliable, > can you use that instead? Done. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:29: video.oncanplaythrough = t.step_func(function(t) { On 2016/04/06 12:17:46, philipj wrote: > I think you can not wait for canplaythrough and just set currentTime directly. > Originally this didn't work, but it should now, since a few years at least. Done. https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow.html:32: }); On 2016/04/07 13:40:32, philipj wrote: > On 2016/04/07 13:37:17, Mounir Lamouri wrote: > > On 2016/04/06 at 12:12:55, philipj wrote: > > > On 2016/04/06 10:21:24, Srirama wrote: > > > > On 2016/04/06 09:22:58, Mounir Lamouri wrote: > > > > > The test name goes here, not in <title>. > > > > > ie. "Test that the cue is not styled when video is in a shadow tree and > > style > > > > is > > > > > in a document." > > > > > > > > Both are same, and having title instead of description would make the test > > look > > > > more simple(even philipj suggested same thing). So what do you suggest? > > > > > > Right, when there's just a single test per file, I really like using > <title>, > > because it's the first thing you see when reading the test. This style is very > > common in upstream web-platform-tests. > > > > What happens if the test fails? ie. what is printed? > > It's the same as if the title is given as an argument to async_test. The title > isn't changed by the test failing, just the things that are printed to the right > of the title. Results or whatever it may be labeled. yes, this is the sample output for your reference. This is a testharness.js-based test. FAIL Test that the cue is not styled when video is in a shadow tree and style is in a document. assert_equals: expected "rgb(255, 255, 0)" but got "rgb(255, 255, 255)" Harness: the test ran to completion.
https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2.html (right): https://codereview.chromium.org/1856383002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2.html:20: assert_equals(getComputedStyle(cueNode).color, "rgb(255, 0, 0)"); On 2016/04/07 13:46:18, Srirama wrote: > On 2016/04/06 12:17:46, philipj wrote: > > Same comments apply to this test. > > Aren't we already checking the actual value here? Here it is "assert_equals". Oops :)
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1856383002/#ps60001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856383002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856383002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856383002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856383002/60001
Message was sent while issue was closed.
Description was changed from ========== Convert css cue track tests from video-test.js to testharness.js based Cleaning up css cue tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert css cue track tests from video-test.js to testharness.js based Cleaning up css cue tests in media/track 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/d7207b6cd046d757ebd38ef490d51d355dde2dde Cr-Commit-Position: refs/heads/master@{#385779} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d7207b6cd046d757ebd38ef490d51d355dde2dde Cr-Commit-Position: refs/heads/master@{#385779}
Message was sent while issue was closed.
Failed to apply patch for third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt: While running git rm third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt; fatal: pathspec 'third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt' did not match any files Patch: D third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt Index: third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt diff --git a/third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt b/third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt deleted file mode 100644 index b0b0f9c10345c5312935b24a4967615f246ac264..0000000000000000000000000000000000000000 --- a/third_party/WebKit/LayoutTests/media/track/css-cue-for-video-in-shadow-2-expected.txt +++ /dev/null @@ -1,8 +0,0 @@ -Test that the cue is styled when video and style is in the same shadow tree. -EVENT(canplaythrough) -EVENT(seeked) -EXPECTED (getComputedStyle(cueNode).color == 'rgb(255, 0, 0)') OK -EXPECTED (getComputedStyle(cueNode).color == 'rgb(0, 128, 0)') OK -EXPECTED (getComputedStyle(cueNode).color == 'rgb(255, 0, 0)') OK -END OF TEST - |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
