Add time and state to builder revision data

Add time and state to builder revision data

This patch is intended to make it possible to add alerts to sheriff-o-matic
for offline and hung bots. Prevoiusly, we'd only output the latest
revision information for each bot. This patch also adds a state
(offline, idle, or running) and last update time (last completed run
if offline, or last started step if running).

Committed: https://chromium.googlesource.com/infra/infra/+/afc42290655d4fe804e3466a6875f1111d339025

[leviw_travelin_and_unemployed, 2014-08-14 22:35:59]

This pushes the builder revisions down into another level, so it breaks all
consumers of this JSON. Is sheriff-o-matic the only consumer?

Also I'd love some advice on how to test this.

[ojan, 2014-08-14 22:47:19]

eseidel should give this a quick look to make sure it fits in his mental model
of how all this should be structured.

The code you wrote in sheriff-o-matic is the only consumer, so it's fine to
break it.

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
File infra/tools/builder_alerts/buildbot.py (right):

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
infra/tools/builder_alerts/buildbot.py:181: def
latest_update_time_for_builder(state, last_build):
Look at alert_builder_tests.test_find_current_step_failures_in_progress as an
example. Just create some build-like object and pass those in as last_build.

Can chat in person if you are stuck.

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
infra/tools/builder_alerts/buildbot.py:183: if state == 'offline':
We should get the same time whether the bot is offline or not.

if times[1] != None:
  last_update = float(last_build['times'][1])
else:
  for step in last_build['steps']:
    ...

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
infra/tools/builder_alerts/buildbot.py:193: def
latest_builder_info_for_master(cache, master_url, master_json):  # pragma: no
cover
If you'd be willing to write tests for this while you're at it, that'd be much
appreciated.

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
infra/tools/builder_alerts/buildbot.py:203: # otherwise iterate across steps to
find the start time of the running one
Leftover comment?

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
infra/tools/builder_alerts/buildbot.py:206:
latest_builder_info[master_name][builder_name] = collections.defaultdict(dict)
No need to defaultdict here.

latest_builder_info[master_name][builder_name] = {
  'revisions': revisions_from_build(last_build),
  'state': state,
  'lastUpdateTime': latest_update_time_for_builder(state, last_build),
}

https://codereview.chromium.org/475943002/diff/1/infra/tools/builder_alerts/b...
infra/tools/builder_alerts/buildbot.py:207:
latest_builder_info[master_name][builder_name]['revisions'] = \
The old code had the wrong style. multi-lines should be done with parens.

[eseidel, 2014-08-14 23:01:26]

I don't think the latest builder info really needs to be part of builder_alerts
at all. :)

But as to extending the existing code, this is fine.

[eseidel, 2014-08-14 23:02:10]

Please add something resembling a test. :)

python test.py at the root of the infra repo can be used to run tests.  It has
various options and will show you coverage information.

git cl presubmit will also run the tests, but in a less friendly way.

[leviw_travelin_and_unemployed, 2014-08-14 23:48:22]

On 2014/08/14 at 23:02:10, eseidel wrote:
> Please add something resembling a test. :)
> 
> python test.py at the root of the infra repo can be used to run tests.  It has
various options and will show you coverage information.
> 
> git cl presubmit will also run the tests, but in a less friendly way.

Don't worry, I was planning on it, hence my request for advice on how to do so
;)

[leviw_travelin_and_unemployed, 2014-08-18 16:28:16]

ptal

[eseidel, 2014-08-18 16:35:43]

please test the None/0 case, but otherwise lgtm

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
File infra/tools/builder_alerts/buildbot.py (right):

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
infra/tools/builder_alerts/buildbot.py:182: last_update = float(0)
None?

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
infra/tools/builder_alerts/buildbot.py:206: 'lastUpdateTime':
latest_update_time_for_builder(last_build),
What does 0 mean here?

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
File infra/tools/builder_alerts/test/buildbot_test.py (right):

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
infra/tools/builder_alerts/test/buildbot_test.py:8: import pdb
?

[leviw_travelin_and_unemployed, 2014-08-18 18:39:03]

TY!

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
File infra/tools/builder_alerts/buildbot.py (right):

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
infra/tools/builder_alerts/buildbot.py:182: last_update = float(0)
On 2014/08/18 16:35:43, eseidel wrote:
> None?

Sure!

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
infra/tools/builder_alerts/buildbot.py:206: 'lastUpdateTime':
latest_update_time_for_builder(last_build),
On 2014/08/18 16:35:43, eseidel wrote:
> What does 0 mean here?

It should never actually be zero. It should be None.

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
File infra/tools/builder_alerts/test/buildbot_test.py (right):

https://codereview.chromium.org/475943002/diff/20001/infra/tools/builder_aler...
infra/tools/builder_alerts/test/buildbot_test.py:8: import pdb
On 2014/08/18 16:35:43, eseidel wrote:
> ?

Whoops!

[leviw_travelin_and_unemployed, 2014-08-18 18:50:24]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-18 18:51:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/475943002/40001

[commit-bot: I haz the power, 2014-08-18 18:51:12]

Committed patchset #3 (40001) as afc42290655d4fe804e3466a6875f1111d339025