Telemetry: re-enable cpu metric for page_cycler.

Telemetry: re-enable cpu metric for page_cycler.

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

[bulach, 2014-04-22 10:25:43]

ptal (I bumped into it as part of https://codereview.chromium.org/239083010/ :))

[qyearsley, 2014-04-22 17:02:18]

On 2014/04/22 10:25:43, bulach wrote:
> ptal (I bumped into it as part of https://codereview.chromium.org/239083010/
:))

I don't remember clearly why CPU metric was disabled -- there might have been
something a little bit wrong about the results that it was reporting in some
cases, but I can't remember clearly what it was ... do you remember there being
a problem, Tony?

Anyway, much thanks! The CPU stats in page_cycler have been abandoned
for too long :-)

[tonyg, 2014-04-22 18:31:42]

lgtm

[commit-bot: I haz the power, 2014-04-22 18:34:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/246453004/1

[commit-bot: I haz the power, 2014-04-22 19:05:26]

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

[commit-bot: I haz the power, 2014-04-22 19:05:27]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[bulach, 2014-04-23 09:38:39]

I should've known better, sorry! I fixed the unittests in the latest patchset...
it was a simple change that added more coverage, but would you mind another
quick look please?

[qyearsley, 2014-04-23 17:03:29]

On 2014/04/23 09:38:39, bulach wrote:
> I should've known better, sorry! I fixed the unittests in the latest
patchset...
> it was a simple change that added more coverage, but would you mind another
> quick look please?

On 2014/04/23 09:38:39, bulach wrote:
> I should've known better, sorry! I fixed the unittests in the latest
patchset...
> it was a simple change that added more coverage, but would you mind another
> quick look please?

By the principle of "one test method only testing one behavior", it seems like
it shouldn't be necessary to check that there are per-page values with the names
  'cpu_utilization.cpu_utilization_gpu'
  'cpu_utilization.cpu_utilization_renderer'
  'cpu_utilization.cpu_utilization_browser'
in two test different methods, called testCacheHandled and testColdWarm.

Maybe there should be a separate test method testPerPageValueNames which only
checks
these value names.

Also, the CPU values (which depend on 'CpuProcessTime' and 'TotalTime' for each
process type, which are set by FakeBrowser._iteration) are not tested here...
which I think is okay, since I think that test should be in
metrics/cpu_unittest.py.

[bulach, 2014-04-23 17:11:46]

https://codereview.chromium.org/246453004/diff/10001/tools/perf/measurements/...
File tools/perf/measurements/page_cycler_unittest.py (right):

https://codereview.chromium.org/246453004/diff/10001/tools/perf/measurements/...
tools/perf/measurements/page_cycler_unittest.py:142:
self.assertEqual(values[0].page, page)
please advise: note that here and the test below, it had already broken the
principle you mentioned as these are not truly "unit", they're larger sort of
integration-type tests..

I'm happy to consolidate (141-147) and (171-181) together in its own test if you
prefer.

as for he values themselves: happy to test if you prefer, but as you said, the
actual computation is deeper down in metrics/cpu..

please let me know your thoughts!

[qyearsley, 2014-04-23 18:13:38]

https://codereview.chromium.org/246453004/diff/10001/tools/perf/measurements/...
File tools/perf/measurements/page_cycler_unittest.py (right):

https://codereview.chromium.org/246453004/diff/10001/tools/perf/measurements/...
tools/perf/measurements/page_cycler_unittest.py:142:
self.assertEqual(values[0].page, page)
On 2014/04/23 17:11:47, bulach wrote:
> please advise: note that here and the test below, it had already broken the
> principle you mentioned as these are not truly "unit", they're larger sort of
> integration-type tests..
> 
> I'm happy to consolidate (141-147) and (171-181) together in its own test if
you
> prefer.
> 
> as for he values themselves: happy to test if you prefer, but as you said, the
> actual computation is deeper down in metrics/cpu..
> 
> please let me know your thoughts!

My thoughts are that this CL LGTM as is. But, in theory, the test would be
easier to understand if it were re-organized so that one test method tests only
one thing. Perhaps outside of the scope of this CL.

[bulach, 2014-04-24 08:57:00]

https://codereview.chromium.org/246453004/diff/10001/tools/perf/measurements/...
File tools/perf/measurements/page_cycler_unittest.py (right):

https://codereview.chromium.org/246453004/diff/10001/tools/perf/measurements/...
tools/perf/measurements/page_cycler_unittest.py:142:
self.assertEqual(values[0].page, page)
On 2014/04/23 18:13:38, qyearsley wrote:
> On 2014/04/23 17:11:47, bulach wrote:
> > please advise: note that here and the test below, it had already broken the
> > principle you mentioned as these are not truly "unit", they're larger sort
of
> > integration-type tests..
> > 
> > I'm happy to consolidate (141-147) and (171-181) together in its own test if
> you
> > prefer.
> > 
> > as for he values themselves: happy to test if you prefer, but as you said,
the
> > actual computation is deeper down in metrics/cpu..
> > 
> > please let me know your thoughts!
> 
> My thoughts are that this CL LGTM as is. But, in theory, the test would be
> easier to understand if it were re-organized so that one test method tests
only
> one thing. Perhaps outside of the scope of this CL.

sgtm! I'll CQ this as is and send you a separate CL shortly splitting the  tests
shortly.

[bulach, 2014-04-24 08:57:05]

The CQ bit was checked by bulach@chromium.org

[commit-bot: I haz the power, 2014-04-24 08:57:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/246453004/10001

[bulach, 2014-04-24 09:30:05]

The CQ bit was unchecked by bulach@chromium.org

[bulach, 2014-04-24 09:30:30]

On 2014/04/24 08:57:20, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/bulach@chromium.org/246453004/10001

unchecking CQ, I didn't see tony's request :)
will hold off until after the fixit.

[bulach, 2014-05-01 14:48:53]

The CQ bit was checked by bulach@chromium.org

[commit-bot: I haz the power, 2014-05-01 14:49:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/246453004/10001

[commit-bot: I haz the power, 2014-05-01 14:52:05]

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

[commit-bot: I haz the power, 2014-05-01 14:52:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[bulach, 2014-05-06 08:42:12]

The CQ bit was checked by bulach@chromium.org

[commit-bot: I haz the power, 2014-05-06 08:42:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bulach@chromium.org/246453004/10001

[commit-bot: I haz the power, 2014-05-06 08:51:48]

Change committed as 268470