Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2741263002:
Added Data Saver ChromeDriver test for via header fallback logic. (Closed)
DescriptionAdded Data Saver ChromeDriver test for via header fallback logic.
This test attempts to load a page through a test proxy that doesn't set
the via header properly on a non-4xx response, and verifies that Chrome
falls back to the next proxy and retries the request.
BUG=680562
Review-Url: https://codereview.chromium.org/2741263002
Cr-Commit-Position: refs/heads/master@{#456806}
Committed: https://chromium.googlesource.com/chromium/src/+/dda4c0c9ee86a2764173799140744e205d2ac0f2
Patch Set 1 : fixed comments #
Total comments: 4
Patch Set 2 : Remove 4xx test, and add remote port check #
Total comments: 2
Patch Set 3 : moved MissingViaHeaderOther fallback test into fallback.py #Messages
Total messages: 23 (13 generated)
sclittle@chromium.org changed reviewers: + megjablon@chromium.org, robertogden@chromium.org
Description was changed from ========== Added Data Saver ChromeDriver test for via header fallback logic. This test attempts to load a page through a test proxy that doesn't set the via header properly, and verifies that Chrome falls back to the next proxy, which does set the proper via header. BUG=680562 ========== to ========== Added Data Saver ChromeDriver tests for via header fallback logic. These tests attempt to load a page through a test proxy that doesn't set the via header properly, and verifies that Chrome either bypasses all proxies and retries the request (if the response code is a 4xx error) or falls back to the next proxy and retries the request (for non-4xx errors). BUG=680562 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
https://codereview.chromium.org/2741263002/diff/60001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/bypass.py (right): https://codereview.chromium.org/2741263002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/bypass.py:108: def testMissingViaHeader4xxBypass(self): Isn't this the ChromeProxyHTTPToDirectFallback telemetry test? Is there another bug for this? Or at least the bug linked looks like it's just for a fallback test. https://codereview.chromium.org/2741263002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/bypass.py:140: def testMissingViaHeaderNon4xxFallback(self): The telemetry tests check the remote port, should we be doing that somewhere in here?
https://codereview.chromium.org/2741263002/diff/60001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/bypass.py (right): https://codereview.chromium.org/2741263002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/bypass.py:108: def testMissingViaHeader4xxBypass(self): On 2017/03/11 00:51:47, megjablon wrote: > Isn't this the ChromeProxyHTTPToDirectFallback telemetry test? Is there another > bug for this? Or at least the bug linked looks like it's just for a fallback > test. The non-4xx test below is the counterpart for the ChromeProxyHTTPFallbackViaHeader test, I think that's the one linked to by the bug: https://cs.chromium.org/chromium/src/tools/chrome_proxy/integration_tests/chr... Good point though, I don't think the 4xx via header bypass behavior had a telemetry counterpart, so the linked bug isn't really related to it. I'll create a separate bug and CL for the 4xx test, and have this CL just include the test for the non-4xx fallback behavior. https://codereview.chromium.org/2741263002/diff/60001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/bypass.py:140: def testMissingViaHeaderNon4xxFallback(self): On 2017/03/11 00:51:47, megjablon wrote: > The telemetry tests check the remote port, should we be doing that somewhere in > here? Done.
Description was changed from ========== Added Data Saver ChromeDriver tests for via header fallback logic. These tests attempt to load a page through a test proxy that doesn't set the via header properly, and verifies that Chrome either bypasses all proxies and retries the request (if the response code is a 4xx error) or falls back to the next proxy and retries the request (for non-4xx errors). BUG=680562 ========== to ========== Added Data Saver ChromeDriver test for via header fallback logic. These tests attempt to load a page through a test proxy that doesn't set the via header properly on a non-4xx response, and verifies that Chrome falls back to the next proxy and retries the request. BUG=680562 ==========
Description was changed from ========== Added Data Saver ChromeDriver test for via header fallback logic. These tests attempt to load a page through a test proxy that doesn't set the via header properly on a non-4xx response, and verifies that Chrome falls back to the next proxy and retries the request. BUG=680562 ========== to ========== Added Data Saver ChromeDriver test for via header fallback logic. This test attempts to load a page through a test proxy that doesn't set the via header properly on a non-4xx response, and verifies that Chrome falls back to the next proxy and retries the request. BUG=680562 ==========
lgtm
The CQ bit was checked by sclittle@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2741263002/diff/40002/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/bypass.py (right): https://codereview.chromium.org/2741263002/diff/40002/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/bypass.py:108: def testMissingViaHeaderNon4xxFallback(self): This belongs in fallback.py.
The CQ bit was unchecked by megjablon@chromium.org
https://codereview.chromium.org/2741263002/diff/40002/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/bypass.py (right): https://codereview.chromium.org/2741263002/diff/40002/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/bypass.py:108: def testMissingViaHeaderNon4xxFallback(self): On 2017/03/14 18:17:03, Robert Ogden wrote: > This belongs in fallback.py. Done.
lgtm
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2741263002/#ps90001 (title: "moved MissingViaHeaderOther fallback test into fallback.py")
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": 90001, "attempt_start_ts": 1489519600470910,
"parent_rev": "5ddc2d6d65793379648307792c763f92cfe92574", "commit_rev":
"dda4c0c9ee86a2764173799140744e205d2ac0f2"}
Message was sent while issue was closed.
Description was changed from ========== Added Data Saver ChromeDriver test for via header fallback logic. This test attempts to load a page through a test proxy that doesn't set the via header properly on a non-4xx response, and verifies that Chrome falls back to the next proxy and retries the request. BUG=680562 ========== to ========== Added Data Saver ChromeDriver test for via header fallback logic. This test attempts to load a page through a test proxy that doesn't set the via header properly on a non-4xx response, and verifies that Chrome falls back to the next proxy and retries the request. BUG=680562 Review-Url: https://codereview.chromium.org/2741263002 Cr-Commit-Position: refs/heads/master@{#456806} Committed: https://chromium.googlesource.com/chromium/src/+/dda4c0c9ee86a276417379914074... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:90001) as https://chromium.googlesource.com/chromium/src/+/dda4c0c9ee86a276417379914074... |
