Refactor of record_wpr.py

Refactor of record_wpr.py

* Breaks major steps out into functions
* Adds additional tests
* Adds support for command line flags defined in benchmarks
* Calls the PageTest RunPage directly, rather than kinda sorta reimplementing parts ourselves
* Fixes running benchmarks with record_wpr.py (crbug.com/378064)
* Page sets are referenced via class name on the command line, rather than by filename.

Other minor changes:
* Output will be different for failed pages at the end, using the default results PrintSummary() rather than a separate implementation (in this case GTestTestResults).

BUG=378064, 367292
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285490

[chrishenry, 2014-07-16 23:41:30]

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:33:
page.file_path.startswith(util.GetUnittestDataDir()))
The unit test should start a HTTP server instead. Similar to TabTestCase (this
may help:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...)

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:70: # dummy PageMeasurementResults
that implements the functions we use.
Can you add a TODO(chrishenry) to remove this?

What do you need from PageMeasurementResults in particular?

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:75: super(RecorderPageTest,
self).RunPage(page, tab, dummy_results)
Why is it calling super(RecorderPageTest, self).RunPage instead of
self.page_test.RunPage?

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:92: def
FindAllActionNames(base_dir):
Make private?

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:98: if not issubclass(cls,
profile_creator.ProfileCreator)]
This is tempting, but may be more readable as:
action_names = set()
for _, cls in ...:
  if not issubclass...:
    ...

return action_names

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr_unittest.py (right):

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr_unittest.py:136: _base_dir =
util.GetPerfDir()
Can you make this not dependent on perf dir? Telemetry code (and unit tests)
should not depend on perf.

[ariblue, 2014-07-17 01:27:26]

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:70: # dummy PageMeasurementResults
that implements the functions we use.
On 2014/07/16 23:41:30, chrishenry wrote:
> Can you add a TODO(chrishenry) to remove this?
> 
> What do you need from PageMeasurementResults in particular?

During the RunPage() sequence in a PageTest, we call the ValidatePage() which is
where testing logic usually lives. For a PageMeasurement, this involves running
the following sequence:
  results.WillMeasurePage(page)
  self.MeasurePage(page,tab,results)
  results.DidMeasurePage()

So our results object needs to have WillMeasurePage and DidMeasurePage
functions.

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:75: super(RecorderPageTest,
self).RunPage(page, tab, dummy_results)
On 2014/07/16 23:41:30, chrishenry wrote:
> Why is it calling super(RecorderPageTest, self).RunPage instead of
> self.page_test.RunPage?

Good question. Fixed.

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:92: def
FindAllActionNames(base_dir):
On 2014/07/16 23:41:30, chrishenry wrote:
> Make private?

Whats the best way to make this private and still test it directly from the
_unittest.py without triggering the "Access to a protected member
_FindAllActionNames of a client class" pylint error?

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:98: if not issubclass(cls,
profile_creator.ProfileCreator)]
On 2014/07/16 23:41:30, chrishenry wrote:
> This is tempting, but may be more readable as:
> action_names = set()
> for _, cls in ...:
>   if not issubclass...:
>     ...
> 
> return action_names

Done.

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr_unittest.py (right):

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr_unittest.py:136: _base_dir =
util.GetPerfDir()
On 2014/07/16 23:41:30, chrishenry wrote:
> Can you make this not dependent on perf dir? Telemetry code (and unit tests)
> should not depend on perf.

Yeah, this dependency isn't needed. Fixed.

[ariblue, 2014-07-17 22:13:22]

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:33:
page.file_path.startswith(util.GetUnittestDataDir()))
On 2014/07/16 23:41:30, chrishenry wrote:
> The unit test should start a HTTP server instead. Similar to TabTestCase (this
> may help:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...)

Done.

[chrishenry, 2014-07-18 00:11:28]

lgtm, but you'll still need either dtu/tonyg's lgtm too.

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/374793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:92: def
FindAllActionNames(base_dir):
On 2014/07/17 01:27:26, ariblue wrote:
> On 2014/07/16 23:41:30, chrishenry wrote:
> > Make private?
> 
> Whats the best way to make this private and still test it directly from the
> _unittest.py without triggering the "Access to a protected member
> _FindAllActionNames of a client class" pylint error?

None I guess... s: It's probably fine to leave it public if you need to unit
test it.

[tonyg, 2014-07-24 16:28:36]

lgtm++

Wow, this is so much better and thanks for the much needed unittesting.

ps. Sorry, this totally slipped my radar for some reason. For future reference,
it's completely appropriate to ping if you don't get a code review response in
24 hours.

[ariblue, 2014-07-24 18:56:16]

The CQ bit was checked by ariblue@google.com

[commit-bot: I haz the power, 2014-07-24 18:58:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ariblue@google.com/374793002/220001

[ariblue, 2014-07-24 19:36:57]

The CQ bit was checked by ariblue@google.com

[commit-bot: I haz the power, 2014-07-24 19:38:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ariblue@google.com/374793002/240001

[commit-bot: I haz the power, 2014-07-24 23:46:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-25 05:44:18]

Change committed as 285490