[Telemetry] Introduce RunState.SetUpBeforeRunPage and RunState.TearDownAfterRunPage.

[Telemetry] Introduce RunState.SetUpBeforeRunPage and RunState.TearDownAfterRunPage.

This is a step closer to the point of making _RunState --> UserStoryState with
the 4 main methods:

__init__ that does classSetUp
SetUpBeforeRunPage that does setUp before each page run
TearDownAfterRunPage that does tearDown after each page run
TearDownAfterRunPageSet that does final classTearDown

BUG=418278
Committed: https://crrev.com/f88052ffcc4a5ad7883c4ca9e3cf39c3e346597d
Cr-Commit-Position: refs/heads/master@{#302695}

[nednguyen, 2014-10-30 22:31:23]

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

[chrishenry, 2014-10-30 23:25:47]

It's coming together nicely (:

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:69: def
SetUpBeforeRunningPage(self, page):
Why not just WillRunPage, DidRunPage, DidRunPageSet? Any reason why you choose
the current naming?

(I also don't see DidRunPageSet, but your description says it.)

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:78: if
(self._test.StopBrowserAfterPage(self.browser, self._current_page)):
nit: remove unnecessary () around conditional.

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:114:
self.browser.platform.network_controller.UpdateReplayForExistingBrowser()
Should this just be self.platform.network_controll...

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:300:
state.StartBrowserIfNeeded(test, page_set, page)
(Will this eventually move to WillRunPage as well? Not on this patch.)

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:308:
_WaitForThermalThrottlingIfNeeded(state.platform)
Should this be part of SetUpBeforeRunningPage?

[nednguyen, 2014-10-31 02:03:58]

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:69: def
SetUpBeforeRunningPage(self, page):
On 2014/10/30 23:25:47, chrishenry wrote:
> Why not just WillRunPage, DidRunPage, DidRunPageSet? Any reason why you choose
> the current naming?

I just worry about the WillRunPage() name is used in multiple place.

> 
> (I also don't see DidRunPageSet, but your description says it.)

Yeah, DidRunPageSet will come in next patches.

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:300:
state.StartBrowserIfNeeded(test, page_set, page)
On 2014/10/30 23:25:47, chrishenry wrote:
> (Will this eventually move to WillRunPage as well? Not on this patch.)

It will.

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:308:
_WaitForThermalThrottlingIfNeeded(state.platform)
On 2014/10/30 23:25:47, chrishenry wrote:
> Should this be part of SetUpBeforeRunningPage?

Maybe not if this should be run for every type of user story.

[slamm, 2014-10-31 04:47:43]

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:44: # Create a possible_browser
with the given options.
Change the comment to a doc string. Perhaps, "Return a possible_browser ..."""
(As we know, it may not get created during this call.)

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:49: sys.exit(-1)
Raise an exception and leave it up to the caller to exit?
Same for the other calls to sys.exit. (Although the exit(0) is not exceptional.)

[nednguyen, 2014-11-03 16:57:13]

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:49: sys.exit(-1)
On 2014/10/31 04:47:43, slamm wrote:
> Raise an exception and leave it up to the caller to exit?
> Same for the other calls to sys.exit. (Although the exit(0) is not
exceptional.)

Done.

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:78: if
(self._test.StopBrowserAfterPage(self.browser, self._current_page)):
On 2014/10/30 23:25:47, chrishenry wrote:
> nit: remove unnecessary () around conditional.

Done.

https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:114:
self.browser.platform.network_controller.UpdateReplayForExistingBrowser()
On 2014/10/30 23:25:47, chrishenry wrote:
> Should this just be self.platform.network_controll...

Done.

[nednguyen, 2014-11-04 17:20:20]

On 2014/11/03 16:57:13, nednguyen wrote:
>
https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
> File tools/telemetry/telemetry/page/page_runner.py (right):
> 
>
https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
> tools/telemetry/telemetry/page/page_runner.py:49: sys.exit(-1)
> On 2014/10/31 04:47:43, slamm wrote:
> > Raise an exception and leave it up to the caller to exit?
> > Same for the other calls to sys.exit. (Although the exit(0) is not
> exceptional.)
> 
> Done.
> 
>
https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
> tools/telemetry/telemetry/page/page_runner.py:78: if
> (self._test.StopBrowserAfterPage(self.browser, self._current_page)):
> On 2014/10/30 23:25:47, chrishenry wrote:
> > nit: remove unnecessary () around conditional.
> 
> Done.
> 
>
https://codereview.chromium.org/692973003/diff/1/tools/telemetry/telemetry/pa...
> tools/telemetry/telemetry/page/page_runner.py:114:
> self.browser.platform.network_controller.UpdateReplayForExistingBrowser()
> On 2014/10/30 23:25:47, chrishenry wrote:
> > Should this just be self.platform.network_controll...
> 
> Done.

PTAL again.

[chrishenry, 2014-11-04 19:28:10]

lgtm

but as discussed offline, let's use Will/DidRunPage for now. The new name
doesn't address the concern you brought up with Will/DidRunPage naming anyway
(execution order). As long as they have the same lifecycle, being consistent in
naming them would be preferable.

[nednguyen, 2014-11-04 19:42:14]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-11-04 19:43:48]

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

[commit-bot: I haz the power, 2014-11-04 21:16:14]

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

[commit-bot: I haz the power, 2014-11-04 21:16:15]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nednguyen, 2014-11-04 21:22:34]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-11-04 21:23:44]

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

[commit-bot: I haz the power, 2014-11-04 22:56:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-04 22:57:48]

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