Integration test for Chrome-Proxy: pass-through directive

Integration test for Chrome-Proxy: pass-through directive

Checks that an image loaded with the Chrome-Proxy: pass-through directive
is larger than the compressed image from the Data Reduction Proxy.

BUG=490397
Committed: https://crrev.com/91e5715acfa6cac439f98438f1bedade3d863b9a
Cr-Commit-Position: refs/heads/master@{#331028}

[megjablon, 2015-05-21 01:24:10]

megjablon@chromium.org changed reviewers:
+ bengr@google.com, sclittle@chromium.org

[sclittle, 2015-05-21 01:54:25]

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/common/c...
File tools/chrome_proxy/common/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/common/c...
tools/chrome_proxy/common/chrome_proxy_metrics.py:98:
chrome_proxy_request_header = self.response.request_headers['Chrome-Proxy']
This code to extract a value from a header seems to be duplicated in a bunch of
places here, could you pull it out to a helper function?

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:21:
request.open("GET", "http://aws1.mdw.la/fw/buddy.jpg");
Please use something off of check.googlezip.net, we're trying to eliminate any
usage of aws1.mdw.la from the tests.

e.g. http://check.googlezip.net/image.png

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:21:
request.open("GET", "http://aws1.mdw.la/fw/buddy.jpg");
Also, don't hardcode the url here, just use |self.url|.

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
nit: add space after comma

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:35:
'http://aws1.mdw.la/fw/buddy.jpg',
same here, use http://check.googlezip.net/image.png or something instead of
aws1.mdw.la.

[Not at Google. Contact bengr, 2015-05-21 18:29:00]

kundaji@chromium.org changed reviewers:
+ kundaji@chromium.org

[Not at Google. Contact bengr, 2015-05-21 18:29:00]

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
For this test to be meaningful, "Chrome-Proxy: pass-through" should be added by
Chrome and not by this test. If some bug causes Chrome to start sending the
wrong header, this test would still pass.

[sclittle, 2015-05-21 18:43:05]

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
On 2015/05/21 18:29:00, kundaji wrote:
> For this test to be meaningful, "Chrome-Proxy: pass-through" should be added
by
> Chrome and not by this test. If some bug causes Chrome to start sending the
> wrong header, this test would still pass.

@kundaji:

IIRC the "Chrome-Proxy: pass-through" request header would be added when a user
clicks "Reload original image" or something in the context menu after
long-pressing an image. We should have a browser test for that, but I don't
think it's possible to trigger that action through Telemetry.

IIUC the idea is that (browser test for using the context menu to reload
original image causes Chrome to send "pass-through") + (this telemetry test for
"pass-through") = fully tested.

You're right that this test doesn't test the full DRP client behavior, and
ideally we'd have an integration test doing the whole thing together from the
context menu and talking to the real DRP, but I think this Javascript approach
is at least better than adding a new Chrome command line flag just for this test
that forces "Chrome-Proxy: pass-through".

WDYT?

[Not at Google. Contact bengr, 2015-05-21 18:50:19]

On 2015/05/21 18:43:05, sclittle wrote:
>
https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
> File
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
> (right):
> 
>
https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
> tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
> request.setRequestHeader("Chrome-Proxy","pass-through");
> On 2015/05/21 18:29:00, kundaji wrote:
> > For this test to be meaningful, "Chrome-Proxy: pass-through" should be added
> by
> > Chrome and not by this test. If some bug causes Chrome to start sending the
> > wrong header, this test would still pass.
> 
> @kundaji:
> 
> IIRC the "Chrome-Proxy: pass-through" request header would be added when a
user
> clicks "Reload original image" or something in the context menu after
> long-pressing an image. We should have a browser test for that, but I don't
> think it's possible to trigger that action through Telemetry.
> 
> IIUC the idea is that (browser test for using the context menu to reload
> original image causes Chrome to send "pass-through") + (this telemetry test
for
> "pass-through") = fully tested.
> 
> You're right that this test doesn't test the full DRP client behavior, and
> ideally we'd have an integration test doing the whole thing together from the
> context menu and talking to the real DRP, but I think this Javascript approach
> is at least better than adding a new Chrome command line flag just for this
test
> that forces "Chrome-Proxy: pass-through".
> 
> WDYT?

You are right: I forgot that Telemetry does not allow clicking menu options.
Given that we cannot force Chrome to send this header in an integration test,
the approach here is definitely better than nothing.

[Not at Google. Contact bengr, 2015-05-21 18:53:54]

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
On 2015/05/21 18:43:05, sclittle wrote:
> On 2015/05/21 18:29:00, kundaji wrote:
> > For this test to be meaningful, "Chrome-Proxy: pass-through" should be added
> by
> > Chrome and not by this test. If some bug causes Chrome to start sending the
> > wrong header, this test would still pass.
> 
> @kundaji:
> 
> IIRC the "Chrome-Proxy: pass-through" request header would be added when a
user
> clicks "Reload original image" or something in the context menu after
> long-pressing an image. We should have a browser test for that, but I don't
> think it's possible to trigger that action through Telemetry.
> 
> IIUC the idea is that (browser test for using the context menu to reload
> original image causes Chrome to send "pass-through") + (this telemetry test
for
> "pass-through") = fully tested.
> 
> You're right that this test doesn't test the full DRP client behavior, and
> ideally we'd have an integration test doing the whole thing together from the
> context menu and talking to the real DRP, but I think this Javascript approach
> is at least better than adding a new Chrome command line flag just for this
test
> that forces "Chrome-Proxy: pass-through".
> 
> WDYT?

You are right: I forgot that Telemetry does not allow clicking menu options.
Given that we cannot force Chrome to send this header in an integration test,
the approach here is definitely better than nothing.

[megjablon, 2015-05-21 19:03:06]

megjablon@chromium.org changed reviewers:
+ bengr@chromium.org
- bengr@google.com

[megjablon, 2015-05-21 19:03:07]

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/common/c...
File tools/chrome_proxy/common/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/common/c...
tools/chrome_proxy/common/chrome_proxy_metrics.py:98:
chrome_proxy_request_header = self.response.request_headers['Chrome-Proxy']
On 2015/05/21 01:54:24, sclittle wrote:
> This code to extract a value from a header seems to be duplicated in a bunch
of
> places here, could you pull it out to a helper function?

WDYT?

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:21:
request.open("GET", "http://aws1.mdw.la/fw/buddy.jpg");
On 2015/05/21 01:54:25, sclittle wrote:
> Please use something off of http://check.googlezip.net, we're trying to
eliminate any
> usage of aws1.mdw.la from the tests.
> 
> e.g. http://check.googlezip.net/image.png

Done.

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
On 2015/05/21 01:54:25, sclittle wrote:
> nit: add space after comma

Done.

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/integrat...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:35:
'http://aws1.mdw.la/fw/buddy.jpg',
On 2015/05/21 01:54:25, sclittle wrote:
> same here, use http://check.googlezip.net/image.png or something instead of
> aws1.mdw.la.

Done.

[sclittle, 2015-05-21 19:28:56]

LGTM % nit

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/common/c...
File tools/chrome_proxy/common/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1151753003/diff/1/tools/chrome_proxy/common/c...
tools/chrome_proxy/common/chrome_proxy_metrics.py:98:
chrome_proxy_request_header = self.response.request_headers['Chrome-Proxy']
On 2015/05/21 19:03:07, megjablon wrote:
> On 2015/05/21 01:54:24, sclittle wrote:
> > This code to extract a value from a header seems to be duplicated in a bunch
> of
> > places here, could you pull it out to a helper function?
> 
> WDYT?

Looks good, thanks!

https://codereview.chromium.org/1151753003/diff/20001/tools/chrome_proxy/inte...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/20001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
nit: add space after comma

[megjablon, 2015-05-21 19:41:34]

https://codereview.chromium.org/1151753003/diff/20001/tools/chrome_proxy/inte...
File tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py
(right):

https://codereview.chromium.org/1151753003/diff/20001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_pagesets/pass_through.py:22:
request.setRequestHeader("Chrome-Proxy","pass-through");
On 2015/05/21 19:28:56, sclittle wrote:
> nit: add space after comma

Ah fixed it and then un-did it. Fixed!

[megjablon, 2015-05-21 23:39:59]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-05-21 23:40:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1151753003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-21 23:41:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151753003/60001

[commit-bot: I haz the power, 2015-05-21 23:46:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-21 23:47:14]

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