Move top_20 tests to a separate suite

Move top_20 tests to a separate suite

We don't want run these tests on the continuous waterfall as they take a lot of time and can be flaky, but they're still valuable to ensure chrome_proxy works on the "Top 20" sites and can be run as needed.

BUG=480032
Committed: https://crrev.com/84809a6eb565c1bc54abb25c261f94ed2b4b3c83
Cr-Commit-Position: refs/heads/master@{#328357}

[bustamante, 2015-04-23 00:06:48]

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

[bustamante, 2015-04-23 00:07:38]

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

[bustamante, 2015-04-23 00:07:39]

Please take a look when you have a chance.

[sclittle, 2015-04-23 01:07:50]

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
File tools/chrome_proxy/live_tests/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:9: from
integration_tests import chrome_proxy_metrics as metrics
IIUC you're actually importing the integration_tests version of metrics.py here,
but you've also created a new metrics.py.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:14: class
ChromeProxyLatency(page_test.PageTest):
Could these latency and data savings tests be removed from the integration_tests
directory? The smoke test currently runs them, but these aren't really pass/fail
tests, so it doesn't really seem to make sense for them to be part of the smoke
test anyways.

The header validation test should continue to exist in the integration_tests
world though.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:53: class
ChromeProxyValidation(page_test.PageTest):
Maybe this file could import the old measurements.py, and just use the
ChromeProxyValidation defined there?

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:110: class
ChromeProxyHeaders(ChromeProxyValidation):
Does it really make sense to run the header validation test against the top20
sites? The test would fail if any of the resources was bypassed, which doesn't
really tell us much.

Maybe this should be a different test, that counts how many resources were
bypassed and how many weren't?

That's probably outside the scope of this CL though.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
File tools/chrome_proxy/live_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_metrics.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
This file is the same as the integration_tests one, why do we have two of them?

Instead of having duplication, maybe this file could just import that one, but
extend it with additional tests? Another option would be to cut this metrics
file down to only include the latency and data savings metrics or whatever.

[bustamante, 2015-04-23 02:26:09]

Thanks!  I made changes based on the comments below.

Regarding importing code from integration_tests, I wanted to create a separate
copy so that changes to integration_tests doesn't affect the live_tests. 
Alternatively we could move the files to separate directory, like "lib", where
it's clearer they're being used by more than one set of tests.  I'm happy to
discuss it.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
File tools/chrome_proxy/live_tests/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:9: from
integration_tests import chrome_proxy_metrics as metrics
On 2015/04/23 01:07:50, sclittle wrote:
> IIUC you're actually importing the integration_tests version of metrics.py
here,
> but you've also created a new metrics.py.

Yeah, Done.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:14: class
ChromeProxyLatency(page_test.PageTest):
On 2015/04/23 01:07:50, sclittle wrote:
> Could these latency and data savings tests be removed from the
integration_tests
> directory? The smoke test currently runs them, but these aren't really
pass/fail
> tests, so it doesn't really seem to make sense for them to be part of the
smoke
> test anyways.
> 
> The header validation test should continue to exist in the integration_tests
> world though.

Yeah I think we should remove them, or add a condition so that if there's 0%
data savings / some large latency value where it's obviously not working we can
fail the test.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:53: class
ChromeProxyValidation(page_test.PageTest):
On 2015/04/23 01:07:50, sclittle wrote:
> Maybe this file could import the old measurements.py, and just use the
> ChromeProxyValidation defined there?

The header test that used this got removed so we don't need it anymore.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:110: class
ChromeProxyHeaders(ChromeProxyValidation):
On 2015/04/23 01:07:50, sclittle wrote:
> Does it really make sense to run the header validation test against the top20
> sites? The test would fail if any of the resources was bypassed, which doesn't
> really tell us much.
> 
> Maybe this should be a different test, that counts how many resources were
> bypassed and how many weren't?
> 
> That's probably outside the scope of this CL though.

Yeah I agree, I'll remove the test for now, and later we can add it back if it's
useful.

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
File tools/chrome_proxy/live_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/1/tools/chrome_proxy/live_tes...
tools/chrome_proxy/live_tests/chrome_proxy_metrics.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/04/23 01:07:50, sclittle wrote:
> This file is the same as the integration_tests one, why do we have two of
them?
> 
> Instead of having duplication, maybe this file could just import that one, but
> extend it with additional tests? Another option would be to cut this metrics
> file down to only include the latency and data savings metrics or whatever.

I went with the latter option, apologies that should have been done in the first
iteration.

[sclittle, 2015-04-24 18:55:22]

A lib directory or something sounds good; stuff like network_metrics.py could be
shared between the integration_tests and the live_tests.

It might also be worth putting the class ChromeProxyBenchmark from benchmark.py
in there too somehow, and ChromeProxyRespose from metrics.py. Maybe also the
class ChromeProxyValidation from measurements.py, although that one isn't
actually used in any of the live_tests right now.

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
File tools/chrome_proxy/live_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
tools/chrome_proxy/live_tests/chrome_proxy_metrics.py:50: def
HasExtraViaHeader(self, extra_header):
I don't think these tests need the extra via header stuff, could this be
removed?

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
File tools/chrome_proxy/live_tests/network_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
tools/chrome_proxy/live_tests/network_metrics.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
Could this file be shared instead of duplicated? Maybe in a lib directory or
something like you were describing before.

[bengr, 2015-04-27 20:06:44]

On 2015/04/24 18:55:22, sclittle wrote:
> A lib directory or something sounds good; stuff like network_metrics.py could
be
> shared between the integration_tests and the live_tests.
> 
> It might also be worth putting the class ChromeProxyBenchmark from
benchmark.py
> in there too somehow, and ChromeProxyRespose from metrics.py. Maybe also the
> class ChromeProxyValidation from measurements.py, although that one isn't
> actually used in any of the live_tests right now.
> 
>
https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
> File tools/chrome_proxy/live_tests/chrome_proxy_metrics.py (right):
> 
>
https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
> tools/chrome_proxy/live_tests/chrome_proxy_metrics.py:50: def
> HasExtraViaHeader(self, extra_header):
> I don't think these tests need the extra via header stuff, could this be
> removed?
> 
>
https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
> File tools/chrome_proxy/live_tests/network_metrics.py (right):
> 
>
https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
> tools/chrome_proxy/live_tests/network_metrics.py:1: # Copyright 2014 The
> Chromium Authors. All rights reserved.
> Could this file be shared instead of duplicated? Maybe in a lib directory or
> something like you were describing before.

I'll defer to Scott's review.

[bustamante, 2015-04-29 20:28:03]

Thanks!  More changes:

- I initially renamed all the test files to remove chrome_proxy since it seemed
redundant, but in patch #5 I reverted that, since later we could potentially
another batch of tests.  This made the review hard to read, but if you compare
batch patch 5 to base it's clearer what's changed.

- Created "common" directory for shared code

- Moved ChromeProxyResponse, ChromeProxyBenchmark, and network_metrics to
common.

- Removed data savings and latency tests from the integration tests.

- Removed unused code in live_tests

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
File tools/chrome_proxy/live_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
tools/chrome_proxy/live_tests/chrome_proxy_metrics.py:50: def
HasExtraViaHeader(self, extra_header):
On 2015/04/24 18:55:22, sclittle wrote:
> I don't think these tests need the extra via header stuff, could this be
> removed?

Yeah, Done.

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
File tools/chrome_proxy/live_tests/network_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/40001/tools/chrome_proxy/live...
tools/chrome_proxy/live_tests/network_metrics.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/04/24 18:55:22, sclittle wrote:
> Could this file be shared instead of duplicated? Maybe in a lib directory or
> something like you were describing before.

Yes, Done.

[sclittle, 2015-04-30 00:07:59]

Could you separate the s/chrome_proxy_pagesets/pagesets/ renaming into a
separate CL? That would make it easier to review this CL.

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right):

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:6: from
integration_tests import pagesets as pagesets
nit: I think you can leave off the "as pagesets" at the end since it's already
imported as "pagesets"

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:14: class
ChromeProxyMetricException(page_test.MeasurementFailure):
Maybe this exception should be moved into common?

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/live...
File tools/chrome_proxy/live_tests/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/live...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:16: self._metrics =
metrics.ChromeProxyMetric()
I'm confused, where is metrics.ChromeProxyMetric()? I don't see it in the list
of files for this CL.

[bustamante, 2015-04-30 20:30:41]

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right):

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:6: from
integration_tests import pagesets as pagesets
On 2015/04/30 00:07:58, sclittle wrote:
> nit: I think you can leave off the "as pagesets" at the end since it's already
> imported as "pagesets"

Done.  I reverted the directory back to chrome_proxy_pagesets, so it makes sense
now.

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right):

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/inte...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:14: class
ChromeProxyMetricException(page_test.MeasurementFailure):
On 2015/04/30 00:07:58, sclittle wrote:
> Maybe this exception should be moved into common?

Done.

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/live...
File tools/chrome_proxy/live_tests/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/1098253004/diff/80001/tools/chrome_proxy/live...
tools/chrome_proxy/live_tests/chrome_proxy_measurements.py:16: self._metrics =
metrics.ChromeProxyMetric()
On 2015/04/30 00:07:58, sclittle wrote:
> I'm confused, where is metrics.ChromeProxyMetric()? I don't see it in the list
> of files for this CL.

I'm not sure why this worked, but I added back chrome_proxy_metrics.py with this
class and the validation code.  Done.

[sclittle, 2015-05-01 23:05:39]

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/int...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics_unittest.py
(right):

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/int...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics_unittest.py:8: from
common import chrome_proxy_metrics as com_metrics
nit: change to common_metrics

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/int...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics_unittest.py:162: def
testChromeProxyResponse(self):
Move this testChromeProxyResponse into a unittest file in the common directory,
since that's where the ChromeProxyResponse lives.

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/liv...
File tools/chrome_proxy/live_tests/chrome_proxy_metrics_unittest.py (right):

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/liv...
tools/chrome_proxy/live_tests/chrome_proxy_metrics_unittest.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
Could you update the run_tests script to run all the unittest files?

[bustamante, 2015-05-01 23:54:11]

Thanks!

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/int...
File tools/chrome_proxy/integration_tests/chrome_proxy_metrics_unittest.py
(right):

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/int...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics_unittest.py:8: from
common import chrome_proxy_metrics as com_metrics
On 2015/05/01 23:05:39, sclittle wrote:
> nit: change to common_metrics

Done.

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/int...
tools/chrome_proxy/integration_tests/chrome_proxy_metrics_unittest.py:162: def
testChromeProxyResponse(self):
On 2015/05/01 23:05:39, sclittle wrote:
> Move this testChromeProxyResponse into a unittest file in the common
directory,
> since that's where the ChromeProxyResponse lives.

Done.

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/liv...
File tools/chrome_proxy/live_tests/chrome_proxy_metrics_unittest.py (right):

https://codereview.chromium.org/1098253004/diff/140001/tools/chrome_proxy/liv...
tools/chrome_proxy/live_tests/chrome_proxy_metrics_unittest.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/05/01 23:05:39, sclittle wrote:
> Could you update the run_tests script to run all the unittest files?

It already does, I verified the tests are being run in all the unittest files. 
It looks like it iterates through everything in /tools/chrome_proxy/... and runs
any tests it finds.

[sclittle, 2015-05-02 00:01:01]

LGTM

[sclittle, 2015-05-02 00:29:21]

Sorry, just noticed this - it looks like this CL didn't modify
integration_tests/chrome_proxy_measurements.py, but ChromeProxyValidation should
have been pulled out of it.

https://codereview.chromium.org/1098253004/diff/160001/tools/chrome_proxy/com...
File tools/chrome_proxy/common/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/1098253004/diff/160001/tools/chrome_proxy/com...
tools/chrome_proxy/common/chrome_proxy_measurements.py:11: class
ChromeProxyValidation(page_test.PageTest):
Could you remove the copy of this class from
integration_tests/chrome_proxy_measurements.py?

[bustamante, 2015-05-02 01:04:58]

Good catch, thanks!

https://codereview.chromium.org/1098253004/diff/160001/tools/chrome_proxy/com...
File tools/chrome_proxy/common/chrome_proxy_measurements.py (right):

https://codereview.chromium.org/1098253004/diff/160001/tools/chrome_proxy/com...
tools/chrome_proxy/common/chrome_proxy_measurements.py:11: class
ChromeProxyValidation(page_test.PageTest):
On 2015/05/02 00:29:21, sclittle wrote:
> Could you remove the copy of this class from
> integration_tests/chrome_proxy_measurements.py?

Done.

[sclittle, 2015-05-02 01:06:52]

LGTM, thanks for doing this!

[bustamante, 2015-05-05 17:17:31]

The CQ bit was checked by bustamante@chromium.org

[commit-bot: I haz the power, 2015-05-05 17:20:52]

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

[commit-bot: I haz the power, 2015-05-05 18:13:37]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-05-05 18:15:08]

Patchset 10 (id:??) landed as
https://crrev.com/84809a6eb565c1bc54abb25c261f94ed2b4b3c83
Cr-Commit-Position: refs/heads/master@{#328357}