Merge lp:~abentley/ci-director/building-trumps-result into lp:ci-director

[Aaron Bentley]

Results are not final until building is false.

This branch changes build results to be non-final until building is done.

There are two problems revealed by the s390x issue
- Premature uploads of a build / repeated uploads of a build.
- Premature triggering of dependant jobs.

The first issue is fixed by using 'building' instead of 'result' to determine whether to upload.

The second issue is fixed in OutcomeJob.from_json, by using 'building' to override 'result' for an indivdual build.

[Curtis Hovey (sinzui)]

thank you.

review: Approve (code)

=== modified file 'cidirector/start_builds.py'
--- cidirector/start_builds.py	2016-08-29 15:46:53 +0000
+++ cidirector/start_builds.py	2016-08-29 19:15:51 +0000
@@ -174,9 +174,9 @@
             for job, job_data in sorted(jobs.items()):
                 artifact_job = ArtifactJob(job, logger)
                 for build_info in job_data['builds'].values():
-                    if build_info['result'] is None:
-                        logger.info(
-                            'No result for {} #{}.  Skipping.'.format(
+                    if build_info['building']:
+                        logger.debug(
+                            'Still building {} #{}.  Skipping.'.format(
                                 job, build_info['number']))
                         continue
                     logger.info(

=== modified file 'cidirector/tests/test_start_builds.py'
--- cidirector/tests/test_start_builds.py	2016-08-02 19:39:07 +0000
+++ cidirector/tests/test_start_builds.py	2016-08-29 19:15:51 +0000
@@ -209,9 +209,9 @@
 
     @staticmethod
     def make_by_revision_build():
-        build_110 = {'result': 'bar', 'number': 110}
-        build_111 = {'result': 'qux', 'number': 111}
-        build_120 = {'result': 'baz', 'number': 120}
+        build_110 = {'building': False, 'result': 'bar', 'number': 110}
+        build_111 = {'building': False, 'result': 'qux', 'number': 111}
+        build_120 = {'building': False, 'result': 'baz', 'number': 120}
         return OrderedDict([
             ('7081', {'foo': {'builds': {120: build_120}}}),
             ('7021', {'bar': {'builds': OrderedDict([
@@ -313,9 +313,10 @@
                  '7021', sb.storage),
             ], aj_instance_mock.archive_results.mock_calls)
 
-    def test_upload_artifacts_null_result(self):
+    def test_upload_artifacts_still_building(self):
         by_revision = self.make_by_revision_build()
-        by_revision['7021']['bar']['builds'][111]['result'] = None
+        by_revision['7081']['foo']['builds'][120]['building'] = True
+        by_revision['7021']['bar']['builds'][110]['building'] = True
         aj_instance_mock = Mock(spec=['archive_results'])
         sb = self.make_start_builds()
         with self.artifact_job_cxt(aj_instance_mock) as aj_mock:
@@ -325,9 +326,7 @@
             call('bar', logging.getLogger()),
             ], aj_mock.call_args_list)
         self.assertEqual([
-            call(sb.jenkins, by_revision['7081']['foo']['builds'][120],
-                 '7081', sb.storage),
-            call(sb.jenkins, by_revision['7021']['bar']['builds'][110],
+            call(sb.jenkins, by_revision['7021']['bar']['builds'][111],
                  '7021', sb.storage),
             ], aj_instance_mock.archive_results.mock_calls)
 

=== modified file 'cidirector/tests/test_update_outcome.py'
--- cidirector/tests/test_update_outcome.py	2016-08-03 14:48:57 +0000
+++ cidirector/tests/test_update_outcome.py	2016-08-29 19:15:51 +0000
@@ -539,13 +539,32 @@
         job = OutcomeJob.from_json('foo', {
             'config': {},
             'builds': {'1': {
-                'building': True,
+                'building': False,
                 'number': 1,
                 'result': SUCCESS,
                 }},
             }, True)
         self.assertEqual(SUCCEEDED, job.status)
 
+    def test_from_json_building_succeeded(self):
+        job = OutcomeJob.from_json('foo', {
+            'config': {},
+            'builds': {
+                '1': {'building': True, 'number': 1, 'result': SUCCESS}
+                },
+            }, True)
+        self.assertEqual(BUILDING, job.status)
+
+    def test_from_json_building_after_succeeded(self):
+        job = OutcomeJob.from_json('foo', {
+            'config': {},
+            'builds': {
+                '1': {'building': False, 'number': 1, 'result': SUCCESS},
+                '2': {'building': True, 'number': 1, 'result': SUCCESS},
+                },
+            }, True)
+        self.assertEqual(SUCCEEDED, job.status)
+
     def test_from_json_failed(self):
         job = OutcomeJob.from_json('foo', {
             'config': {},

=== modified file 'cidirector/update_outcome.py'
--- cidirector/update_outcome.py	2016-08-29 15:46:53 +0000
+++ cidirector/update_outcome.py	2016-08-29 19:15:51 +0000
@@ -267,12 +267,19 @@
         conflicts = set(config.get('conflicts', '').split())
         for build in json['builds'].values():
             build_numbers.append(build['number'])
+            # A build may get a result before building is done.  i.e.
+            # archiving artifacts happens while a result is set, but before
+            # building is complete.  Presumably a failure archiving artifacts
+            # would change the result, so it's not a final result until
+            # building is False.
             if build['building']:
                 building = True
-            if build.get('result') == SUCCESS:
+            elif build.get('result') == SUCCESS:
                 succeeded = True
-            if build.get('result') == FAILURE:
+            elif build.get('result') == FAILURE:
                 failure_count += 1
+        # A final success overrides BUILDING; once you have succeeded, future
+        # builds can't change that to BUILDING status.
         if succeeded:
             status = SUCCEEDED
         elif building: