Kill AddError/AddErrorMessage from PageTestResults.

Kill AddError/AddErrorMessage from PageTestResults, switch existing usage to use AddFailure.

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

[Ken Russell (switch to Gerrit), 2014-07-15 00:20:20]

Please send this CL to the try servers. A few comments.

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_conformance.py (left):

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_conformance.py:72:
super(WebglConformanceValidator, self).__init__(attempts=1, max_errors=10)
What's the side-effect of this change? The maximum number of failures should be
clamped to 10, too, in order to prevent runaway test runs if somebody breaks
something badly in one of their try jobs, and each failure would for whatever
reason have a really long timeout.

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

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/profile_generator.py:51: if results.failures:
This test is redundant. It was before, too.

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/gtest_test_results_unittest.py (left):

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/gtest_test_results_unittest.py:72: def
testSingleErrorPage(self):
Please add a new test which exceeds the maximum number of failures of the
validator and ensures that it stops after running that number.

[nduca, 2014-07-15 02:02:39]

lgtm

[nduca, 2014-07-15 02:03:28]

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_conformance.py (left):

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_conformance.py:72:
super(WebglConformanceValidator, self).__init__(attempts=1, max_errors=10)
is it important to distinguish between errors and failures for your logic?

[nednguyen, 2014-07-15 03:07:43]

lgtm

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

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:40:
self.pages_that_had_failures.add(page)
It seems like this pages_that_had_failures is not needed?

[Ken Russell (switch to Gerrit), 2014-07-15 04:42:19]

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_conformance.py (left):

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_conformance.py:72:
super(WebglConformanceValidator, self).__init__(attempts=1, max_errors=10)
On 2014/07/15 02:03:28, nduca wrote:
> is it important to distinguish between errors and failures for your logic?

No (Brandon, Mo, correct me if I'm wrong), but it is important to abort runs
that are failing lots of tests.

[chrishenry, 2014-07-15 17:55:53]

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_conformance.py (left):

https://codereview.chromium.org/390233002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_conformance.py:72:
super(WebglConformanceValidator, self).__init__(attempts=1, max_errors=10)
On 2014/07/15 00:20:20, Ken Russell wrote:
> What's the side-effect of this change? The maximum number of failures should
be
> clamped to 10, too, in order to prevent runaway test runs if somebody breaks
> something badly in one of their try jobs, and each failure would for whatever
> reason have a really long timeout.

Previously this test doesn't clamp down on number of failures, only errors. I
added max_failures=10 to this call, so it'll now clamp down on failures too.

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

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/profile_generator.py:51: if results.failures:
On 2014/07/15 00:20:20, Ken Russell wrote:
> This test is redundant. It was before, too.

Done.

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/gtest_test_results_unittest.py (left):

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/gtest_test_results_unittest.py:72: def
testSingleErrorPage(self):
On 2014/07/15 00:20:20, Ken Russell wrote:
> Please add a new test which exceeds the maximum number of failures of the
> validator and ensures that it stops after running that number.

Added the test in page_runner_unittest (where this behavior is implemented).

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

https://codereview.chromium.org/390233002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:40:
self.pages_that_had_failures.add(page)
On 2014/07/15 03:07:42, nednguyen wrote:
> It seems like this pages_that_had_failures is not needed?

It is being used from PageMeasurementResults.

[Ken Russell (switch to Gerrit), 2014-07-15 18:27:47]

Nice, thanks. LGTM

[chrishenry, 2014-07-15 19:53:43]

Hi Ken, is there any specific try bot you want me to run that is not already run
by CQ? Otherwise, I'll let CQ run them instead. Let me know. (:

[Ken Russell (switch to Gerrit), 2014-07-15 20:09:44]

On 2014/07/15 19:53:43, chrishenry wrote:
> Hi Ken, is there any specific try bot you want me to run that is not already
run
> by CQ? Otherwise, I'll let CQ run them instead. Let me know. (:

No, the CQ contains all the relevant bots. Feel free to go ahead.

[chrishenry, 2014-07-16 22:55:17]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-07-16 23:00:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/390233002/120001

[commit-bot: I haz the power, 2014-07-17 07:06:37]

Change committed as 283679