Added integration test to Chrome Proxy to verify fallback to HTTP after bad HTTPS response from pro…

Added integration test to Chrome Proxy to verify fallback to HTTP after bad HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the chromeproxy-test server which will give a 404 error on the request, the client will fallback to using a HTTP proxy instead.

BUG=430206
Committed: https://crrev.com/820e88826d774eeb9b3f2e3eee9b79839ece7ce9
Cr-Commit-Position: refs/heads/master@{#412603}

[Robert Ogden, 2016-08-09 20:10:20]

Description was changed from

==========
Added integration test to Chrome Proxy to verify fallback to HTTP after bad
HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the
chromeproxy-test server which will give a 404 error on the request, the client
will fallback to using a HTTP proxy instead.

Implemented test

BUG=
==========

to

==========
Added integration test to Chrome Proxy to verify fallback to HTTP after bad
HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the
chromeproxy-test server which will give a 404 error on the request, the client
will fallback to using a HTTP proxy instead.

BUG=430206
==========

[Robert Ogden, 2016-08-09 20:10:20]

robertogden@chromium.org changed reviewers:
+ bustamante@chromium.org, sclittle@chromium.org

[Robert Ogden, 2016-08-09 20:11:08]

Completed second test case. Please review :)

[sclittle, 2016-08-09 20:29:16]

Sorry for going all meta here, but do we really need this test? IIRC, we already
have a test ChromeProxyHTTPFallbackViaHeader that checks that the missing via
header bypass works, isn't that essentially the same as what this test does? Or
am I misunderstanding the new test that you're adding?

We also have a test ChromeProxyHTTPToDirectFallback that tests that the primary
proxy is bypassed if the connection fail, and that bypasses work properly for
the fallback proxy.

We should probably go through all the old integration test crbugs and decide
which ones really need to be done, and which ones are already covered by other
integration tests or better covered by unit tests.

I'm just worried that we're making our lives harder by adding redundant
integration tests, which adds more test flake and code to maintain.

[bustamante, 2016-08-09 20:42:08]

On 2016/08/09 20:29:16, sclittle wrote:
> Sorry for going all meta here, but do we really need this test? IIRC, we
already
> have a test ChromeProxyHTTPFallbackViaHeader that checks that the missing via
> header bypass works, isn't that essentially the same as what this test does?
Or
> am I misunderstanding the new test that you're adding?
> 
> We also have a test ChromeProxyHTTPToDirectFallback that tests that the
primary
> proxy is bypassed if the connection fail, and that bypasses work properly for
> the fallback proxy.
> 
> We should probably go through all the old integration test crbugs and decide
> which ones really need to be done, and which ones are already covered by other
> integration tests or better covered by unit tests.
> 
> I'm just worried that we're making our lives harder by adding redundant
> integration tests, which adds more test flake and code to maintain.

Yeah I think so, specifically this tests that XT will fallback to http. 
Compared to ChromeProxyHTTPFallbackViaHeader which tests a proxy that returns a
canned bad response from a fake proxy.

For example a bad config / build push could enable https for XT and this would
catch that.  The point of end-to-end tests is to test that the actual services
behave as expected.  If you just mock out everything with fake services, it's
easy to have gaps in test coverage.

[sclittle, 2016-08-09 20:47:41]

On 2016/08/09 20:42:08, bustamante wrote:
> On 2016/08/09 20:29:16, sclittle wrote:
> > Sorry for going all meta here, but do we really need this test? IIRC, we
> already
> > have a test ChromeProxyHTTPFallbackViaHeader that checks that the missing
via
> > header bypass works, isn't that essentially the same as what this test does?
> Or
> > am I misunderstanding the new test that you're adding?
> > 
> > We also have a test ChromeProxyHTTPToDirectFallback that tests that the
> primary
> > proxy is bypassed if the connection fail, and that bypasses work properly
for
> > the fallback proxy.
> > 
> > We should probably go through all the old integration test crbugs and decide
> > which ones really need to be done, and which ones are already covered by
other
> > integration tests or better covered by unit tests.
> > 
> > I'm just worried that we're making our lives harder by adding redundant
> > integration tests, which adds more test flake and code to maintain.
> 
> Yeah I think so, specifically this tests that XT will fallback to http. 
> Compared to ChromeProxyHTTPFallbackViaHeader which tests a proxy that returns
a
> canned bad response from a fake proxy.
> 
> For example a bad config / build push could enable https for XT and this would
> catch that.  The point of end-to-end tests is to test that the actual services
> behave as expected.  If you just mock out everything with fake services, it's
> easy to have gaps in test coverage.

OK, that makes sense then. Thanks for clarifying this.

[Robert Ogden, 2016-08-09 21:12:47]

The CQ bit was checked by robertogden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 21:13:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 22:00:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 22:00:03]

Dry run: This issue passed the CQ dry run.

[bustamante, 2016-08-12 20:01:57]

Looks good, I just have one comment.

https://codereview.chromium.org/2227163002/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/2227163002/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:197:
'https://%s;http://compress.googlezip.net' % _TEST_SERVER)
In some cases we'll want to set the proxy to an internal url, which we can do
when calling the test.  But this may overwrite that.  Have you tried running the
test setting the proxy to XT with command line flags?

I agree we can't check in internal url's since it's public

[Robert Ogden, 2016-08-15 17:13:14]

Yes, the proxy url can be overridden via the command line with
--extra-browser-args="--data-reduction-proxy-http-proxies=%s"

https://codereview.chromium.org/2227163002/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/2227163002/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:197:
'https://%s;http://compress.googlezip.net' % _TEST_SERVER)
On 2016/08/12 20:01:57, bustamante wrote:
> In some cases we'll want to set the proxy to an internal url, which we can do
> when calling the test.  But this may overwrite that.  Have you tried running
the
> test setting the proxy to XT with command line flags?
> 
> I agree we can't check in internal url's since it's public

Yes, and it can be overridden from the command line with
--extra-browser-args="--data-reduction-proxy-http-proxies=%s"

[bustamante, 2016-08-15 17:45:38]

On 2016/08/15 17:13:14, Robert Ogden wrote:
> Yes, the proxy url can be overridden via the command line with
> --extra-browser-args="--data-reduction-proxy-http-proxies=%s"
> 
>
https://codereview.chromium.org/2227163002/diff/1/tools/chrome_proxy/integrat...
> File tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py
(right):
> 
>
https://codereview.chromium.org/2227163002/diff/1/tools/chrome_proxy/integrat...
> tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py:197:
> 'https://%s;http://compress.googlezip.net' % _TEST_SERVER)
> On 2016/08/12 20:01:57, bustamante wrote:
> > In some cases we'll want to set the proxy to an internal url, which we can
do
> > when calling the test.  But this may overwrite that.  Have you tried running
> the
> > test setting the proxy to XT with command line flags?
> > 
> > I agree we can't check in internal url's since it's public
> 
> Yes, and it can be overridden from the command line with
> --extra-browser-args="--data-reduction-proxy-http-proxies=%s"

lgtm, cool!

[Robert Ogden, 2016-08-15 17:49:03]

updated to remove hardcoded proxy setting

[sclittle, 2016-08-17 17:20:58]

lgtm % nit

https://codereview.chromium.org/2227163002/diff/40001/tools/chrome_proxy/inte...
File
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/bad_https_fallback.py
(right):

https://codereview.chromium.org/2227163002/diff/40001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/bad_https_fallback.py:23:
'http://check.googlezip.net/test.html'
Instead of creating a whole new pageset here, could you just use the existing
synthetic pageset?
https://cs.chromium.org/chromium/src/tools/chrome_proxy/integration_tests/chr...

[Robert Ogden, 2016-08-17 18:14:56]

The CQ bit was checked by robertogden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 18:15:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Robert Ogden, 2016-08-17 18:15:35]

sclittle@, is this what you meant? See Patch 4

https://codereview.chromium.org/2227163002/diff/40001/tools/chrome_proxy/inte...
File
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/bad_https_fallback.py
(right):

https://codereview.chromium.org/2227163002/diff/40001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/bad_https_fallback.py:23:
'http://check.googlezip.net/test.html'
On 2016/08/17 17:20:58, sclittle wrote:
> Instead of creating a whole new pageset here, could you just use the existing
> synthetic pageset?
>
https://cs.chromium.org/chromium/src/tools/chrome_proxy/integration_tests/chr...

Done.

[sclittle, 2016-08-17 18:24:54]

On 2016/08/17 18:15:35, Robert Ogden wrote:
> sclittle@, is this what you meant? See Patch 4
> 
>
https://codereview.chromium.org/2227163002/diff/40001/tools/chrome_proxy/inte...
> File
>
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/bad_https_fallback.py
> (right):
> 
>
https://codereview.chromium.org/2227163002/diff/40001/tools/chrome_proxy/inte...
>
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/bad_https_fallback.py:23:
> 'http://check.googlezip.net/test.html'
> On 2016/08/17 17:20:58, sclittle wrote:
> > Instead of creating a whole new pageset here, could you just use the
existing
> > synthetic pageset?
> >
>
https://cs.chromium.org/chromium/src/tools/chrome_proxy/integration_tests/chr...
> 
> Done.

Yep, that's what I meant, thanks! Still LGTM

[commit-bot: I haz the power, 2016-08-17 18:58:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 18:58:44]

Dry run: This issue passed the CQ dry run.

[Robert Ogden, 2016-08-17 18:59:18]

The CQ bit was checked by robertogden@chromium.org

[Robert Ogden, 2016-08-17 18:59:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
bustamante@chromium.org
Link to the patchset: https://codereview.chromium.org/2227163002/#ps60001
(title: "Uses SyntheticStorySet instead now")

[commit-bot: I haz the power, 2016-08-17 18:59:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 19:03:47]

Description was changed from

==========
Added integration test to Chrome Proxy to verify fallback to HTTP after bad
HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the
chromeproxy-test server which will give a 404 error on the request, the client
will fallback to using a HTTP proxy instead.

BUG=430206
==========

to

==========
Added integration test to Chrome Proxy to verify fallback to HTTP after bad
HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the
chromeproxy-test server which will give a 404 error on the request, the client
will fallback to using a HTTP proxy instead.

BUG=430206
==========

[commit-bot: I haz the power, 2016-08-17 19:03:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-17 19:05:49]

Description was changed from

==========
Added integration test to Chrome Proxy to verify fallback to HTTP after bad
HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the
chromeproxy-test server which will give a 404 error on the request, the client
will fallback to using a HTTP proxy instead.

BUG=430206
==========

to

==========
Added integration test to Chrome Proxy to verify fallback to HTTP after bad
HTTPS response from proxy.

This checks that when the proxy responds with an error, in this case using the
chromeproxy-test server which will give a 404 error on the request, the client
will fallback to using a HTTP proxy instead.

BUG=430206

Committed: https://crrev.com/820e88826d774eeb9b3f2e3eee9b79839ece7ce9
Cr-Commit-Position: refs/heads/master@{#412603}
==========

[commit-bot: I haz the power, 2016-08-17 19:05:50]

Patchset 4 (id:??) landed as
https://crrev.com/820e88826d774eeb9b3f2e3eee9b79839ece7ce9
Cr-Commit-Position: refs/heads/master@{#412603}