Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702753002:
Add video metrics integration test (Closed)
|
Created:
3 years, 10 months ago by Robert Ogden Modified:
3 years, 10 months ago CC:
chromium-reviews, posciak+watch_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, Tom Bergan, bolian Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd video metrics integration test
BUG=680579
Review-Url: https://codereview.chromium.org/2702753002
Cr-Commit-Position: refs/heads/master@{#451775}
Committed: https://chromium.googlesource.com/chromium/src/+/0dcca663b5138b8d79faf2702c3f9b5846938586
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add delta on assertion #
Total comments: 4
Patch Set 3 : 1 ms delta instead #Patch Set 4 : Use a shorter video #Messages
Total messages: 24 (9 generated)
robertogden@chromium.org changed reviewers: + ryansturm@chromium.org
tombergan@chromium.org changed reviewers: + tombergan@chromium.org
lgtm https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:49: 'duration': 60.127, I haven't done the math, but at first glance, this number looks like it might not be precisely representable in 64-bit floating point. That means this comparison is subject to imprecision. Suggest using an approximate comparison of +/- 0.25s ?
https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:49: 'duration': 60.127, On 2017/02/17 18:20:48, Tom Bergan wrote: > I haven't done the math, but at first glance, this number looks like it might > not be precisely representable in 64-bit floating point. That means this > comparison is subject to imprecision. Suggest using an approximate comparison of > +/- 0.25s ? Done.
https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:49: 'duration': 60.127, On 2017/02/17 18:49:28, Robert Ogden wrote: > On 2017/02/17 18:20:48, Tom Bergan wrote: > > I haven't done the math, but at first glance, this number looks like it might > > not be precisely representable in 64-bit floating point. That means this > > comparison is subject to imprecision. Suggest using an approximate comparison > of > > +/- 0.25s ? > > Done. As a nit on a nit, delta=0.25 is way, way bigger than any float round-off we'd get here. I would take .25 off as a noticeable issue that could rear it's head as the audio being out of sync by .25 seconds. Can you just make it a millisecond. https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/video.py:66: time.sleep(80) What is the impact of having a test that takes 80 seconds to run (impact on the continuous test harness)? I know if you add this test, I will end up never running all the integration tests because this one takes too long to run. I have no suggestions on how to fix it if the test truly needs to watch the whole video.
https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:49: 'duration': 60.127, On 2017/02/17 19:24:10, Ryan Sturm wrote: > On 2017/02/17 18:49:28, Robert Ogden wrote: > > On 2017/02/17 18:20:48, Tom Bergan wrote: > > > I haven't done the math, but at first glance, this number looks like it > might > > > not be precisely representable in 64-bit floating point. That means this > > > comparison is subject to imprecision. Suggest using an approximate > comparison > > of > > > +/- 0.25s ? > > > > Done. > > As a nit on a nit, delta=0.25 is way, way bigger than any float round-off we'd > get here. I would take .25 off as a noticeable issue that could rear it's head > as the audio being out of sync by .25 seconds. Can you just make it a > millisecond. Done. https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/video.py:66: time.sleep(80) On 2017/02/17 19:24:10, Ryan Sturm wrote: > What is the impact of having a test that takes 80 seconds to run (impact on the > continuous test harness)? I know if you add this test, I will end up never > running all the integration tests because this one takes too long to run. I have > no suggestions on how to fix it if the test truly needs to watch the whole > video. The test harness itself it happy to wait, but yes it'll make the whole test suite a rather long event. This test has to play all the way through because I don't have another way to ensure chrome decodes all the frames. Besides, so far all the tests in this class save the XHR one wait through the entire video :/
lgtm https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/video.py:66: time.sleep(80) On 2017/02/17 19:28:18, Robert Ogden wrote: > On 2017/02/17 19:24:10, Ryan Sturm wrote: > > What is the impact of having a test that takes 80 seconds to run (impact on > the > > continuous test harness)? I know if you add this test, I will end up never > > running all the integration tests because this one takes too long to run. I > have > > no suggestions on how to fix it if the test truly needs to watch the whole > > video. > > The test harness itself it happy to wait, but yes it'll make the whole test > suite a rather long event. This test has to play all the way through because I > don't have another way to ensure chrome decodes all the frames. Besides, so far > all the tests in this class save the XHR one wait through the entire video :/ We should be able to use a shorter video. No reason the video needs to be 60s. +bolian +harringtond
lgtm
On 2017/02/17 20:10:56, Tom Bergan wrote: > lgtm > > https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/video.py (right): > > https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/video.py:66: time.sleep(80) > On 2017/02/17 19:28:18, Robert Ogden wrote: > > On 2017/02/17 19:24:10, Ryan Sturm wrote: > > > What is the impact of having a test that takes 80 seconds to run (impact on > > the > > > continuous test harness)? I know if you add this test, I will end up never > > > running all the integration tests because this one takes too long to run. I > > have > > > no suggestions on how to fix it if the test truly needs to watch the whole > > > video. > > > > The test harness itself it happy to wait, but yes it'll make the whole test > > suite a rather long event. This test has to play all the way through because I > > don't have another way to ensure chrome decodes all the frames. Besides, so > far > > all the tests in this class save the XHR one wait through the entire video :/ > > We should be able to use a shorter video. No reason the video needs to be 60s. > > +bolian > +harringtond yep, a smaller video would work just as well. On the other hand, we aren't probably changing code that will break this test. Is 60s a barrier to running it weekly?
bolian@chromium.org changed reviewers: + bolian@chromium.org
https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/video.py:66: time.sleep(80) I am not sure how the test works internally. At least Chrome seems to know all the duration, width, height, etc as long as the video starts playing. So, do you need to wait for it to finish playing? If that is not possible, adding a much shorter video sounds good to me.
On 2017/02/17 20:21:57, bolian wrote: > https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/video.py (right): > > https://codereview.chromium.org/2702753002/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/video.py:66: time.sleep(80) > I am not sure how the test works internally. At least Chrome seems to know all > the duration, width, height, etc as long as the video starts playing. So, do you > need to wait for it to finish playing? > > If that is not possible, adding a much shorter video sounds good to me. Shorter video it is. Done.
The CQ bit was checked by robertogden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, tombergan@chromium.org Link to the patchset: https://codereview.chromium.org/2702753002/#ps50001 (title: "Use a shorter video")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by robertogden@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 50001, "attempt_start_ts": 1487692357735190,
"parent_rev": "27f8b7ea6bbee0b512ce820842ebe8bba019b050", "commit_rev":
"0dcca663b5138b8d79faf2702c3f9b5846938586"}
Message was sent while issue was closed.
Description was changed from ========== Add video metrics integration test BUG=680579 ========== to ========== Add video metrics integration test BUG=680579 Review-Url: https://codereview.chromium.org/2702753002 Cr-Commit-Position: refs/heads/master@{#451775} Committed: https://chromium.googlesource.com/chromium/src/+/0dcca663b5138b8d79faf2702c3f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/0dcca663b5138b8d79faf2702c3f... |
