Empty per_page_values shouldn't imply test success

Empty per_page_values shouldn't imply test success.

The entire test harness failed which caused per_page_values to come back empty, so that the recipes found no concrete failures, and thus assumed that there was success. Now we pay attention to the return code explicitly, so that we will report failure in that case.

Also changed default step_test_data for LocalTelemetryGPUTest to be passing instead of failing, which was nonsense.

Depends on https://codereview.chromium.org/1366353002/ (starting on patchset 13, so inter-patch diffs won't work across that boundary)

BUG=536192, 499940
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296913

[Sergey Berezin, 2015-09-26 00:58:58]

sergeyberezin@chromium.org changed reviewers:
+ sergeyberezin@chromium.org

[Sergey Berezin, 2015-09-26 00:58:58]

lgtm

[luqui, 2015-09-26 02:23:19]

luqui@chromium.org changed reviewers:
+ sergiyb@chromium.org

[luqui, 2015-09-26 02:23:20]

ptal +sergiyb@, as you are the original author

[Sergiy Byelozyorov, 2015-09-28 13:55:30]

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
scripts/slave/recipes/chromium_trybot.py:8: import json
this is probably a debugging leftover, please remove

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
File scripts/slave/recipes/gpu/download_and_test.expected/linux_debug.json
(left):

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
scripts/slave/recipes/gpu/download_and_test.expected/linux_debug.json:266:
"@@@STEP_TEXT@<br/>failures:<br/>Test.Test1<br/>@@@"
I find it strange that we loose reported failures everywhere... shouldn't this
only affect tests that have non-zero return value? and even if we do not fail a
specific test, shouldn't we get a failure of the entire test suite.

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
File scripts/slave/recipes/gpu/download_and_test.py (right):

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
scripts/slave/recipes/gpu/download_and_test.py:88:
api.test_utils.canned_telemetry_gpu_output(
why was this test changed?

[luqui, 2015-09-28 17:33:35]

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
scripts/slave/recipes/chromium_trybot.py:8: import json
On 2015/09/28 13:55:29, Sergiy Byelozyorov wrote:
> this is probably a debugging leftover, please remove

Done.

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
File scripts/slave/recipes/gpu/download_and_test.expected/linux_debug.json
(left):

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
scripts/slave/recipes/gpu/download_and_test.expected/linux_debug.json:266:
"@@@STEP_TEXT@<br/>failures:<br/>Test.Test1<br/>@@@"
On 2015/09/28 13:55:29, Sergiy Byelozyorov wrote:
> I find it strange that we loose reported failures everywhere... shouldn't this
> only affect tests that have non-zero return value? and even if we do not fail
a
> specific test, shouldn't we get a failure of the entire test suite.

This is due to the steps.py change, which changes the default for "Test1" from
failing to success.  We assume that it was default failing as a lazy way to get
coverage, but it's strange to have a test fail by default.

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
File scripts/slave/recipes/gpu/download_and_test.py (right):

https://codereview.chromium.org/1372723002/diff/230001/scripts/slave/recipes/...
scripts/slave/recipes/gpu/download_and_test.py:88:
api.test_utils.canned_telemetry_gpu_output(
On 2015/09/28 13:55:29, Sergiy Byelozyorov wrote:
> why was this test changed?

In order to pass per_page_values correctly, which the test logic depends on.

[luqui, 2015-09-28 17:35:29]

The CQ bit was checked by luqui@chromium.org

[luqui, 2015-09-28 17:35:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/1372723002/#ps250001
(title: "Remove dead code")

[commit-bot: I haz the power, 2015-09-28 17:36:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372723002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372723002/250001

[commit-bot: I haz the power, 2015-09-28 17:44:39]

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

[commit-bot: I haz the power, 2015-09-28 17:44:40]

Try jobs failed on following builders:
  build_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)

[Sergiy Byelozyorov, 2015-09-28 17:46:09]

lgtm

[luqui, 2015-09-28 18:10:00]

The CQ bit was checked by luqui@chromium.org

[commit-bot: I haz the power, 2015-09-28 18:10:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372723002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372723002/250001

[commit-bot: I haz the power, 2015-09-28 18:14:46]

Committed patchset #14 (id:250001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296913

[Ken Russell (switch to Gerrit), 2015-09-28 18:26:50]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-09-28 18:26:51]

LGTM after the fact. Thank you for tracking this down and fixing it on such
short and urgent notice!