Add Data Saver ChromeDriver test for exp directive protocol.

Add Data Saver ChromeDriver test for exp directive protocol.

This test verifies that the Data Reduction Proxy understands the
"exp=<...>" directive in the Chrome-Proxy header, by verifying that the
specific exp directive test page gets bypassed when loaded with
"exp=test" set in the request.

The test also verifies that other pages aren't bypassed when "exp=test"
is set, and that the same exp directive test page loads through the
proxy without getting bypassed when "exp=test" is not set.

BUG=680557
Review-Url: https://codereview.chromium.org/2748233002
Cr-Commit-Position: refs/heads/master@{#457143}
Committed: https://chromium.googlesource.com/chromium/src/+/a5ff10d1ddd66832a3843cb16e4f024c40d9054e

[sclittle, 2017-03-14 21:45:35]

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

[megjablon, 2017-03-14 22:22:56]

lgtm w/ comment

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/bypass.py (right):

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/bypass.py:162: with TestDriver() as test_driver:
Rather than using another test driver in another test I did this by:

test_driver._StopDriver()
test_driver.RemoveChromeArg('--data-reduction-proxy-experiment=test')

Not sure which one is better, but we could decide on one to be consistent.

[sclittle, 2017-03-14 22:48:59]

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/bypass.py (right):

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/bypass.py:162: with TestDriver() as test_driver:
On 2017/03/14 22:22:55, megjablon wrote:
> Rather than using another test driver in another test I did this by:
> 
> test_driver._StopDriver()
> test_driver.RemoveChromeArg('--data-reduction-proxy-experiment=test')
> 
> Not sure which one is better, but we could decide on one to be consistent. 

Good point. @robertogden, WDYT?

I think underscore-prefixed methods are supposed to be used internally within a
class (https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming),
but we could just make StopDriver() public to fix that.

I think I'd still lean towards using a separate TestDriver, since the separate
indentation of each "with" block seems a bit easier to read to me. It also means
that we don't have to individually remove flags that are no longer needed, which
seems a bit simpler.

Using a separate test driver could lead to more duplication if lots of the same
flags are needed each time, but maybe that could be mitigated by making a helper
function that adds the flags that are common to each of those runs.

WDYT?

[Robert Ogden, 2017-03-15 15:43:29]

lgtm

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/bypass.py (right):

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/bypass.py:162: with TestDriver() as test_driver:
On 2017/03/14 22:48:59, sclittle wrote:
> On 2017/03/14 22:22:55, megjablon wrote:
> > Rather than using another test driver in another test I did this by:
> > 
> > test_driver._StopDriver()
> > test_driver.RemoveChromeArg('--data-reduction-proxy-experiment=test')
> > 
> > Not sure which one is better, but we could decide on one to be consistent. 
> 
> Good point. @robertogden, WDYT?
> 
> I think underscore-prefixed methods are supposed to be used internally within
a
> class
(https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming),
> but we could just make StopDriver() public to fix that.
> 
> I think I'd still lean towards using a separate TestDriver, since the separate
> indentation of each "with" block seems a bit easier to read to me. It also
means
> that we don't have to individually remove flags that are no longer needed,
which
> seems a bit simpler.
> 
> Using a separate test driver could lead to more duplication if lots of the
same
> flags are needed each time, but maybe that could be mitigated by making a
helper
> function that adds the flags that are common to each of those runs.
> 
> WDYT?

I agree using a separate 'with' is cleaner, although functionally equivalent.

[sclittle, 2017-03-15 18:01:36]

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/bypass.py (right):

https://codereview.chromium.org/2748233002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/bypass.py:162: with TestDriver() as test_driver:
On 2017/03/15 15:43:29, Robert Ogden wrote:
> On 2017/03/14 22:48:59, sclittle wrote:
> > On 2017/03/14 22:22:55, megjablon wrote:
> > > Rather than using another test driver in another test I did this by:
> > > 
> > > test_driver._StopDriver()
> > > test_driver.RemoveChromeArg('--data-reduction-proxy-experiment=test')
> > > 
> > > Not sure which one is better, but we could decide on one to be consistent.

> > 
> > Good point. @robertogden, WDYT?
> > 
> > I think underscore-prefixed methods are supposed to be used internally
within
> a
> > class
> (https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming),
> > but we could just make StopDriver() public to fix that.
> > 
> > I think I'd still lean towards using a separate TestDriver, since the
separate
> > indentation of each "with" block seems a bit easier to read to me. It also
> means
> > that we don't have to individually remove flags that are no longer needed,
> which
> > seems a bit simpler.
> > 
> > Using a separate test driver could lead to more duplication if lots of the
> same
> > flags are needed each time, but maybe that could be mitigated by making a
> helper
> > function that adds the flags that are common to each of those runs.
> > 
> > WDYT?
> 
> I agree using a separate 'with' is cleaner, although functionally equivalent.

OK, I'll leave this as-is then. Thanks!

[sclittle, 2017-03-15 18:01:59]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:03:01]

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, 2017-03-15 18:20:47]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489600919640160, "parent_rev":
"131dc94f32a5dfb6cdb08c1a650af8aa104ce3a5", "commit_rev":
"a5ff10d1ddd66832a3843cb16e4f024c40d9054e"}

[commit-bot: I haz the power, 2017-03-15 18:21:32]

Description was changed from

==========
Add Data Saver ChromeDriver test for exp directive protocol.

This test verifies that the Data Reduction Proxy understands the
"exp=<...>" directive in the Chrome-Proxy header, by verifying that the
specific exp directive test page gets bypassed when loaded with
"exp=test" set in the request.

The test also verifies that other pages aren't bypassed when "exp=test"
is set, and that the same exp directive test page loads through the
proxy without getting bypassed when "exp=test" is not set.

BUG=680557
==========

to

==========
Add Data Saver ChromeDriver test for exp directive protocol.

This test verifies that the Data Reduction Proxy understands the
"exp=<...>" directive in the Chrome-Proxy header, by verifying that the
specific exp directive test page gets bypassed when loaded with
"exp=test" set in the request.

The test also verifies that other pages aren't bypassed when "exp=test"
is set, and that the same exp directive test page loads through the
proxy without getting bypassed when "exp=test" is not set.

BUG=680557

Review-Url: https://codereview.chromium.org/2748233002
Cr-Commit-Position: refs/heads/master@{#457143}
Committed:
https://chromium.googlesource.com/chromium/src/+/a5ff10d1ddd66832a3843cb16e4f...
==========

[commit-bot: I haz the power, 2017-03-15 18:21:33]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a5ff10d1ddd66832a3843cb16e4f...