Remove page_measurement_results.

Remove page_measurement_results.

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

[chrishenry, 2014-07-31 05:01:13]

https://codereview.chromium.org/436453002/diff/1/tools/perf/metrics/test_page...
File tools/perf/metrics/test_page_measurement_results.py (right):

https://codereview.chromium.org/436453002/diff/1/tools/perf/metrics/test_page...
tools/perf/metrics/test_page_measurement_results.py:10: class
TestPageTestResults(
Can we kill this class instead? Pretty please? Let's just fix the users to check
for the values directly.

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

https://codereview.chromium.org/436453002/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/record_wpr.py:66: # TODO(chrishenry): Fix the
need for a dummy_results object.
I think we don't need no dummy_results anymore.

[nednguyen, 2014-07-31 14:24:51]

https://codereview.chromium.org/436453002/diff/1/tools/perf/metrics/test_page...
File tools/perf/metrics/test_page_measurement_results.py (right):

https://codereview.chromium.org/436453002/diff/1/tools/perf/metrics/test_page...
tools/perf/metrics/test_page_measurement_results.py:10: class
TestPageTestResults(
On 2014/07/31 05:01:12, chrishenry wrote:
> Can we kill this class instead? Pretty please? Let's just fix the users to
check
> for the values directly.

I can do it a different patch. There maybe some the common pattern of testing
results that appear often enough that people need at least utility functions to
compare results.

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

https://codereview.chromium.org/436453002/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/record_wpr.py:66: # TODO(chrishenry): Fix the
need for a dummy_results object.
On 2014/07/31 05:01:13, chrishenry wrote:
> I think we don't need no dummy_results anymore.

Done.

[chrishenry, 2014-07-31 15:24:27]

lgtm, but please see the comment on chrome_proxy below.

https://codereview.chromium.org/436453002/diff/1/tools/perf/metrics/test_page...
File tools/perf/metrics/test_page_measurement_results.py (right):

https://codereview.chromium.org/436453002/diff/1/tools/perf/metrics/test_page...
tools/perf/metrics/test_page_measurement_results.py:10: class
TestPageTestResults(
On 2014/07/31 14:24:51, nednguyen wrote:
> On 2014/07/31 05:01:12, chrishenry wrote:
> > Can we kill this class instead? Pretty please? Let's just fix the users to
> check
> > for the values directly.
> 
> I can do it a different patch. There maybe some the common pattern of testing
> results that appear often enough that people need at least utility functions
to
> compare results.

There might be indeed, but it shouldn't need a subclass of PageTestResults
(perhaps a subclass of unittest or just a free function).

That said, the assertion below is poorly implemented anyway.

https://codereview.chromium.org/436453002/diff/20001/tools/perf/metrics/test_...
File tools/perf/metrics/test_page_measurement_results.py (right):

https://codereview.chromium.org/436453002/diff/20001/tools/perf/metrics/test_...
tools/perf/metrics/test_page_measurement_results.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
Btw I believe this is being used by chrome_proxy too:
https://code.google.com/p/chromium/codesearch#search/&type=cs&q=TestPageMeasu...

Since we are going to delete this anyway (or modify it in the next patch),
perhaps you want to just make this inherits from PageTestResults without
changing the name? If you're changing the name anyway, please rename the file
name.

[nednguyen, 2014-07-31 15:44:52]

https://codereview.chromium.org/436453002/diff/20001/tools/perf/metrics/test_...
File tools/perf/metrics/test_page_measurement_results.py (right):

https://codereview.chromium.org/436453002/diff/20001/tools/perf/metrics/test_...
tools/perf/metrics/test_page_measurement_results.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/07/31 15:24:27, chrishenry wrote:
> Btw I believe this is being used by chrome_proxy too:
>
https://code.google.com/p/chromium/codesearch#search/&type=cs&q=TestPageMeasu...
> 
> Since we are going to delete this anyway (or modify it in the next patch),
> perhaps you want to just make this inherits from PageTestResults without
> changing the name? If you're changing the name anyway, please rename the file
> name.

Done. Renamed the file.

[nednguyen, 2014-07-31 15:44:57]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-07-31 15:46:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nednguyen@google.com/436453002/40001

[commit-bot: I haz the power, 2014-07-31 15:47:00]

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

[commit-bot: I haz the power, 2014-07-31 15:47:01]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tonyg, 2014-07-31 15:56:43]

lgtm

[commit-bot: I haz the power, 2014-07-31 22:22:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nednguyen, 2014-07-31 22:33:00]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-07-31 22:34:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nednguyen@google.com/436453002/60001

[commit-bot: I haz the power, 2014-08-01 06:10:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 06:44:49]

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

[commit-bot: I haz the power, 2014-08-01 06:44:50]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishenry, 2014-08-01 06:53:28]

Oops, sorry about the merge. Some additional thoughts on the unittest as well
(was reading it again post-rebase).

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results_unittest.py (right):

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:31: def
_PrintPerfResult(self, *args):
I don't understand these two subclasses btw. _PrintPerfResult does nothing since
we never call it. We can probably replace both with plain PageTestResult.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:68:
results.StartTest(self.pages[0])
Sorry about the merge, these need to change to WillRunPage, DidRunPage.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:134:
results.AddValue(failure.FailureValue.FromMessage(self.pages[0], 'message'))
This AddValue should happen between WillRunPage and DidRunPage. (It's ok now I
guess, but we should fix it anyway).

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:139:
results.AddValue(failure.FailureValue.FromMessage(self.pages[1], 'message'))
Here too.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:142:
self.assertEquals(results.results, [])
Here, the only place where results.results is used, we're comparing it with [].
Wat?

[nednguyen, 2014-08-06 05:03:02]

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results_unittest.py (right):

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:31: def
_PrintPerfResult(self, *args):
On 2014/08/01 06:53:28, chrishenry wrote:
> I don't understand these two subclasses btw. _PrintPerfResult does nothing
since
> we never call it. We can probably replace both with plain PageTestResult.

These are legacy that left from the day when buidbot, etc still subclass
page_measurement_results.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:68:
results.StartTest(self.pages[0])
On 2014/08/01 06:53:28, chrishenry wrote:
> Sorry about the merge, these need to change to WillRunPage, DidRunPage.

Done.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:134:
results.AddValue(failure.FailureValue.FromMessage(self.pages[0], 'message'))
On 2014/08/01 06:53:28, chrishenry wrote:
> This AddValue should happen between WillRunPage and DidRunPage. (It's ok now I
> guess, but we should fix it anyway).

Done.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:139:
results.AddValue(failure.FailureValue.FromMessage(self.pages[1], 'message'))
On 2014/08/01 06:53:28, chrishenry wrote:
> Here too.

Done.

https://codereview.chromium.org/436453002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results_unittest.py:142:
self.assertEquals(results.results, [])
On 2014/08/01 06:53:28, chrishenry wrote:
> Here, the only place where results.results is used, we're comparing it with
[].
> Wat?

Modify this test to test pages_that_succeeded is empty.

[nednguyen, 2014-08-06 05:03:08]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-08-06 05:04:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nednguyen@google.com/436453002/100001

[commit-bot: I haz the power, 2014-08-06 08:03:47]

Change committed as 287734