Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793673002:
Add Data Saver ChromeDriver tests for long bypasses. (Closed)
DescriptionAdd Data Saver ChromeDriver tests for long bypasses.
This CL adds two tests, one that triggers a 20 second bypass and
verifies that the Data Reduction Proxy is used after the 20 seconds are
up, and a test that triggers a 1-5 minute bypass and verifies that the
Data Reduction Proxy is used after 5 minutes.
BUG=680563, 680561
Review-Url: https://codereview.chromium.org/2793673002
Cr-Commit-Position: refs/heads/master@{#462170}
Committed: https://chromium.googlesource.com/chromium/src/+/878ac4d2acc8771023bbfb167cc071a5f1455470
Patch Set 1 #
Total comments: 12
Patch Set 2 : added class comment for ReenableAfterBypass. #Messages
Total messages: 18 (9 generated)
sclittle@chromium.org changed reviewers: + robertogden@chromium.org, ryansturm@chromium.org
https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/reenable_after_bypass.py (right): https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:12: class ReenableAfterBypass(IntegrationTest): Go ahead and just stick these in the bypass.py file https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:37: time.sleep(20) Any risk on flakiness here? What about 21 seconds https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:68: time.sleep(60 * 4 + 30) Same flakiness concern
https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/reenable_after_bypass.py (right): https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:12: class ReenableAfterBypass(IntegrationTest): On 2017/04/04 19:42:59, Robert Ogden wrote: > Go ahead and just stick these in the bypass.py file Are you sure? These tests take a really really long time to run. Keeping them in a different file makes it easier to run all the tests except for these ones, or at least to run these super long tests. I'm worried that putting this with the other bypass tests in bypass.py will discourage people from running these tests e.g. before landing a CL, especially since these reenable-after-bypass tests aren't likely to be relevant to most CLs. https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:37: time.sleep(20) On 2017/04/04 19:42:59, Robert Ogden wrote: > Any risk on flakiness here? What about 21 seconds I was banking on the fact that test_driver.LoadURL() is synchronous, so the 20 second bypass should have started strictly earlier than this time.sleep(20) starts. I can change it to 21 seconds or more if you'd like though. https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:68: time.sleep(60 * 4 + 30) On 2017/04/04 19:42:59, Robert Ogden wrote: > Same flakiness concern Similar response as above :)
lgtm https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/reenable_after_bypass.py (right): https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:12: class ReenableAfterBypass(IntegrationTest): On 2017/04/04 20:41:24, sclittle wrote: > On 2017/04/04 19:42:59, Robert Ogden wrote: > > Go ahead and just stick these in the bypass.py file > > Are you sure? These tests take a really really long time to run. Keeping them in > a different file makes it easier to run all the tests except for these ones, or > at least to run these super long tests. > > I'm worried that putting this with the other bypass tests in bypass.py will > discourage people from running these tests e.g. before landing a CL, especially > since these reenable-after-bypass tests aren't likely to be relevant to most > CLs. Ah, that's right, we talked about this. What you have is good https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:37: time.sleep(20) On 2017/04/04 20:41:24, sclittle wrote: > On 2017/04/04 19:42:59, Robert Ogden wrote: > > Any risk on flakiness here? What about 21 seconds > > I was banking on the fact that test_driver.LoadURL() is synchronous, so the 20 > second bypass should have started strictly earlier than this time.sleep(20) > starts. > > I can change it to 21 seconds or more if you'd like though. Cool, sgtm
The CQ bit was checked by sclittle@chromium.org
The CQ bit was unchecked by sclittle@chromium.org
The CQ bit was checked by sclittle@chromium.org
lgtm % add/maybe fix comment https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/reenable_after_bypass.py (right): https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:12: class ReenableAfterBypass(IntegrationTest): On 2017/04/04 20:44:25, Robert Ogden wrote: > On 2017/04/04 20:41:24, sclittle wrote: > > On 2017/04/04 19:42:59, Robert Ogden wrote: > > > Go ahead and just stick these in the bypass.py file > > > > Are you sure? These tests take a really really long time to run. Keeping them > in > > a different file makes it easier to run all the tests except for these ones, > or > > at least to run these super long tests. > > > > I'm worried that putting this with the other bypass tests in bypass.py will > > discourage people from running these tests e.g. before landing a CL, > especially > > since these reenable-after-bypass tests aren't likely to be relevant to most > > CLs. > > Ah, that's right, we talked about this. What you have is good Can you add a comment about this issue, so they are added to the waterfall correctly. https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:44: # Verify that when the Data Reduction Proxy responds with the "block=0" I thought "block=0" was block once. What is the directive for block once?
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 sclittle@chromium.org
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertogden@chromium.org, ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2793673002/#ps20001 (title: "added class comment for ReenableAfterBypass.")
https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/reenable_after_bypass.py (right): https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:12: class ReenableAfterBypass(IntegrationTest): On 2017/04/05 18:42:09, Ryan Sturm wrote: > On 2017/04/04 20:44:25, Robert Ogden wrote: > > On 2017/04/04 20:41:24, sclittle wrote: > > > On 2017/04/04 19:42:59, Robert Ogden wrote: > > > > Go ahead and just stick these in the bypass.py file > > > > > > Are you sure? These tests take a really really long time to run. Keeping > them > > in > > > a different file makes it easier to run all the tests except for these ones, > > or > > > at least to run these super long tests. > > > > > > I'm worried that putting this with the other bypass tests in bypass.py will > > > discourage people from running these tests e.g. before landing a CL, > > especially > > > since these reenable-after-bypass tests aren't likely to be relevant to most > > > CLs. > > > > Ah, that's right, we talked about this. What you have is good > > Can you add a comment about this issue, so they are added to the waterfall > correctly. Done. https://codereview.chromium.org/2793673002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/reenable_after_bypass.py:44: # Verify that when the Data Reduction Proxy responds with the "block=0" On 2017/04/05 18:42:09, Ryan Sturm wrote: > I thought "block=0" was block once. What is the directive for block once? "block=0" bypasses the proxies for a random duration between 1 and 5 minutes. The directive for block once is "block-once" :)
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": 20001, "attempt_start_ts": 1491418741069910,
"parent_rev": "b594d33b1b5a11701884592ba7c82babe0119516", "commit_rev":
"878ac4d2acc8771023bbfb167cc071a5f1455470"}
Message was sent while issue was closed.
Description was changed from ========== Add Data Saver ChromeDriver tests for long bypasses. This CL adds two tests, one that triggers a 20 second bypass and verifies that the Data Reduction Proxy is used after the 20 seconds are up, and a test that triggers a 1-5 minute bypass and verifies that the Data Reduction Proxy is used after 5 minutes. BUG=680563, 680561 ========== to ========== Add Data Saver ChromeDriver tests for long bypasses. This CL adds two tests, one that triggers a 20 second bypass and verifies that the Data Reduction Proxy is used after the 20 seconds are up, and a test that triggers a 1-5 minute bypass and verifies that the Data Reduction Proxy is used after 5 minutes. BUG=680563, 680561 Review-Url: https://codereview.chromium.org/2793673002 Cr-Commit-Position: refs/heads/master@{#462170} Committed: https://chromium.googlesource.com/chromium/src/+/878ac4d2acc8771023bbfb167cc0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/878ac4d2acc8771023bbfb167cc0... |
