Updates the test runner script exit codes

Updates the test runner script exit codes

The script now returns exit codes based on what occurred in the
test. Exit codes:
0 -- normal
1 -- fail/crash
88 -- warning

In addition, the scripts now handle DeviceUnresponsiveError by
returning a warning exit code overall, rather than silently moving
on.

BUG=170477, 258171
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210749

[frankf, 2013-07-03 21:26:37]

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (left):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:157: test_collection.add(test)
This decreases robustness. Currently, if we catch this exception, we add the
tests back to queue to get retried.

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:217: run_results.exit_code =
constants.WARNING_EXIT_CODE
Perhaps a singleton object that flags this exception happened is cleaner in this
case, than propagating the exit_code through the results object. If we're
returning a warning code, we should also do logging.error() of which device is
unresponsive.

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/browserte...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/browserte...
build/android/pylib/browsertests/dispatch.py:25: def Dispatch(options):
Future TODO: There's a lot of duplicaiton in dispatch modules. Currently they
all:

1. Get list of devices
2. Reset test server allocation
3. Create runners and shard tests between them
4. report results (java tests now do this in test_runner.py)

The common code should be pulled out into test_runner.py

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:41: Exception: Test suite not found.
Exception if test ...

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:227: for suite_name, suite_path in
all_test_suites:
Future TODO: I don't think it's common workflow to run all the suites at once.
So let's make -s mandatory which is consistant with how instrumentation tests
are run as well.

https://codereview.chromium.org/18323020/diff/1/build/android/test_runner.py
File build/android/test_runner.py (left):

https://codereview.chromium.org/18323020/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:119:
option_parser.add_option('--buildbot-step-failure',
I think this is related to this CL, should we address it here?

[gkanwar, 2013-07-03 23:15:18]

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (left):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:157: test_collection.add(test)
On 2013/07/03 21:26:37, frankf wrote:
> This decreases robustness. Currently, if we catch this exception, we add the
> tests back to queue to get retried. 

I'm not sure I fully understand -- is this decreasing robustness by not adding
the tests back to the queue to be retried? The except clause below should catch
this exception and do that. The only change is whether we re-raise or silently
ignore the exception.

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:217: run_results.exit_code =
constants.WARNING_EXIT_CODE
On 2013/07/03 21:26:37, frankf wrote:
> Perhaps a singleton object that flags this exception happened is cleaner in
this
> case, than propagating the exit_code through the results object. If we're
> returning a warning code, we should also do logging.error() of which device is
> unresponsive.

That's a good point. Updated to use a singleton instead.

Logging added when we set this exit code here.

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/browserte...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/browserte...
build/android/pylib/browsertests/dispatch.py:25: def Dispatch(options):
On 2013/07/03 21:26:37, frankf wrote:
> Future TODO: There's a lot of duplicaiton in dispatch modules. Currently they
> all:
> 
> 1. Get list of devices
> 2. Reset test server allocation
> 3. Create runners and shard tests between them
> 4. report results (java tests now do this in test_runner.py)
> 
> The common code should be pulled out into test_runner.py

Agreed. Should I file a bug for this perhaps?

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:41: Exception: Test suite not found.
On 2013/07/03 21:26:37, frankf wrote:
> Exception if test ...

Done.

https://codereview.chromium.org/18323020/diff/1/build/android/test_runner.py
File build/android/test_runner.py (left):

https://codereview.chromium.org/18323020/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:119:
option_parser.add_option('--buildbot-step-failure',
On 2013/07/03 21:26:37, frankf wrote:
> I think this is related to this CL, should we address it here?

I wasn't sure if we wanted to leave more time before removing it, but looking
through the source, it looks like there's no use of it in the buildbot scripts
(unless I'm missing something?), and the PrintStepResultIfNeeded function which
uses it isn't called anywhere either.

[gkanwar, 2013-07-04 00:25:33]

On 2013/07/03 23:15:18, gkanwar wrote:
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
> File build/android/pylib/base/shard.py (left):
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
> build/android/pylib/base/shard.py:157: test_collection.add(test)
> On 2013/07/03 21:26:37, frankf wrote:
> > This decreases robustness. Currently, if we catch this exception, we add the
> > tests back to queue to get retried. 
> 
> I'm not sure I fully understand -- is this decreasing robustness by not adding
> the tests back to the queue to be retried? The except clause below should
catch
> this exception and do that. The only change is whether we re-raise or silently
> ignore the exception.
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
> File build/android/pylib/base/shard.py (right):
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
> build/android/pylib/base/shard.py:217: run_results.exit_code =
> constants.WARNING_EXIT_CODE
> On 2013/07/03 21:26:37, frankf wrote:
> > Perhaps a singleton object that flags this exception happened is cleaner in
> this
> > case, than propagating the exit_code through the results object. If we're
> > returning a warning code, we should also do logging.error() of which device
is
> > unresponsive.
> 
> That's a good point. Updated to use a singleton instead.
> 
> Logging added when we set this exit code here.
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/browserte...
> File build/android/pylib/browsertests/dispatch.py (right):
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/browserte...
> build/android/pylib/browsertests/dispatch.py:25: def Dispatch(options):
> On 2013/07/03 21:26:37, frankf wrote:
> > Future TODO: There's a lot of duplicaiton in dispatch modules. Currently
they
> > all:
> > 
> > 1. Get list of devices
> > 2. Reset test server allocation
> > 3. Create runners and shard tests between them
> > 4. report results (java tests now do this in test_runner.py)
> > 
> > The common code should be pulled out into test_runner.py
> 
> Agreed. Should I file a bug for this perhaps?
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
> File build/android/pylib/gtest/dispatch.py (right):
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/pylib/gtest/dis...
> build/android/pylib/gtest/dispatch.py:41: Exception: Test suite not found.
> On 2013/07/03 21:26:37, frankf wrote:
> > Exception if test ...
> 
> Done.
> 
> https://codereview.chromium.org/18323020/diff/1/build/android/test_runner.py
> File build/android/test_runner.py (left):
> 
>
https://codereview.chromium.org/18323020/diff/1/build/android/test_runner.py#...
> build/android/test_runner.py:119:
> option_parser.add_option('--buildbot-step-failure',
> On 2013/07/03 21:26:37, frankf wrote:
> > I think this is related to this CL, should we address it here?
> 
> I wasn't sure if we wanted to leave more time before removing it, but looking
> through the source, it looks like there's no use of it in the buildbot scripts
> (unless I'm missing something?), and the PrintStepResultIfNeeded function
which
> uses it isn't called anywhere either.

Updated the code to have the sharder manage exit codes.

[frankf, 2013-07-04 00:46:19]

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (left):

https://codereview.chromium.org/18323020/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:157: test_collection.add(test)
Ah, it makes sense.

On 2013/07/03 23:15:18, gkanwar wrote:
> On 2013/07/03 21:26:37, frankf wrote:
> > This decreases robustness. Currently, if we catch this exception, we add the
> > tests back to queue to get retried. 
> 
> I'm not sure I fully understand -- is this decreasing robustness by not adding
> the tests back to the queue to be retried? The except clause below should
catch
> this exception and do that. The only change is whether we re-raise or silently
> ignore the exception.

https://codereview.chromium.org/18323020/diff/7001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/7001/build/android/test_runner....
build/android/test_runner.py:413: returned_exit_code = exit_code.GetExitCode()
Why not just return this on line 417?

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/b...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/b...
build/android/pylib/base/base_test_result.py:7: from pylib import constants
Remove this

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:203: exit_code = 0
you mean NORMAL?

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:219: exit_code = exit_code or
constants.WARNING_EXIT_CODE
why not just
exit_code = constants.WARNING_EXIT_CODE

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:223: if len(run_results.GetNotPass()):
not DidRunPass()

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:293: return
(base_test_result.TestRunResults(), 0)
I think it makes sense to change this to

logging.error(...)
return (..., constants.ERROR_EXIT_CODE)

Since if we don't want to pass if there are no tests.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/buildb...
File build/android/pylib/buildbot_report.py (left):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/buildb...
build/android/pylib/buildbot_report.py:49: def PrintStepResultIfNeeded(options,
result):
Actually the calls to this were removed here:

https://codereview.chromium.org/15942016/diff/109001/build/android/run_instru...

Although I don't see --buildbot_step_failure being passed anywhere.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:234: if test_exit_code and exit_code !=
constants.ERROR_EXIT_CODE:
the warning would get overwritten here right?

https://codereview.chromium.org/18323020/diff/3002/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/test_runner....
build/android/test_runner.py:391: if test_exit_code and exit_code !=
constants.ERROR_EXIT_CODE:
Same here. You're throwing out the warning.

[frankf, 2013-07-04 01:22:35]

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/buildb...
File build/android/pylib/buildbot_report.py (left):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/buildb...
build/android/pylib/buildbot_report.py:49: def PrintStepResultIfNeeded(options,
result):
On second look, we use --buildbot-step-failure downstream. I'll submit a patch
to fix the logic there.

On 2013/07/04 00:46:19, frankf wrote:
> Actually the calls to this were removed here:
> 
>
https://codereview.chromium.org/15942016/diff/109001/build/android/run_instru...
> 
> Although I don't see --buildbot_step_failure being passed anywhere.

[gkanwar, 2013-07-08 18:50:24]

https://codereview.chromium.org/18323020/diff/7001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/7001/build/android/test_runner....
build/android/test_runner.py:413: returned_exit_code = exit_code.GetExitCode()
On 2013/07/04 00:46:19, frankf wrote:
> Why not just return this on line 417?

In the updated code we get the exit_code before printing the logs. The gtest and
content_browsertest exit_codes don't return immediately for uniformity.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/b...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/b...
build/android/pylib/base/base_test_result.py:7: from pylib import constants
On 2013/07/04 00:46:19, frankf wrote:
> Remove this

Done.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:203: exit_code = 0
On 2013/07/04 00:46:19, frankf wrote:
> you mean NORMAL?

Done.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:219: exit_code = exit_code or
constants.WARNING_EXIT_CODE
On 2013/07/04 00:46:19, frankf wrote:
> why not just
> exit_code = constants.WARNING_EXIT_CODE

Done.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:223: if len(run_results.GetNotPass()):
On 2013/07/04 00:46:19, frankf wrote:
> not DidRunPass()

Done.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:293: return
(base_test_result.TestRunResults(), 0)
On 2013/07/04 00:46:19, frankf wrote:
> I think it makes sense to change this to
> 
> logging.error(...)
> return (..., constants.ERROR_EXIT_CODE)
> 
> Since if we don't want to pass if there are no tests.

That make sense. Done.

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:234: if test_exit_code and exit_code !=
constants.ERROR_EXIT_CODE:
On 2013/07/04 00:46:19, frankf wrote:
> the warning would get overwritten here right?

If exit_code is warning or normal, and test_exit_code is warning or error it
should overwrite it (this is just allowing only exit code escalation).

https://codereview.chromium.org/18323020/diff/3002/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/3002/build/android/test_runner....
build/android/test_runner.py:391: if test_exit_code and exit_code !=
constants.ERROR_EXIT_CODE:
On 2013/07/04 00:46:19, frankf wrote:
> Same here. You're throwing out the warning.

See my other comment.

[frankf, 2013-07-08 19:13:43]

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/build...
File build/android/pylib/buildbot_report.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/build...
build/android/pylib/buildbot_report.py:5: """Helper functions to print buildbot
messages."""
Let's move this to build/android/buildbot. This could be a follow up CL if there
are many dependencies on this.

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:129: Exception: If device not attached, or
failed to reset test server port.
Not sure if this is the whole truth. Many of the function calls below can also
throw exceptions.

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:227: exit_code = 0
Hmm. Actually, I'm not sure having a NORMAL exit code constant adds any value.
This is less verbose.

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/test_runner.py:25: 'base_unittests':
'base/base_unittests.isolate',
Let's not touch the style in this file. I have a pending CL that heavily
modifies this.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:367: Exception: Unknown command name passed in.
Same here, is this the only exception that can be thrown.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:386: exit_code = test_exit_code
No need for the intermediate variable

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:407: exit_code = test_exit_code
Same here.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:519: exit_code =
VALID_COMMANDS[command].run_command_func(
Just return inline.

[frankf, 2013-07-08 19:23:28]

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:189:
report_results.PrintAnnotation(test_results)
Also, we don't need to call this anymore. I believe, in the past, run_tests.py
was responsible for creating buildbot steps (There's an unsued step_name var in
this file).

[gkanwar, 2013-07-08 21:17:07]

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/build...
File build/android/pylib/buildbot_report.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/build...
build/android/pylib/buildbot_report.py:5: """Helper functions to print buildbot
messages."""
On 2013/07/08 19:13:43, frankf wrote:
> Let's move this to build/android/buildbot. This could be a follow up CL if
there
> are many dependencies on this.

I'm not sure it actually makes sense to do this. These functions are relevant
only to files within pylib, whereas files in the buildbot folder are generally
executable scripts used by buildbot. Currently the buildbot folder isn't even a
Python module for this reason (I think).

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:129: Exception: If device not attached, or
failed to reset test server port.
On 2013/07/08 19:13:43, frankf wrote:
> Not sure if this is the whole truth. Many of the function calls below can also
> throw exceptions.

Hmm, I didn't realize that. Slightly rephrased.

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:189:
report_results.PrintAnnotation(test_results)
On 2013/07/08 19:23:29, frankf wrote:
> Also, we don't need to call this anymore. I believe, in the past, run_tests.py
> was responsible for creating buildbot steps (There's an unsued step_name var
in
> this file). 

Removed. Also removed step_name.

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:227: exit_code = 0
On 2013/07/08 19:13:43, frankf wrote:
> Hmm. Actually, I'm not sure having a NORMAL exit code constant adds any value.
> This is less verbose.

Fair enough... I could see an argument for both ways. I added it as a constant
for completeness, but leaving it as zero is a bit cleaner, and there's also a
lot of exit code 0's that should get changed if we're doing with
NORMAL_EXIT_CODE as a constant. Removed.

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/pylib/gtest...
build/android/pylib/gtest/test_runner.py:25: 'base_unittests':
'base/base_unittests.isolate',
On 2013/07/08 19:13:43, frankf wrote:
> Let's not touch the style in this file. I have a pending CL that heavily
> modifies this.

Done.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:367: Exception: Unknown command name passed in.
On 2013/07/08 19:13:43, frankf wrote:
> Same here, is this the only exception that can be thrown.

Updated docstring.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:386: exit_code = test_exit_code
On 2013/07/08 19:13:43, frankf wrote:
> No need for the intermediate variable

Done.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:407: exit_code = test_exit_code
On 2013/07/08 19:13:43, frankf wrote:
> Same here.

Done.

https://codereview.chromium.org/18323020/diff/20001/build/android/test_runner...
build/android/test_runner.py:519: exit_code =
VALID_COMMANDS[command].run_command_func(
On 2013/07/08 19:13:43, frankf wrote:
> Just return inline.

Done.

[frankf, 2013-07-08 23:59:12]

lgtm with nits.

So, as follow up to this, we need to change the upstream buildbot scripts to
understand the warning return code.

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:96:
report_results.PrintAnnotation(test_results)
Remove this.

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:129: Exception: For various reasons
including device failure or failing to reset
Not sure if this adds any value.

[gkanwar, 2013-07-09 00:06:10]

On 2013/07/08 23:59:12, frankf wrote:
> lgtm with nits.
> 
> So, as follow up to this, we need to change the upstream buildbot scripts to
> understand the warning return code.
I think the upstream bots already interpret the warning exit code correctly (see
RunCmd in bb_utils.py).
> 
>
https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/brows...
> File build/android/pylib/browsertests/dispatch.py (right):
> 
>
https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/brows...
> build/android/pylib/browsertests/dispatch.py:96:
> report_results.PrintAnnotation(test_results)
> Remove this.
> 
>
https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/gtest...
> File build/android/pylib/gtest/dispatch.py (right):
> 
>
https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/gtest...
> build/android/pylib/gtest/dispatch.py:129: Exception: For various reasons
> including device failure or failing to reset
> Not sure if this adds any value.

[gkanwar, 2013-07-09 00:06:29]

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:96:
report_results.PrintAnnotation(test_results)
On 2013/07/08 23:59:12, frankf wrote:
> Remove this.

Done.

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18323020/diff/30001/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:129: Exception: For various reasons
including device failure or failing to reset
On 2013/07/08 23:59:12, frankf wrote:
> Not sure if this adds any value.

I still feel like having a Raises section is useful, even if the causes for
Exceptions are really varied. It's nice to know that this function will be
throwing Exceptions.

[gkanwar, 2013-07-09 00:28:52]

Isaac -- Frank suggested that I copy buildbot_report.py into the buildbot
directory (instead of moving) as a temporary measure while we update the
downstream scripts, at which point we can remove the old copy of
buildbot_report.py. Could you review this change please?

Thanks!

[frankf, 2013-07-09 00:37:56]

https://codereview.chromium.org/18323020/diff/25017/build/android/buildbot/bb...
File build/android/buildbot/bb_utils.py (right):

https://codereview.chromium.org/18323020/diff/25017/build/android/buildbot/bb...
build/android/buildbot/bb_utils.py:12: import buildbot_report
I think the convention is bb_*. It might makes sense to have this in bb_utils. I
defer to ilevy.

https://codereview.chromium.org/18323020/diff/25017/build/android/buildbot/bb...
build/android/buildbot/bb_utils.py:53: if code != constants.WARNING_EXIT_CODE
and flunk_on_failure:
You're assuming every arbitrary command uses our warning convention. Rather, you
should have constants.WARNING_EXIT_CODE as default parameter like before.

[gkanwar, 2013-07-09 00:47:42]

https://codereview.chromium.org/18323020/diff/25017/build/android/buildbot/bb...
File build/android/buildbot/bb_utils.py (right):

https://codereview.chromium.org/18323020/diff/25017/build/android/buildbot/bb...
build/android/buildbot/bb_utils.py:53: if code != constants.WARNING_EXIT_CODE
and flunk_on_failure:
On 2013/07/09 00:37:56, frankf wrote:
> You're assuming every arbitrary command uses our warning convention. Rather,
you
> should have constants.WARNING_EXIT_CODE as default parameter like before.

Ah my mistake. Switched back to a default parameter.

[Isaac (away), 2013-07-09 00:52:49]

lgtm

[gkanwar, 2013-07-09 00:54:40]

On 2013/07/09 00:52:49, Isaac wrote:
> lgtm

Isaac -- did you see Frank's comment about naming the new file? Should it become
bb_*, or maybe be moved into bb_utils.py?

[Isaac (away), 2013-07-09 01:18:00]

On 2013/07/09 00:54:40, gkanwar wrote:
> On 2013/07/09 00:52:49, Isaac wrote:
> > lgtm
> 
> Isaac -- did you see Frank's comment about naming the new file? Should it
become
> bb_*, or maybe be moved into bb_utils.py?

Let's keep separate for now -- you can name bb_annotations.py.

[gkanwar, 2013-07-09 01:20:57]

On 2013/07/09 01:18:00, Isaac wrote:
> On 2013/07/09 00:54:40, gkanwar wrote:
> > On 2013/07/09 00:52:49, Isaac wrote:
> > > lgtm
> > 
> > Isaac -- did you see Frank's comment about naming the new file? Should it
> become
> > bb_*, or maybe be moved into bb_utils.py?
> 
> Let's keep separate for now -- you can name bb_annotations.py.

Okay, renamed. Just waiting for the test_runner.py patch to reland, so I can
rebase, test, and CQ this.

[gkanwar, 2013-07-09 17:12:28]

On 2013/07/09 01:20:57, gkanwar wrote:
> On 2013/07/09 01:18:00, Isaac wrote:
> > On 2013/07/09 00:54:40, gkanwar wrote:
> > > On 2013/07/09 00:52:49, Isaac wrote:
> > > > lgtm
> > > 
> > > Isaac -- did you see Frank's comment about naming the new file? Should it
> > become
> > > bb_*, or maybe be moved into bb_utils.py?
> > 
> > Let's keep separate for now -- you can name bb_annotations.py.
> 
> Okay, renamed. Just waiting for the test_runner.py patch to reland, so I can
> rebase, test, and CQ this.

Rebased onto master now that the test_runner.py change landed. Hopefully the try
bots can apply the patch cleanly. Just waiting on try bots now.

[gkanwar, 2013-07-09 20:34:32]

On 2013/07/09 17:12:28, gkanwar wrote:
> On 2013/07/09 01:20:57, gkanwar wrote:
> > On 2013/07/09 01:18:00, Isaac wrote:
> > > On 2013/07/09 00:54:40, gkanwar wrote:
> > > > On 2013/07/09 00:52:49, Isaac wrote:
> > > > > lgtm
> > > > 
> > > > Isaac -- did you see Frank's comment about naming the new file? Should
it
> > > become
> > > > bb_*, or maybe be moved into bb_utils.py?
> > > 
> > > Let's keep separate for now -- you can name bb_annotations.py.
> > 
> > Okay, renamed. Just waiting for the test_runner.py patch to reland, so I can
> > rebase, test, and CQ this.
> 
> Rebased onto master now that the test_runner.py change landed. Hopefully the
try
> bots can apply the patch cleanly. Just waiting on try bots now.

Try bots look good. CQing.

[commit-bot: I haz the power, 2013-07-09 20:34:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@google.com/18323020/55018

[commit-bot: I haz the power, 2013-07-10 04:57:11]

Change committed as 210749