Merge lp:~jtv/launchpad/bug-569108 into lp:launchpad

[Jeroen T. Vermeulen]

Fix deletion of TranslationTemplateBuildJobs.

= Bug 569108 =

Work in progress. Fixing up deletion of BuildQueue/IBuildFarmJobDerived/Job when done.

[Jeroen T. Vermeulen (jtv)]

Ready for review now. Also passed Q/A on dogfood. No lint. To test:
{{{
./bin/test -vv -t test_translationtemplatesbuildjob
}}}

As a drive-by, I also removed a TranslationTemplatesBuildBehavior test that had already been moved out into a file of its own, but never deleted from the file it was in.

Jeroen

[Michael Nelson (michael.nelson)]

Nice clean fix, thanks Jeroen.

I'm not sure how I'll cope with this when I merge it into the refactoring work, where BuildFarmJobs (and their derivations) can be db-backed themselves, but I'll figure that out :)

review: Approve (code)

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
--- lib/lp/buildmaster/interfaces/buildfarmjob.py	2010-04-21 07:15:40 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjob.py	2010-04-28 10:17:30 +0000
@@ -93,7 +93,7 @@
 class IBuildFarmJobDerived(Interface):
     """Common functionality required by classes delegating IBuildFarmJob.
 
-    An implementation of this class must setup the necessary delagation.
+    An implementation of this class must setup the necessary delegation.
     """
 
     def getByJob(job):
@@ -149,3 +149,5 @@
             accurately based on this job's properties.
         """
 
+    def cleanUp():
+        """Job's finished.  Delete its supporting data."""

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2010-04-21 07:15:40 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2010-04-28 10:17:30 +0000
@@ -8,14 +8,16 @@
     ]
 
 
+import hashlib
+
 from lazr.delegates import delegates
 
-import hashlib
-
 from zope.component import getUtility
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
+from storm.store import Store
+
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
 
@@ -128,4 +130,6 @@
 
         return hashlib.sha1(contents).hexdigest()
 
-
+    def cleanUp(self):
+        """See `IBuildFarmJob`."""
+        Store.of(self).remove(self)

=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2010-03-24 07:17:20 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2010-04-28 10:17:30 +0000
@@ -18,7 +18,6 @@
 
 from sqlobject import (
     StringCol, ForeignKey, BoolCol, IntCol, IntervalCol, SQLObjectNotFound)
-from storm.store import Store
 from zope.component import getSiteManager, getUtility
 from zope.interface import implements
 
@@ -130,7 +129,7 @@
         job = self.job
         specific_job = self.specific_job
         SQLBase.destroySelf(self)
-        Store.of(specific_job).remove(specific_job)
+        specific_job.cleanUp()
         job.destroySelf()
 
     def manualScore(self, value):

=== modified file 'lib/lp/buildmaster/model/packagebuildfarmjob.py'
--- lib/lp/buildmaster/model/packagebuildfarmjob.py	2010-04-20 14:38:20 +0000
+++ lib/lp/buildmaster/model/packagebuildfarmjob.py	2010-04-28 10:17:30 +0000
@@ -56,5 +56,3 @@
     """
     def _set_build_farm_job(self):
         self._build_farm_job = PackageBuildFarmJob(self.build)
-
-

=== modified file 'lib/lp/translations/model/translationtemplatesbuildjob.py'
--- lib/lp/translations/model/translationtemplatesbuildjob.py	2010-04-16 13:31:48 +0000
+++ lib/lp/translations/model/translationtemplatesbuildjob.py	2010-04-28 10:17:30 +0000
@@ -13,6 +13,8 @@
 from zope.interface import classProvides, implements
 from zope.security.proxy import removeSecurityProxy
 
+from storm.store import Store
+
 from canonical.config import config
 
 from canonical.launchpad.webapp.interfaces import (
@@ -67,6 +69,14 @@
         """See `IBuildFarmJob`."""
         return '%s translation templates build' % self.branch.bzr_identity
 
+    def cleanUp(self):
+        """See `IBuildFarmJob`."""
+        # This class is not itself database-backed.  But it delegates to
+        # one that is.  We can't call its SQLObject destroySelf method
+        # though, because then the BuildQueue and the BranchJob would
+        # both try to delete the attached Job.
+        Store.of(self.context).remove(self.context)
+
     @classmethod
     def _hasPotteryCompatibleSetup(cls, branch):
         """Does `branch` look as if pottery can generate templates for it?

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildjob.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildjob.py	2010-04-16 13:31:48 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildjob.py	2010-04-28 10:17:30 +0000
@@ -9,8 +9,9 @@
 from zope.event import notify
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from sqlobject import SQLObjectNotFound
+from storm.store import Store
+
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
@@ -20,10 +21,9 @@
 
 from lp.buildmaster.interfaces.buildfarmjob import (
     IBuildFarmJob, IBuildFarmJobDerived)
-from lp.buildmaster.interfaces.buildfarmjobbehavior import (
-    IBuildFarmJobBehavior)
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.code.interfaces.branch import IBranchSet
 from lp.code.interfaces.branchjob import IBranchJob
 from lp.code.model.branchjob import BranchJob
 from lp.code.model.directbranchcommit import DirectBranchCommit
@@ -105,6 +105,29 @@
         # future however the scoring system is to be revisited.
         self.assertEqual(1000, self.specific_job.score())
 
+    def test_cleanUp(self):
+        # TranslationTemplatesBuildJob has its own customized cleanup
+        # behaviour, since it's actually a BranchJob.
+        job = removeSecurityProxy(self.specific_job.job)
+        buildqueue = getUtility(IBuildQueueSet).getByJob(job)
+
+        job_id = job.id
+        store = Store.of(job)
+        branch_name = self.branch.unique_name
+
+        buildqueue.destroySelf()
+
+        # BuildQueue is gone.
+        self.assertIs(
+            None, store.find(BuildQueue, BuildQueue.job == job_id).one())
+        # Job is gone.
+        self.assertIs(None, store.find(Job, Job.id == job_id).one())
+        # TranslationTemplatesBuildJob is gone.
+        self.assertIs(None, TranslationTemplatesBuildJob.getByJob(job_id))
+        # Branch is still here.
+        branch_set = getUtility(IBranchSet)
+        self.assertEqual(self.branch, branch_set.getByUniqueName(branch_name))
+
 
 class FakeTranslationTemplatesJobSource(TranslationTemplatesBuildJob):
     """Fake utility class.
@@ -255,35 +278,5 @@
         self.assertEqual(tail, url[-len(tail):])
 
 
-class TestTranslationTemplatesBuildBehavior(TestCaseWithFactory):
-    """Test `TranslationTemplatesBuildBehavior`."""
-
-    layer = ZopelessDatabaseLayer
-
-    def setUp(self):
-        super(TestTranslationTemplatesBuildBehavior, self).setUp()
-        self.jobset = getUtility(ITranslationTemplatesBuildJobSource)
-        self.branch = self.factory.makeBranch()
-        self.specific_job = self.jobset.create(self.branch)
-        self.behavior = IBuildFarmJobBehavior(self.specific_job)
-
-    def test_getChroot(self):
-        # _getChroot produces the current chroot for the current Ubuntu
-        # release, on the nominated architecture for
-        # architecture-independent builds.
-        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-        current_ubuntu = ubuntu.currentseries
-        distroarchseries = current_ubuntu.nominatedarchindep
-
-        # Set an arbitrary chroot file.
-        fake_chroot_file = getUtility(ILibraryFileAliasSet)[1]
-        distroarchseries.addOrUpdateChroot(fake_chroot_file)
-
-        chroot = self.behavior._getChroot()
-
-        self.assertNotEqual(None, chroot)
-        self.assertEqual(fake_chroot_file, chroot)
-
-
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)