launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/fix-pending-build-determination into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #680733 broken recipe build gets stuck at top of "5 latest builds" list on recipe page, forever
  https://bugs.launchpad.net/bugs/680733

For more details, see:
https://code.launchpad.net/~thumper/launchpad/fix-pending-build-determination/+merge/46088

Some recipe builds were getting stuck at the top of the list on the main recipe page.  This is due to an incorrect query to determine pending builds.

I also refactored the getBuilds method so it doesn't take a boolean parameter to change the behaviour.  I found this very confusing while reading the browser code.
-- 
https://code.launchpad.net/~thumper/launchpad/fix-pending-build-determination/+merge/46088
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/fix-pending-build-determination into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-01-11 01:09:43 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-01-13 03:59:58 +0000
@@ -221,7 +221,7 @@
         All pending builds are shown, as well as 1-5 recent builds.
         Recent builds are ordered by date completed.
         """
-        builds = list(self.context.getBuilds(pending=True))
+        builds = list(self.context.getPendingBuilds())
         for build in self.context.getBuilds():
             builds.append(build)
             if len(builds) >= 5:

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-01-11 01:09:43 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-01-13 03:59:58 +0000
@@ -1038,7 +1038,7 @@
         browser.getControl('Request builds').click()
 
         login(ANONYMOUS)
-        builds = recipe.getBuilds(True)
+        builds = recipe.getPendingBuilds()
         build_distros = [
             build.distroseries.displayname for build in builds]
         build_distros.sort()

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2010-12-02 16:13:51 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2011-01-13 03:59:58 +0000
@@ -106,12 +106,11 @@
         :param distroseries: The distroseries to build for.
         """
 
-    def getBuilds(pending=False):
-        """Return a ResultSet of all the builds in the given state.
+    def getBuilds():
+        """Return a ResultSet of all the non-pending builds."""
 
-        :param pending: If True, select all builds that are pending.  If
-            False, select all builds that are not pending.
-        """
+    def getPendingBuilds():
+        """Return a ResultSet of all the pending builds."""
 
     def getLastBuild():
         """Return the the most recent build of this recipe."""

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2010-12-15 14:07:17 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-01-13 03:59:58 +0000
@@ -246,19 +246,27 @@
             queue_record.manualScore(queue_record.lastscore + 100)
         return build
 
-    def getBuilds(self, pending=False):
-        """See `ISourcePackageRecipe`."""
-        if pending:
-            clauses = [BuildFarmJob.date_finished == None]
-        else:
-            clauses = [BuildFarmJob.date_finished != None]
+    def getBuilds(self):
+        """See `ISourcePackageRecipe`."""
+        where_clause = BuildFarmJob.status != BuildStatus.NEEDSBUILD
+        order_by = Desc(BuildFarmJob.date_finished)
+        return self._getBuilds(where_clause, order_by)
+
+    def getPendingBuilds(self):
+        """See `ISourcePackageRecipe`."""
+        where_clause = BuildFarmJob.status == BuildStatus.NEEDSBUILD
+        order_by = Desc(BuildFarmJob.date_created)
+        return self._getBuilds(where_clause, order_by)
+
+    def _getBuilds(self, where_clause, order_by):
+        """The actual query to get the builds."""
         result = Store.of(self).find(
             SourcePackageRecipeBuild,
             SourcePackageRecipeBuild.recipe==self,
             SourcePackageRecipeBuild.package_build_id == PackageBuild.id,
             PackageBuild.build_farm_job_id == BuildFarmJob.id,
-            *clauses)
-        result.order_by(Desc(BuildFarmJob.date_finished))
+            where_clause)
+        result.order_by(order_by)
         return result
 
     def getLastBuild(self):

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-12-02 16:13:51 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-01-13 03:59:58 +0000
@@ -585,6 +585,21 @@
         self.assertEqual(
             timedelta(minutes=11), recipe.getMedianBuildDuration())
 
+    def test_getBuilds(self):
+        # Builds that need building are pending.
+        recipe = self.factory.makeSourcePackageRecipe()
+        build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
+        self.assertEqual([], list(recipe.getBuilds()))
+        self.assertEqual([build], list(recipe.getPendingBuilds()))
+
+    def test_getBuilds_cancelled(self):
+        # Cancelled builds are not considered pending.
+        recipe = self.factory.makeSourcePackageRecipe()
+        build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
+        build.cancelBuild()
+        self.assertEqual([build], list(recipe.getBuilds()))
+        self.assertEqual([], list(recipe.getPendingBuilds()))
+
 
 class TestRecipeBranchRoundTripping(TestCaseWithFactory):
 

=== modified file 'lib/lp/code/scripts/tests/test_request_daily_builds.py'
--- lib/lp/code/scripts/tests/test_request_daily_builds.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/scripts/tests/test_request_daily_builds.py	2011-01-13 03:59:58 +0000
@@ -28,14 +28,14 @@
         pack_branch = self.factory.makePackageBranch()
         pack_recipe = self.factory.makeSourcePackageRecipe(
             build_daily=True, is_stale=True, branches=[pack_branch])
-        self.assertEqual(0, prod_recipe.getBuilds(True).count())
-        self.assertEqual(0, pack_recipe.getBuilds(True).count())
+        self.assertEqual(0, prod_recipe.getPendingBuilds().count())
+        self.assertEqual(0, pack_recipe.getPendingBuilds().count())
         transaction.commit()
         retcode, stdout, stderr = run_script(
             'cronscripts/request_daily_builds.py', [])
         self.assertIn('Requested 2 daily builds.', stderr)
-        self.assertEqual(1, prod_recipe.getBuilds(True).count())
-        self.assertEqual(1, pack_recipe.getBuilds(True).count())
+        self.assertEqual(1, prod_recipe.getPendingBuilds().count())
+        self.assertEqual(1, pack_recipe.getPendingBuilds().count())
         self.assertFalse(prod_recipe.is_stale)
         self.assertFalse(pack_recipe.is_stale)
 
@@ -47,7 +47,7 @@
         transaction.commit()
         retcode, stdout, stderr = run_script(
             'cronscripts/request_daily_builds.py', [])
-        self.assertEqual(0, recipe.getBuilds(True).count())
+        self.assertEqual(0, recipe.getPendingBuilds().count())
         self.assertIn('Requested 0 daily builds.', stderr)
         utility = ErrorReportingUtility()
         utility.configure('request_daily_builds')