Move max_failures option from PageTest to benchmark.

Move max_failures option from PageTest to benchmark.

BUG=440101
Committed: https://crrev.com/b86b8b1a1cd213734f58a4282269f89c8a285d32
Cr-Commit-Position: refs/heads/master@{#308924}

[chrishenry, 2014-12-13 02:45:40]

chrishenry@google.com changed reviewers:
+ kbr@chromium.org, nednguyen@google.com, slamm@chromium.org

[nednguyen, 2014-12-15 17:47:09]

I think testMaxFailuresOption* in page_run_end_to_end_unittest should be moved
to user_story_runner_unittest.

[slamm, 2014-12-15 18:19:25]

Moving the tests seems like a good idea.

https://codereview.chromium.org/790673005/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/790673005/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner.py:225:
effective_max_failures = max_failures
One other option here is to set effective_max_failures to infinity instead of
the None check in the loop.

226:   if effective_max_failures is None:
227:     effective_max_failures = 1e309  # infinity

[Ken Russell (switch to Gerrit), 2014-12-15 19:00:05]

GPU test changes LGTM

[chrishenry, 2014-12-17 16:45:50]

On 2014/12/15 17:47:09, nednguyen wrote:
> I think testMaxFailuresOption* in page_run_end_to_end_unittest should be moved
> to user_story_runner_unittest.

Moved the unit tests and make it not depend on Page.

[slamm, 2014-12-17 23:53:22]

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right):

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:454: def
_testMaxFailuresOptionIsRespectedAndOverridable(self, max_failures=None):
Test methods that use this would be clearer if they gave both the inputs and
expected outputs:

def _testMaxFailures(self, runner_max_failures, options_max_failures,
expected_num_failures):

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:494: for ii
in range(5):
Add a constant at the top of the function:

    NUM_PAGES = 5

Index variable is unused:

    for _ in range(NUM_PAGES):
      ...

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:512: for ii
in range(len(uss.user_stories)):
for ii, user_story in enumerate(uss.user_stories):
  self.assertEqual(ii < expected_num_failures, user_story.was_run)

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:519:
self._testMaxFailuresOptionIsRespectedAndOverridable()
self._testMaxFailuresOptionIsRespectedAndOverridable(
    runner_max_failures=3, options_max_failures=None, expected_num_failures=4)

Give the +1 comment here?

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:522:
self._testMaxFailuresOptionIsRespectedAndOverridable(1)
self._testMaxFailuresOptionIsRespectedAndOverridable(
    runner_max_failures=3, options_max_failures=1, expected_num_failures=2)

[nednguyen, 2014-12-18 00:41:52]

lgtm

[slamm, 2014-12-18 01:11:57]

lgtm. My suggestions can be done later (or skipped).

[slamm, 2014-12-18 01:12:03]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-12-18 01:13:29]

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

[chrishenry, 2014-12-18 01:27:01]

Eh yeah, I already addressed most of them.

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right):

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:454: def
_testMaxFailuresOptionIsRespectedAndOverridable(self, max_failures=None):
On 2014/12/17 23:53:22, slamm wrote:
> Test methods that use this would be clearer if they gave both the inputs and
> expected outputs:
> 
> def _testMaxFailures(self, runner_max_failures, options_max_failures,
> expected_num_failures):

Done.

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:494: for ii
in range(5):
On 2014/12/17 23:53:22, slamm wrote:
> Add a constant at the top of the function:
> 
>     NUM_PAGES = 5
> 
> Index variable is unused:
> 
>     for _ in range(NUM_PAGES):
>       ...

Not needed.

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:512: for ii
in range(len(uss.user_stories)):
On 2014/12/17 23:53:22, slamm wrote:
> for ii, user_story in enumerate(uss.user_stories):
>   self.assertEqual(ii < expected_num_failures, user_story.was_run)

Done.

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:519:
self._testMaxFailuresOptionIsRespectedAndOverridable()
On 2014/12/17 23:53:22, slamm wrote:
> self._testMaxFailuresOptionIsRespectedAndOverridable(
>     runner_max_failures=3, options_max_failures=None, expected_num_failures=4)
> 
> Give the +1 comment here?

Done.

https://codereview.chromium.org/790673005/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:522:
self._testMaxFailuresOptionIsRespectedAndOverridable(1)
On 2014/12/17 23:53:22, slamm wrote:
> self._testMaxFailuresOptionIsRespectedAndOverridable(
>     runner_max_failures=3, options_max_failures=1, expected_num_failures=2)

Done.

[commit-bot: I haz the power, 2014-12-18 02:01:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-18 02:02:24]

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