Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714003002:
Adding a probe fallback to HTTP test (Closed)
DescriptionAdding a probe fallback to HTTP test
This tests that when the probe fails to get an "OK" from the secure
proxy server that requests are served using the unsecured proxy server
(i.e., over HTTP).
BUG=680558
Review-Url: https://codereview.chromium.org/2714003002
Cr-Commit-Position: refs/heads/master@{#452916}
Committed: https://chromium.googlesource.com/chromium/src/+/fdf80d0bbaa412014232cbe84299492207386d9c
Patch Set 1 #Patch Set 2 : reworded comments #
Total comments: 6
Patch Set 3 : tbansal spin on histogram #
Total comments: 1
Patch Set 4 : tbansal nit #Messages
Total messages: 27 (15 generated)
The CQ bit was checked by ryansturm@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...
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + robertogden@chromium.org, tbansal@chromium.org
tbansal, robertogden: PTAL
https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/fallback.py (right): https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): SecureProxyCheckFallback https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/fallback.py:31: self.assertEqual(u'http/1.1', response.protocol) Is this flaky? why is it guaranteed that the secure proxy check URL will be fetched before the URL is loaded?
https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/fallback.py (right): https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): On 2017/02/23 22:26:34, tbansal1 wrote: > SecureProxyCheckFallback I wasn't sure if there were other fallbacks intended for this file. https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/fallback.py:31: self.assertEqual(u'http/1.1', response.protocol) On 2017/02/23 22:26:34, tbansal1 wrote: > Is this flaky? why is it guaranteed that the secure proxy check URL will be > fetched before the URL is loaded? Hmm. Any suggestions here? Like the copied comment says, it was flaky until someone switched to the google.com favicon. My proposal is navigating to http://www.google.com/favicon.ico throwing out those responses and then navigating to http://www.google.com/favicon.ico
> My proposal is navigating to http://www.google.com/favicon.ico throwing out > those responses and then navigating to http://www.google.com/favicon.ico ** The second navigation would be to http://check.googlezip.net/test.html
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/23 22:33:17, Ryan Sturm wrote: > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/fallback.py (right): > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): > On 2017/02/23 22:26:34, tbansal1 wrote: > > SecureProxyCheckFallback > > I wasn't sure if there were other fallbacks intended for this file. > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/fallback.py:31: self.assertEqual(u'http/1.1', > response.protocol) > On 2017/02/23 22:26:34, tbansal1 wrote: > > Is this flaky? why is it guaranteed that the secure proxy check URL will be > > fetched before the URL is loaded? > > Hmm. Any suggestions here? Like the copied comment says, it was flaky until > someone switched to the http://google.com favicon. Can you run this 10 or so times, and see if it is flaky. We can add a generic wait function since this will come up again and again: Wait until a certain histogram bucket is populated, and then you can wait until the histogram for secure proxy check is populated. > > My proposal is navigating to http://www.google.com/favicon.ico throwing out > those responses and then navigating to http://www.google.com/favicon.ico I still do not think that this guarantees that secure proxy check URL will be fetched first.
On 2017/02/23 22:50:50, tbansal1 wrote: > On 2017/02/23 22:33:17, Ryan Sturm wrote: > > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > > File tools/chrome_proxy/webdriver/fallback.py (right): > > > > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > > tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): > > On 2017/02/23 22:26:34, tbansal1 wrote: > > > SecureProxyCheckFallback > > > > I wasn't sure if there were other fallbacks intended for this file. > > > > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > > tools/chrome_proxy/webdriver/fallback.py:31: self.assertEqual(u'http/1.1', > > response.protocol) > > On 2017/02/23 22:26:34, tbansal1 wrote: > > > Is this flaky? why is it guaranteed that the secure proxy check URL will be > > > fetched before the URL is loaded? > > > > Hmm. Any suggestions here? Like the copied comment says, it was flaky until > > someone switched to the http://google.com favicon. > Can you run this 10 or so times, and see if it is flaky. > > We can add a generic wait function since this will come up again and again: > Wait until a certain histogram bucket is populated, and then you can wait until > the > histogram for secure proxy check is populated. > > > > My proposal is navigating to http://www.google.com/favicon.ico throwing out > > those responses and then navigating to http://www.google.com/favicon.ico > I still do not think that this guarantees that secure proxy check URL will be > fetched first. For some reason I thought that if two requests to the same URL are issued for a GET, then they will share the response, so when the first navigation finishes, it will mean that we will stop secure proxy use.
On 2017/02/23 22:56:05, Ryan Sturm wrote: > On 2017/02/23 22:50:50, tbansal1 wrote: > > On 2017/02/23 22:33:17, Ryan Sturm wrote: > > > > > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > > > File tools/chrome_proxy/webdriver/fallback.py (right): > > > > > > > > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > > > tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): > > > On 2017/02/23 22:26:34, tbansal1 wrote: > > > > SecureProxyCheckFallback > > > > > > I wasn't sure if there were other fallbacks intended for this file. > > > > > > > > > https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... > > > tools/chrome_proxy/webdriver/fallback.py:31: self.assertEqual(u'http/1.1', > > > response.protocol) > > > On 2017/02/23 22:26:34, tbansal1 wrote: > > > > Is this flaky? why is it guaranteed that the secure proxy check URL will > be > > > > fetched before the URL is loaded? > > > > > > Hmm. Any suggestions here? Like the copied comment says, it was flaky until > > > someone switched to the http://google.com favicon. > > Can you run this 10 or so times, and see if it is flaky. > > > > We can add a generic wait function since this will come up again and again: > > Wait until a certain histogram bucket is populated, and then you can wait > until > > the > > histogram for secure proxy check is populated. > > > > > > My proposal is navigating to http://www.google.com/favicon.ico throwing out > > > those responses and then navigating to http://www.google.com/favicon.ico > > I still do not think that this guarantees that secure proxy check URL will be > > fetched first. > > For some reason I thought that if two requests to the same URL are issued for a > GET, then they will share the response, so when the first navigation finishes, > it will mean that we will stop secure proxy use. No. Especially not for secure proxy check which uses its own fetcher and may be posting tasks (so the posted tasks may get processed later than other tasks, leading to delay). It is also that secure proxy URL is fetched on a different connection (non QUIC, non HTTP2), so we do not want to rely on which connection succeeds first.
lgtm https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/fallback.py (right): https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): On 2017/02/23 22:33:17, Ryan Sturm wrote: > On 2017/02/23 22:26:34, tbansal1 wrote: > > SecureProxyCheckFallback > > I wasn't sure if there were other fallbacks intended for this file. There are :) Good as is
The CQ bit was checked by ryansturm@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...
tbansal: PTAL
lgtm % 1 super nit. Thanks for adding the histogram function!! https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/fallback.py (right): https://codereview.chromium.org/2714003002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/fallback.py:9: class Fallback(IntegrationTest): On 2017/02/23 22:33:17, Ryan Sturm wrote: > On 2017/02/23 22:26:34, tbansal1 wrote: > > SecureProxyCheckFallback > > I wasn't sure if there were other fallbacks intended for this file. Acknowledged. https://codereview.chromium.org/2714003002/diff/40001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2714003002/diff/40001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:330: def GetHistogram(self, histogram, timeout=30): May be useful to add a comment that this is 30 seconds.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertogden@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2714003002/#ps60001 (title: "tbansal nit")
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": 60001, "attempt_start_ts": 1487966605608100,
"parent_rev": "f58111ac12ccc0969e28b634c82be76764d472fb", "commit_rev":
"fdf80d0bbaa412014232cbe84299492207386d9c"}
Message was sent while issue was closed.
Description was changed from ========== Adding a probe fallback to HTTP test This tests that when the probe fails to get an "OK" from the secure proxy server that requests are served using the unsecured proxy server (i.e., over HTTP). BUG=680558 ========== to ========== Adding a probe fallback to HTTP test This tests that when the probe fails to get an "OK" from the secure proxy server that requests are served using the unsecured proxy server (i.e., over HTTP). BUG=680558 Review-Url: https://codereview.chromium.org/2714003002 Cr-Commit-Position: refs/heads/master@{#452916} Committed: https://chromium.googlesource.com/chromium/src/+/fdf80d0bbaa412014232cbe84299... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/fdf80d0bbaa412014232cbe84299... |
