Fixes crbug.com/378064 - record_wpr no longerworks with benchmark

Fixes crbug.com/378064 - record_wpr no longerworks with benchmark

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

[nednguyen, 2014-06-04 04:06:16]

On 2014/06/04 03:55:46, ariblue wrote:

Please don't remove the check. These kind of failures on the recording are
signal telling that the measurement will also fail.

[nednguyen, 2014-06-04 04:20:15]

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

https://codereview.chromium.org/312043003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/record_wpr.py:44: self.test.options =
self.options
This doesn't seem like the right fix to me. 

Can you take a look at why
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

doesn't get run properly?

[nednguyen, 2014-06-04 20:14:04]

On 2014/06/04 04:20:15, nednguyen wrote:
>
https://codereview.chromium.org/312043003/diff/1/tools/telemetry/telemetry/pa...
> File tools/telemetry/telemetry/page/record_wpr.py (right):
> 
>
https://codereview.chromium.org/312043003/diff/1/tools/telemetry/telemetry/pa...
> tools/telemetry/telemetry/page/record_wpr.py:44: self.test.options =
> self.options
> This doesn't seem like the right fix to me. 
> 
> Can you take a look at why
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> doesn't get run properly?

Sorry, after discussion it turns out to be the right fix for this particular
problem. However, we also agree that it's better for RecordPage to inherit from
actual page_measurement test rather than having it as the wrapper that owns the
test.

[ariblue, 2014-06-05 04:13:33]

On 2014/06/04 20:14:04, nednguyen wrote:
> On 2014/06/04 04:20:15, nednguyen wrote:
> >
>
https://codereview.chromium.org/312043003/diff/1/tools/telemetry/telemetry/pa...
> > File tools/telemetry/telemetry/page/record_wpr.py (right):
> > 
> >
>
https://codereview.chromium.org/312043003/diff/1/tools/telemetry/telemetry/pa...
> > tools/telemetry/telemetry/page/record_wpr.py:44: self.test.options =
> > self.options
> > This doesn't seem like the right fix to me. 
> > 
> > Can you take a look at why
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > 
> > doesn't get run properly?
> 
> Sorry, after discussion it turns out to be the right fix for this particular
> problem. However, we also agree that it's better for RecordPage to inherit
from
> actual page_measurement test rather than having it as the wrapper that owns
the
> test.

Okay, I've made a few changes:

(1) After chatting with Ned, I agree that this change should not be modifying
the timeline_based_measurement.py. That change has been reverted.

(2) I moved the MeasurePage logic from before the action sequence to after.
There are two reasons for this. First, this allows record_wpr.py to more closely
follow page_test.py's implementation of the RunPage() function. Second, since in
our tests, we initialize all the interactions in the action sequence, calling
MeasurePage() before we actually do anything throws an 'Expected at least one
Interaction on the page' exception.

(3) I switched from using MeasurePage to ValidatePage. Again, this allows us to
more closely follow page_test.py's logic. Also, someone could theoretically
overwrite the ValidatePage function in their PageMeasurement class, in which
case we would be running the wrong thing here.

(4) I moved the sleep(3) outside of the if statement. It looks like this part is
going to get re-factored anyways, and I couldn't see a good reason why we would
want to run it in one case and not the other.

Definitely let me know if anyone has questions. This CL is meant to be a quick
fix for the problem at hand, not a comprehensive update to the record_wpr logic.

[tonyg, 2014-06-05 04:18:51]

lgtm

[nednguyen, 2014-06-05 04:50:22]

lgtm

[ariblue, 2014-06-05 18:21:16]

The CQ bit was checked by ariblue@google.com

[commit-bot: I haz the power, 2014-06-05 18:23:00]

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

[dtu, 2014-06-05 18:31:20]

lgtm

[dtu, 2014-06-05 18:31:21]

lgtm

lgtm

[commit-bot: I haz the power, 2014-06-05 22:27:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 02:54:24]

Change committed as 275302