Checking for failure in requested builds.

scripts/slave/recipe_modules/auto_bisect/revision_state.py

Index: scripts/slave/recipe_modules/auto_bisect/revision_state.py
diff --git a/scripts/slave/recipe_modules/auto_bisect/revision_state.py b/scripts/slave/recipe_modules/auto_bisect/revision_state.py
index 1f9e244eec513bf380c294c334e5dfe70cdac6d4..bf15d00f0bbc77954473a2f8a3a5cddf23af8033 100644
--- a/scripts/slave/recipe_modules/auto_bisect/revision_state.py
+++ b/scripts/slave/recipe_modules/auto_bisect/revision_state.py
@@ -91,6 +91,7 @@ class RevisionState(object):
     self.std_dev = None
     self.repeat_count = MINIMUM_SAMPLE_SIZE
     self._test_config = None
+    self.build_number = None
 
   @property
   def tested(self):
@@ -219,8 +220,11 @@ class RevisionState(object):
     To wait for the test we try to get the buildbot job url from GS, and if
     available, we query the status of such job.
     """
-    if self.status == RevisionState.BUILDING and self._is_build_archived():
-      self.start_job()
+    if self.status == RevisionState.BUILDING:
+      if self._is_build_archived():
+        self.start_job()
+      elif self._is_build_failed():
+        self.status = RevisionState.FAILED
     elif (self.status in (RevisionState.TESTING, RevisionState.NEED_MORE_DATA)
           and self._results_available()):
       # If we have already decided whether the revision is good or bad we
@@ -244,6 +248,51 @@ class RevisionState(object):
 
     return self.build_archived
 
+  def _is_build_failed(self):
+    api = self.bisector.api
+    current_build = None
+    base_url = '%sjson/builders/%s'% (
+        os.environ.get('BUILDBOT_URL', 'http://localhost:8041/'),

[qyearsley, 2016/03/04 22:06:28]

Why http://localhost:8041/?

[RobertoCN, 2016/03/14 01:00:25]

The reasonable assumption if this env variable is not set, is that this is a
workstation running a local master. I added a comment to that effect.

+        self.bisector.get_builder_bot_for_this_platform())
+    if self.build_number is None:

[qyearsley, 2016/03/04 22:06:28]

Does this mean no build started yet, or we're not sure what the build number is
yet?

[RobertoCN, 2016/03/14 01:00:25]

It means we don't know the build number yet. The build number is not assigned
until the build is started however.

+      try:
+        builder_state_url = base_url + '?as_text=1'
+        fetch_step = api.m.url.fetch_to_file(
+            builder_state_url, None,
+            step_name='fetch builder state',
+            stdout=api.m.raw_io.output())
+        builder_state = fetch_step.stdout
+        builder_state = json.loads(builder_state or '{}')
+        for build_number in builder_state.get('cachedBuilds', []):
+          build_url = '%s/builds/%s?as_text=1' % (base_url, build_number)
+          fetch_result = api.m.url.fetch_to_file(
+              build_url, None,
+              step_name='fetch build details',
+              stdout=api.m.raw_io.output())
+          build = json.loads(fetch_result.stdout or '{}')
+          for p in build.get('properties', []):
+            if p[0] == 'build_archive_url' and p[1] == self.build_url:
+              current_build = build
+              self.build_number = build_number
+              break

[qyearsley, 2016/03/04 22:06:28]

Is build.get('properties') expected to be a list of 2-element lists? e.g.
[
 ['k', 'v'],
 ['build_archive_url', 'http://example.com/'],
]

If so, you can convert it to a dict, e.g.
{'k', 'v2'}

So, this could be changed to something like:

build_properties = dict(build.get('properties', []))
if build_properties.get('build_archive_url') == self.build_url:
  current_build = build
  self.build_number = build_number

[RobertoCN, 2016/03/14 01:00:25]

Thanks! I don't know how I didn't see it.
Done.

+          if self.build_number:
+            break
+      except api.m.step.StepFailure:  # pragma: no cover
+        # If we cannot get json from buildbot, we cannot determine if a build is
+        # failed, hence we consider it in progress until it times out.
+        return False
+    if self.build_number is None:
+      # The build hasn't started yet, therefore it's not failed.
+      return False
+    if not current_build:
+      build_url = '%s/builds/%s?as_text=1' % (base_url, self.build_number)
+      fetch_result = api.m.url.fetch_to_file(
+          build_url, None,
+          step_name='fetch build details',
+          stdout=api.m.raw_io.output())
+      current_build = json.loads(fetch_result.stdout or '{}')

[qyearsley, 2016/03/04 22:06:28]

This section of code (where the URL is put together, and a request for build
details JSON is made and then that JSON is parsed) is repeated -- it should be
extracted to a separate method (which takes a base URL and build number, and
returns the parsed data).

[RobertoCN, 2016/03/14 01:00:25]

Done.

+    return current_build.get('results') in [2, 3, 4]

[qyearsley, 2016/03/04 22:06:28]

These could be set as constants, ideally with some reference as to where they
come from. e.g.

# Buildbot job result codes.
# See http://docs.buildbot.net/current/developer/results.html
SUCCESS, WARNINGS, FAILURE, SKIPPED, EXCEPTION = range(5)

I'm not sure if that's correct though.

[RobertoCN, 2016/03/14 01:00:25]

Works for me.

+
   def _results_available(self):
     """Checks if the results for the test job have been uploaded."""
     api = self.bisector.api
@@ -317,7 +366,7 @@ class RevisionState(object):
     # TODO: Rewrite using the trigger module.
     api = self.bisector.api
     bot_name = self.bisector.get_builder_bot_for_this_platform()
-    if self.bisector.dummy_builds:
+    if self.bisector.bisect_config.get('dummy_job_names'):
       self.job_name = self.commit_hash + '-build'
     else:  # pragma: no cover
       self.job_name = uuid.uuid4().hex
@@ -383,7 +432,7 @@ class RevisionState(object):
     the test will be run on the same machine. Otherwise, this posts
     a request to buildbot to download and perf-test this build.
     """
-    if self.bisector.dummy_builds:
+    if self.bisector.bisect_config.get('dummy_job_names'):
       self.job_name = self.commit_hash + '-test'
     else:  # pragma: no cover
       self.job_name = uuid.uuid4().hex