Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2666813004:
Implement the Chrome Proxy bypass integration test with ChromeDriver (Closed)
DescriptionImplement the Chrome Proxy bypass integration test with ChromeDriver
BUG=680554
Review-Url: https://codereview.chromium.org/2666813004
Cr-Commit-Position: refs/heads/master@{#447379}
Committed: https://chromium.googlesource.com/chromium/src/+/63e8b962f4207e65dd6357a86089b0115d2c21f6
Patch Set 1 #
Total comments: 2
Patch Set 2 : test next page is bypassed #Messages
Total messages: 17 (9 generated)
megjablon@chromium.org changed reviewers: + robertogden@chromium.org, ryansturm@chromium.org
PTAL, thanks!
lgtm % really long suggestion. Feel free to ignore if you think the via header check is enough. https://codereview.chromium.org/2666813004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/bypass.py (right): https://codereview.chromium.org/2666813004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/bypass.py:18: for response in t.GetHTTPResponses(): tl;dr: Should we check if the CP header is present in request headers? Should we call LoadURL on a different check.googlezip.net page afterwards to verify that it does not use DRP? If I understand this test correctly (which I might not), when LoadURL is called, subsequent requests will not be attempted via DRP for 1-5 minutes (randomly in that interval), and this request will be retried without DRP. If that is the case, it might be worth also checking if checking if the CP header is also not in the request headers. Since the main frame request is retried with two different sets of headers, I am guessing that only the second set of headers is considered when fetching the headers. Since not having CP header implies not having via in the CP header, you could replace the assertNotHasChromeProxyViaHeader. The syntax would be something like: self.assert(response['request_headers']['chrome-proxy']) In the same vein, it might be worth adding a check to make sure we bypass other navigations that happen within a minute of this one, so maybe LoadURL("http://check.googlezip.net/test.html") and verify that the CP header is not present.
lgtm
https://codereview.chromium.org/2666813004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/bypass.py (right): https://codereview.chromium.org/2666813004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/bypass.py:18: for response in t.GetHTTPResponses(): On 2017/01/31 19:48:23, Ryan Sturm wrote: > tl;dr: Should we check if the CP header is present in request headers? Should we > call LoadURL on a different http://check.googlezip.net page afterwards to verify that > it does not use DRP? > > If I understand this test correctly (which I might not), when LoadURL is called, > subsequent requests will not be attempted via DRP for 1-5 minutes (randomly in > that interval), and this request will be retried without DRP. > > If that is the case, it might be worth also checking if checking if the CP > header is also not in the request headers. > > Since the main frame request is retried with two different sets of headers, I am > guessing that only the second set of headers is considered when fetching the > headers. Since not having CP header implies not having via in the CP header, you > could replace the assertNotHasChromeProxyViaHeader. > > The syntax would be something like: > > self.assert(response['request_headers']['chrome-proxy']) > > In the same vein, it might be worth adding a check to make sure we bypass other > navigations that happen within a minute of this one, so maybe > LoadURL("http://check.googlezip.net/test.html") and verify that the CP header is > not present. Can you check if the retried request is treated as a separate request/response? If so, maybe just leave the Via header checks and add a subsequent navigation that also just checks the via header in the response.
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/31 20:39:10, Ryan Sturm wrote: > https://codereview.chromium.org/2666813004/diff/1/tools/chrome_proxy/webdrive... > File tools/chrome_proxy/webdriver/bypass.py (right): > > https://codereview.chromium.org/2666813004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/bypass.py:18: for response in t.GetHTTPResponses(): > On 2017/01/31 19:48:23, Ryan Sturm wrote: > > tl;dr: Should we check if the CP header is present in request headers? Should > we > > call LoadURL on a different http://check.googlezip.net page afterwards to > verify that > > it does not use DRP? > > > > If I understand this test correctly (which I might not), when LoadURL is > called, > > subsequent requests will not be attempted via DRP for 1-5 minutes (randomly in > > that interval), and this request will be retried without DRP. > > > > If that is the case, it might be worth also checking if checking if the CP > > header is also not in the request headers. > > > > Since the main frame request is retried with two different sets of headers, I > am > > guessing that only the second set of headers is considered when fetching the > > headers. Since not having CP header implies not having via in the CP header, > you > > could replace the assertNotHasChromeProxyViaHeader. > > > > The syntax would be something like: > > > > self.assert(response['request_headers']['chrome-proxy']) > > > > In the same vein, it might be worth adding a check to make sure we bypass > other > > navigations that happen within a minute of this one, so maybe > > LoadURL("http://check.googlezip.net/test.html") and verify that the CP header > is > > not present. > > Can you check if the retried request is treated as a separate request/response? > If so, maybe just leave the Via header checks and add a subsequent navigation > that also just checks the via header in the response. PTAL if you want to Ryan, otherwise I'll land in the next hour or so.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm The second loop over t.GetHTTPResponses() is looping over both page load responses, FYI. That might be more of a harness issue, so this is probably good in case the underlying response aggregation changes in GetHTTPResponses().
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertogden@chromium.org Link to the patchset: https://codereview.chromium.org/2666813004/#ps20001 (title: "test next page is bypassed")
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": 1485907335053780,
"parent_rev": "2028c664f02f529fa688418bc7607800bf7dc521", "commit_rev":
"63e8b962f4207e65dd6357a86089b0115d2c21f6"}
Message was sent while issue was closed.
Description was changed from ========== Implement the Chrome Proxy bypass integration test with ChromeDriver BUG=680554 ========== to ========== Implement the Chrome Proxy bypass integration test with ChromeDriver BUG=680554 Review-Url: https://codereview.chromium.org/2666813004 Cr-Commit-Position: refs/heads/master@{#447379} Committed: https://chromium.googlesource.com/chromium/src/+/63e8b962f4207e65dd6357a86089... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/63e8b962f4207e65dd6357a86089... |
