Check if device disconnects every time the last test of a revision fails

Check if device disconnects every time the last test of a revision fails

Right now when the test device is disconnected from host, the host will
not rerun the tests on its other available device, but will continue
using the disconnected device and fail all the tests. This problem can be
solved by checking connected devices in every revision test and raising
step failure if test device disconnects.
BUG=chromium:635183
Committed: https://chromium.googlesource.com/chromium/tools/build/+/2c78661f30da29ac3d08a6e26b2392ada22dbba4

[Ziqi Xiong, 2016-08-05 23:28:52]

zxiong@google.com changed reviewers:
+ robertocn@chromium.org

[Ziqi Xiong, 2016-08-05 23:28:53]

This might fix the bisect problem we saw today?

[RobertoCN, 2016-08-05 23:35:46]

It's worth noting that the reason why we need to manually raise a step failure
is because we are catching them further down the line to enable: a) return_code
bisects and b)bisecting on benchmarks that are a little flaky.

This LGTM, but let's get someone else's opinion on the matter, we should add
sullivan@ or dtu@ as reviewers.

[Ziqi Xiong, 2016-08-06 01:16:04]

zxiong@google.com changed reviewers:
+ dtu@chromium.org, sullivan@chromium.org

[sullivan, 2016-08-08 22:13:28]

Agreed with Roberto that there should be more code comments here.

Dave, do you have an opinion on Roberto's comments?

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:359: if
test_results['retcodes'][-1] != 0 and (
Is test_results['retcodes'] always defined? Is it always an array with a length?
Roberto, can you help us understand how defensively to program here?

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:361:
api.m.chromium_android.device_status()
What does device_status() do?

[sullivan, 2016-08-08 22:45:13]

lgtm after addressing comments

[RobertoCN, 2016-08-08 22:46:43]

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:359: if
test_results['retcodes'][-1] != 0 and (
On 2016/08/08 22:13:28, sullivan wrote:
> Is test_results['retcodes'] always defined? Is it always an array with a
length?
> Roberto, can you help us understand how defensively to program here?

it will always be a list. And as long as the test runs once, it will not be
empty. I guess it won't hurt to do:
if test_results.get('retcodes') and test_results['retcodes'][-1] !=0....

[Ziqi Xiong, 2016-08-09 18:59:11]

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:359: if
test_results['retcodes'][-1] != 0 and (
On 2016/08/08 22:46:42, RobertoCN wrote:
> On 2016/08/08 22:13:28, sullivan wrote:
> > Is test_results['retcodes'] always defined? Is it always an array with a
> length?
> > Roberto, can you help us understand how defensively to program here?
> 
> it will always be a list. And as long as the test runs once, it will not be
> empty. I guess it won't hurt to do:
> if test_results.get('retcodes') and test_results['retcodes'][-1] !=0....

Done.

https://codereview.chromium.org/2224583003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:361:
api.m.chromium_android.device_status()
On 2016/08/08 22:13:28, sullivan wrote:
> What does device_status() do?

It rechecks the devices connected to host and then updates
api.m.chromium_android.devices.

[dtu, 2016-08-09 23:30:11]

lgtm

[Ziqi Xiong, 2016-08-09 23:34:47]

The CQ bit was checked by zxiong@google.com

[Ziqi Xiong, 2016-08-09 23:34:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org, robertocn@chromium.org
Link to the patchset: https://codereview.chromium.org/2224583003/#ps20001
(title: "Review fix")

[commit-bot: I haz the power, 2016-08-09 23:34:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 23:40:29]

Description was changed from

==========
Check if device disconnects every time the last test of a revision fails

Right now when the test device is disconnected from host, the host will
not rerun the tests on its other available device, but will continue
using the disconnected device and fail all the tests. This problem can be
solved by checking connected devices in every revision test and raising
step failure if test device disconnects.
BUG=chromium:635183
==========

to

==========
Check if device disconnects every time the last test of a revision fails

Right now when the test device is disconnected from host, the host will
not rerun the tests on its other available device, but will continue
using the disconnected device and fail all the tests. This problem can be
solved by checking connected devices in every revision test and raising
step failure if test device disconnects.
BUG=chromium:635183

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/2c78661f30da29ac3d08...
==========

[commit-bot: I haz the power, 2016-08-09 23:40:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/build/+/2c78661f30da29ac3d08...