launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/simplify-packagecopyjob-tests into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/simplify-packagecopyjob-tests/+merge/115159

There is no point in mimicking the job runner in the PCJ tests; it isn't any faster as far as I can tell, and it introduces an extra source of possible mistakes.  We might as well just use the job runner directly.
-- 
https://code.launchpad.net/~cjwatson/launchpad/simplify-packagecopyjob-tests/+merge/115159
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/simplify-packagecopyjob-tests into lp:launchpad.

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-07-12 08:52:45 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-07-16 14:35:33 +0000
@@ -6,7 +6,6 @@
 import operator
 from textwrap import dedent
 
-from lazr.jobrunner.jobrunner import SuspendJobException
 from storm.store import Store
 from testtools.content import text_content
 from testtools.matchers import MatchesStructure
@@ -25,6 +24,7 @@
 from lp.services.database.lpstorm import IStore
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.runner import JobRunner
 from lp.services.job.tests import (
     block_on_job,
     pop_remote_notifications,
@@ -163,22 +163,8 @@
 
     def runJob(self, job):
         """Helper to switch to the right DB user and run the job."""
-        # We are basically mimicking the job runner here.
         switch_dbuser(self.dbuser)
-        # Set the state to RUNNING.
-        job.start()
-        # Commit the RUNNING state.
-        self.layer.txn.commit()
-        try:
-            job.run()
-        except SuspendJobException:
-            # Re-raise this one as many tests check for its presence.
-            raise
-        except:
-            transaction.abort()
-            job.fail()
-        else:
-            job.complete()
+        JobRunner([job]).runAll()
 
 
 class PlainPackageCopyJobTests(TestCaseWithFactory, LocalTestHelper):
@@ -755,10 +741,8 @@
             include_binaries=False,
             requester=requester)
 
-        self.assertRaises(SuspendJobException, self.runJob, job)
-        # Simulate the job runner suspending after getting a
-        # SuspendJobException
-        job.suspend()
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         switch_dbuser("launchpad_main")
 
         # Add some overrides to the job.
@@ -816,7 +800,8 @@
 
         # The job should be suspended and there's a PackageUpload with
         # its package_copy_job set.
-        self.assertRaises(SuspendJobException, self.runJob, job)
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         pu = Store.of(target_archive).find(
             PackageUpload,
             PackageUpload.package_copy_job_id == job.id).one()
@@ -864,8 +849,8 @@
         job = self.createCopyJobForSPPH(spph, source_archive, target_archive)
 
         # Run the job so it gains a PackageUpload.
-        self.assertRaises(SuspendJobException, self.runJob, job)
-        job.suspend()
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         if return_job:
             return job
         pcj = removeSecurityProxy(job).context
@@ -926,7 +911,8 @@
 
         # The job should be suspended and there's a PackageUpload with
         # its package_copy_job set in the UNAPPROVED queue.
-        self.assertRaises(SuspendJobException, self.runJob, job)
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
 
         pu = Store.of(target_archive).find(
             PackageUpload,
@@ -963,10 +949,8 @@
             spph, source_archive, target_archive, requester=requester)
 
         # Run the job so it gains a PackageUpload.
-        self.assertRaises(SuspendJobException, self.runJob, job)
-        # Simulate the job runner suspending after getting a
-        # SuspendJobException
-        job.suspend()
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         switch_dbuser("launchpad_main")
 
         # Accept the upload to release the job then run it.
@@ -1099,8 +1083,8 @@
         self.assertTrue(job.unembargo)
 
         # Run the job so it gains a PackageUpload.
-        self.assertRaises(SuspendJobException, self.runJob, job)
-        job.suspend()
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         switch_dbuser("launchpad_main")
 
         # Accept the upload to release the job then run it.
@@ -1152,8 +1136,8 @@
             include_binaries=True)
 
         # Start, accept, and run the job.
-        self.assertRaises(SuspendJobException, self.runJob, job)
-        job.suspend()
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         switch_dbuser("launchpad_main")
         pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
             [removeSecurityProxy(job).context.id]).one()
@@ -1320,10 +1304,8 @@
             source_pub, source_archive, target_archive)
 
         # Run the job so it gains a PackageUpload.
-        self.assertRaises(SuspendJobException, self.runJob, job)
-        # Simulate the job runner suspending after getting a
-        # SuspendJobException
-        job.suspend()
+        self.runJob(job)
+        self.assertEqual(JobStatus.SUSPENDED, job.status)
         switch_dbuser("launchpad_main")
 
         # Patch the job's attemptCopy() method so it just raises an