Merge lp:~abentley/launchpad/fix-build-time-display into lp:launchpad

[Aaron Bentley]

No exception when checking for estimated completion time.

= Summary =
Fix bug #570884: build time display oopses for active builds

== Proposed fix ==
Change the displayed time so that it is always the completion time, whether
actual or estimated.

== Pre-implementation notes ==
None

== Implementation details ==
There are now two phases of ETA. When the Job is WAITING, the eta is
calculated by adding the estimated duration to the estimated start time.

When the job is not waiting, the ETA is calculated by adding the estimated
duration to the actual start time.

== Tests ==
bin/test -vt browser test_sourcepackagerecipe

== Demo and Q/A ==
Request a build. Repeatedly refresh until the build is in progress. It should
display an estimated completion time, and there should be no oops.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

[Paul Hummer (rockstar)]

<rockstar> abentley, I'm not sure TestSourcePackageRecipeBuildView should inherit from BrowserTestCase - TestCaseWithFactory is probably fine.
<rockstar> I say this because it doesn't actually use a browser, but just instantiates a raw view.
<abentley> rockstar, Okay.

review: Approve (code)

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-04-23 03:30:30 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-04-28 22:49:29 +0000
@@ -45,6 +45,7 @@
     IArchiveSet)
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.job.interfaces.job import JobStatus
 
 
 class IRecipesForPerson(Interface):
@@ -52,8 +53,7 @@
 
 
 class RecipesForPersonBreadcrumb(Breadcrumb):
-    """A Breadcrumb that will handle the "Recipes" link for recipe breadcrumbs.
-    """
+    """A Breadcrumb to handle the "Recipes" link for recipe breadcrumbs."""
 
     rootsite = 'code'
     text = 'Recipes'
@@ -229,14 +229,26 @@
 
     @property
     def eta(self):
-        """The datetime when the build job is estimated to begin."""
+        """The datetime when the build job is estimated to complete.
+
+        This is the BuildQueue.estimated_duration plus the
+        Job.date_started or BuildQueue.getEstimatedJobStartTime.
+        """
         if self.context.buildqueue_record is None:
             return None
-        return self.context.buildqueue_record.getEstimatedJobStartTime()
+        queue_record = self.context.buildqueue_record
+        if queue_record.job.status == JobStatus.WAITING:
+            start_time = queue_record.getEstimatedJobStartTime()
+            if start_time is None:
+                return None
+        else:
+            start_time = queue_record.job.date_started
+        duration = queue_record.estimated_duration
+        return start_time + duration
 
     @property
     def date(self):
-        """The date when the build complete or will begin."""
+        """The date when the build completed or is estimated to complete."""
         if self.estimate:
             return self.eta
         return self.context.datebuilt
@@ -244,7 +256,9 @@
     @property
     def estimate(self):
         """If true, the date value is an estimate."""
-        return (self.context.datebuilt is None and self.eta is not None)
+        if self.context.datebuilt is not None:
+            return False
+        return self.eta is not None
 
 
 class ISourcePackageAddEditSchema(Interface):
@@ -272,7 +286,7 @@
     def validate(self, data):
         try:
             parser = RecipeParser(data['recipe_text'])
-            recipe_text = parser.parse()
+            parser.parse()
         except RecipeParseError:
             self.setFieldError(
                 'recipe_text',

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-04-23 02:35:47 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-04-28 22:49:29 +0000
@@ -19,9 +19,11 @@
     extract_text, find_main_content, find_tags_by_class)
 from canonical.testing import DatabaseFunctionalLayer
 from lp.buildmaster.interfaces.buildbase import BuildStatus
-from lp.code.browser.sourcepackagerecipe import SourcePackageRecipeView
+from lp.code.browser.sourcepackagerecipe import (
+    SourcePackageRecipeView, SourcePackageRecipeBuildView
+)
 from lp.code.interfaces.sourcepackagerecipe import MINIMAL_RECIPE_TEXT
-from lp.testing import ANONYMOUS, BrowserTestCase, login
+from lp.testing import ANONYMOUS, BrowserTestCase, login, TestCaseWithFactory
 
 
 class TestCaseForRecipe(BrowserTestCase):
@@ -330,6 +332,50 @@
         self.assertEqual(['Secret Squirrel', 'Woody'], build_distros)
 
 
+class TestSourcePackageRecipeBuildView(TestCaseWithFactory):
+    """Test behaviour of SourcePackageReciptBuildView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_estimate(self):
+        """Time should be estimated until the job is completed."""
+        build = self.factory.makeSourcePackageRecipeBuild()
+        queue_entry = self.factory.makeSourcePackageRecipeBuildJob(
+            recipe_build=build)
+        self.factory.makeBuilder()
+        view = SourcePackageRecipeBuildView(build, None)
+        self.assertTrue(view.estimate)
+        queue_entry.job.start()
+        self.assertTrue(view.estimate)
+        removeSecurityProxy(build).datebuilt = datetime.now(utc)
+        self.assertFalse(view.estimate)
+
+    def test_eta(self):
+        """ETA should be reasonable.
+
+        It should be None if there is no builder or queue entry.
+        It should be getEstimatedJobStartTime + estimated duration for jobs
+        that have not started.
+        It should be job.date_started + estimated duration for jobs that have
+        started.
+        """
+        build = self.factory.makeSourcePackageRecipeBuild()
+        view = SourcePackageRecipeBuildView(build, None)
+        self.assertIs(None, view.eta)
+        queue_entry = self.factory.makeSourcePackageRecipeBuildJob(
+            recipe_build=build)
+        queue_entry._now = lambda: datetime(1970, 1, 1, 0, 0, 0, 0, utc)
+        self.factory.makeBuilder()
+        self.assertIsNot(None, view.eta)
+        self.assertEqual(
+            queue_entry.getEstimatedJobStartTime() +
+            queue_entry.estimated_duration, view.eta)
+        queue_entry.job.start()
+        self.assertEqual(
+            queue_entry.job.date_started + queue_entry.estimated_duration,
+            view.eta)
+
+
 class TestSourcePackageRecipeDeleteView(TestCaseForRecipe):
 
     layer = DatabaseFunctionalLayer