launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/recipe-build-missing-dep-email into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #732343 in Launchpad itself: "Recipe source builds failing because of "Could not build because of missing dependencies" do not send notification email"
  https://bugs.launchpad.net/launchpad/+bug/732343

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/recipe-build-missing-dep-email/+merge/61058

Ensure email is sent when a spr build fails due to a package dependency problem.

== Implementation ==

The twisted _handleStatus_DEPFAIL callback in packagebuild.py wasn't calling notify()
Also did some drive by lint fixes.

== Tests ==

Added new tests to test_packagebuild.py to ensure notify is called when a build fails. There are 3 failure modes requiring emails to be sent and there were no existing tests to ensure notify was invoked. There were already tests however to test that if notify() is invoked, it does the correct thing (test_build_notify.py)

test_handleStatus_DEPFAIL_notifies()
test_handleStatus_CHROOTFAIL_notifies()
test_handleStatus_PACKAGEFAIL_notifies()

== Launchpad lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/buildmaster/model/packagebuild.py
  lib/lp/buildmaster/tests/test_packagebuild.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/recipe-build-missing-dep-email/+merge/61058
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/recipe-build-missing-dep-email into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2011-05-04 06:37:50 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2011-05-16 03:43:27 +0000
@@ -298,7 +298,7 @@
         # Release the builder for another job.
         d = self.buildqueue_record.builder.cleanSlave()
         # Remove BuildQueue record.
-        return d.addCallback(lambda x:self.buildqueue_record.destroySelf())
+        return d.addCallback(lambda x: self.buildqueue_record.destroySelf())
 
     def _handleStatus_OK(self, librarian, slave_status, logger):
         """Handle a package that built successfully.
@@ -367,8 +367,8 @@
             # files from the slave.
             if successful_copy_from_slave:
                 logger.info(
-                    "Gathered %s %d completely. Moving %s to uploader queue." % (
-                    self.__class__.__name__, self.id, upload_leaf))
+                    "Gathered %s %d completely. Moving %s to uploader queue."
+                    % (self.__class__.__name__, self.id, upload_leaf))
                 target_dir = os.path.join(root, "incoming")
                 self.status = BuildStatus.UPLOADING
             else:
@@ -384,8 +384,8 @@
             # Release the builder for another job.
             d = self._release_builder_and_remove_queue_item()
 
-            # Commit so there are no race conditions with archiveuploader about
-            # self.status.
+            # Commit so there are no race conditions with archiveuploader
+            # about self.status.
             Store.of(self).commit()
 
             # Move the directory used to grab the binaries into
@@ -411,11 +411,12 @@
         remove Buildqueue entry.
         """
         self.status = BuildStatus.FAILEDTOBUILD
+
         def build_info_stored(ignored):
             self.notify()
             d = self.buildqueue_record.builder.cleanSlave()
             return d.addCallback(
-                lambda x:self.buildqueue_record.destroySelf())
+                lambda x: self.buildqueue_record.destroySelf())
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -428,12 +429,14 @@
         entry and release builder slave for another job.
         """
         self.status = BuildStatus.MANUALDEPWAIT
+
         def build_info_stored(ignored):
             logger.critical("***** %s is MANUALDEPWAIT *****"
                             % self.buildqueue_record.builder.name)
+            self.notify()
             d = self.buildqueue_record.builder.cleanSlave()
             return d.addCallback(
-                lambda x:self.buildqueue_record.destroySelf())
+                lambda x: self.buildqueue_record.destroySelf())
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -446,13 +449,14 @@
         and release the builder.
         """
         self.status = BuildStatus.CHROOTWAIT
+
         def build_info_stored(ignored):
             logger.critical("***** %s is CHROOTWAIT *****" %
                             self.buildqueue_record.builder.name)
             self.notify()
             d = self.buildqueue_record.builder.cleanSlave()
             return d.addCallback(
-                lambda x:self.buildqueue_record.destroySelf())
+                lambda x: self.buildqueue_record.destroySelf())
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -468,6 +472,7 @@
                        % self.buildqueue_record.builder.name)
         self.buildqueue_record.builder.failBuilder(
             "Builder returned BUILDERFAIL when asked for its status")
+
         def build_info_stored(ignored):
             # simply reset job
             self.buildqueue_record.reset()
@@ -484,6 +489,7 @@
         logger.warning("***** %s is GIVENBACK by %s *****"
                        % (self.buildqueue_record.specific_job.build.title,
                           self.buildqueue_record.builder.name))
+
         def build_info_stored(ignored):
             # XXX cprov 2006-05-30: Currently this information is not
             # properly presented in the Web UI. We will discuss it in

=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2011-05-05 02:39:26 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2011-05-16 03:43:27 +0000
@@ -46,6 +46,7 @@
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
+from lp.testing.mail_helpers import pop_notifications
 
 
 class TestPackageBuildBase(TestCaseWithFactory):
@@ -373,9 +374,29 @@
         d = self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
                 })
+
         def got_status(ignored):
             self.assertNotEqual(None, self.build.log)
-        return d.addCallback(got_status)
+
+        return d.addCallback(got_status)
+
+    def _test_handleStatus_notifies(self, status):
+        # An email notification is sent for a given build status.
+        def got_status(ignored):
+            self.failIf(
+                len(pop_notifications()) == 0, "No notifications received")
+
+        d = self.build.handleStatus(status, None, {})
+        return d.addCallback(got_status)
+
+    def test_handleStatus_DEPFAIL_notifies(self):
+        return self._test_handleStatus_notifies("DEPFAIL")
+
+    def test_handleStatus_CHROOTFAIL_notifies(self):
+        return self._test_handleStatus_notifies("CHROOTFAIL")
+
+    def test_handleStatus_PACKAGEFAIL_notifies(self):
+        return self._test_handleStatus_notifies("PACKAGEFAIL")
 
     def test_date_finished_set(self):
         # The date finished is updated during handleStatus_OK.
@@ -384,6 +405,8 @@
         d = self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
                 })
+
         def got_status(ignored):
             self.assertNotEqual(None, self.build.date_finished)
+
         return d.addCallback(got_status)