Merge lp:~allenap/launchpad/job-status-validator into lp:launchpad

Proposed by Gavin Panella
Status: Work in progress
Proposed branch: lp:~allenap/launchpad/job-status-validator
Merge into: lp:launchpad
Diff against target: 694 lines (+182/-86)
18 files modified
lib/lp/buildmaster/model/builder.py (+2/-2)
lib/lp/buildmaster/model/buildqueue.py (+1/-1)
lib/lp/code/model/branch.py (+2/-2)
lib/lp/code/model/branchjob.py (+2/-2)
lib/lp/code/model/branchmergeproposal.py (+1/-1)
lib/lp/code/model/branchmergeproposaljob.py (+2/-2)
lib/lp/code/model/tests/test_branchjob.py (+1/-1)
lib/lp/code/scripts/tests/test_scan_branches.py (+1/-1)
lib/lp/registry/model/persontransferjob.py (+1/-1)
lib/lp/registry/tests/test_person_merge_job.py (+1/-1)
lib/lp/services/job/model/job.py (+38/-27)
lib/lp/services/job/testing/__init__.py (+37/-0)
lib/lp/services/job/tests/test_job.py (+76/-26)
lib/lp/soyuz/model/distroseriesdifferencejob.py (+1/-1)
lib/lp/soyuz/model/packagecopyjob.py (+2/-2)
lib/lp/soyuz/tests/test_distroseriesdifferencejob.py (+2/-1)
lib/lp/soyuz/tests/test_packagecopyjob.py (+11/-14)
lib/lp/translations/model/translationsharingjob.py (+1/-1)
To merge this branch: bzr merge lp:~allenap/launchpad/job-status-validator
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Disapprove
Jeroen T. Vermeulen (community) code Needs Information
Ian Booth (community) code* Approve
Review via email: mp+62509@code.launchpad.net

Commit message

Consolidate the purpose of Job's status/_status/_set_status code into a single validator.

Description of the change

This removes the status/_status/_set_status issue in Job (where status
is a gettable property, _status represents the database column, and
_set_status is what Job's state transition methods call).

Why?

- I've lost time before wondering why queries aren't working before
  realising I need to use Job._status, so this clears that up.

- It simplifies the code because everything goes via the status
  property, and all transitions are validated.

- Even code within Job can't (accidentally) cheat on status
  transitions, to the extent that a helper, prepare_job(), was
  introduced in the tests in order to get a Job in a desired starting
  state.

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

This is a great change. As you say, everything is much clearer now.

review: Approve (code*)
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

This one's bitten me too, so I'll be very glad to see it go.

One thing I missed was a pre-imp. Gavin, did you verify that there isn't some good but invisible reason for the current design? I believe Aaron wrote this, and he seems to prefer explaining his code in person over leaving comments.

review: Needs Fixing (code)
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Ahem. Something went wrong with my vote there.

review: Needs Information (code)
Revision history for this message
Gavin Panella (allenap) wrote :

Hi Aaron,

I forgot to ping you today about this, so for now this will have to
do. When you have a chance can you have a gander at this, especially
with respect to Jeroen's comment:

> One thing I missed was a pre-imp. Gavin, did you verify that there
> isn't some good but invisible reason for the current design? I
> believe Aaron wrote this, and he seems to prefer explaining his code
> in person over leaving comments.

I'm very bad at doing pre-implementation calls, so I apologise in
advance if I've done something stupid because of the lack of that.

I'm back on Tuesday, so if you'd like to talk about it let's try to
talk then.

Thanks, Gavin.

Revision history for this message
Aaron Bentley (abentley) wrote :

I didn't make _status private because only some transitions are valid. That would be silly, because you can just make a read/write property for that. And the solution you've given would be even better in that case.

I made status private because status is part of a more complex state that includes the date_started and date_finished. It should not be set in isolation, so I made it private and provided methods for setting it to the desired state. By providing a writable status, you encourage people to set it directly, which will produce inconsistent states (e.g. Job.status == JobStatus.RUNNING, but Job.date_started is None).

For example, prepare_job will apparently produce inconsistent states. IIUC, prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED) will produce a job that is COMPLETED, but has no date_started or date_finished. Now obviously, this is test code and it doesn't matter for those particular tests, but at least in the previous version, it was clear that they were doing something slightly dirty, because they were setting a private variable.

review: Disapprove
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Why not have a public status that you can read and use in queries, and make the validator reject everything because you're not using the mutator methods.

This is what happens on PackageUpload - see lib/lp/soyuz/model/queue.py and search for validate_status.

The current situation is plainly suboptimal and I agree with the intention of this branch.

Revision history for this message
Aaron Bentley (abentley) wrote :

If we can have a readonly "status" attribute and still write to the database column via mutators, that certainly works for me.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

That's exactly what the PackageUpload code is doing.

13127. By Gavin Panella

Test to demonstrate Job's attribute access.

13128. By Gavin Panella

Test access of all attributes at once, so that we get a complete picture on failure.

13129. By Gavin Panella

Merge devel, resolving several conflicts.

13130. By Gavin Panella

Change prepare_job() to automatically figure out how to transition.

13131. By Gavin Panella

Switch to a single argument version of prepare_job().

13132. By Gavin Panella

Move 'invariants' code into validate_status_transition().

13133. By Gavin Panella

Use super(); Exception is now a new-style class.

13134. By Gavin Panella

Merge devel.

13135. By Gavin Panella

Move prepare_job into a testing package.

13136. By Gavin Panella

Merge devel.

13137. By Gavin Panella

Adjust some more parts of Launchpad to the Job._status -> .status change.

13138. By Gavin Panella

Merge devel.

Unmerged revisions

13138. By Gavin Panella

Merge devel.

13137. By Gavin Panella

Adjust some more parts of Launchpad to the Job._status -> .status change.

13136. By Gavin Panella

Merge devel.

13135. By Gavin Panella

Move prepare_job into a testing package.

13134. By Gavin Panella

Merge devel.

13133. By Gavin Panella

Use super(); Exception is now a new-style class.

13132. By Gavin Panella

Move 'invariants' code into validate_status_transition().

13131. By Gavin Panella

Switch to a single argument version of prepare_job().

13130. By Gavin Panella

Change prepare_job() to automatically figure out how to transition.

13129. By Gavin Panella

Merge devel, resolving several conflicts.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/model/builder.py'
2--- lib/lp/buildmaster/model/builder.py 2011-09-13 05:23:16 +0000
3+++ lib/lp/buildmaster/model/builder.py 2011-10-07 13:40:29 +0000
4@@ -854,7 +854,7 @@
5 BuildQueue,
6 BuildQueue.job == Job.id,
7 BuildQueue.builder == self.id,
8- Job._status == JobStatus.RUNNING,
9+ Job.status == JobStatus.RUNNING,
10 Job.date_started != None).one()
11
12 def getCurrentBuildFarmJob(self):
13@@ -914,7 +914,7 @@
14 Coalesce(BuildQueue.virtualized, True)),
15 Processor.id == BuildQueue.processorID,
16 Job.id == BuildQueue.jobID,
17- Job._status == JobStatus.WAITING).group_by(
18+ Job.status == JobStatus.WAITING).group_by(
19 Processor, Coalesce(BuildQueue.virtualized, True))
20
21 result_dict = {'virt': {}, 'nonvirt': {}}
22
23=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
24--- lib/lp/buildmaster/model/buildqueue.py 2010-08-30 15:00:23 +0000
25+++ lib/lp/buildmaster/model/buildqueue.py 2011-10-07 13:40:29 +0000
26@@ -553,6 +553,6 @@
27 # Avoid corrupt build jobs where the builder is None.
28 BuildQueue.builder != None,
29 # status is a property. Let's use _status.
30- Job._status == JobStatus.RUNNING,
31+ Job.status == JobStatus.RUNNING,
32 Job.date_started != None)
33 return result_set
34
35=== modified file 'lib/lp/code/model/branch.py'
36--- lib/lp/code/model/branch.py 2011-09-27 00:57:13 +0000
37+++ lib/lp/code/model/branch.py 2011-10-07 13:40:29 +0000
38@@ -1176,8 +1176,8 @@
39 BranchJob,
40 BranchJob.branch == self,
41 Job.id == BranchJob.jobID,
42- Job._status != JobStatus.COMPLETED,
43- Job._status != JobStatus.FAILED,
44+ Job.status != JobStatus.COMPLETED,
45+ Job.status != JobStatus.FAILED,
46 BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
47 return jobs.count() > 0
48
49
50=== modified file 'lib/lp/code/model/branchjob.py'
51--- lib/lp/code/model/branchjob.py 2011-08-26 00:43:31 +0000
52+++ lib/lp/code/model/branchjob.py 2011-10-07 13:40:29 +0000
53@@ -926,8 +926,8 @@
54 Job.id == BranchJob.jobID,
55 BranchJob.branch == branch,
56 BranchJob.job_type == BranchJobType.ROSETTA_UPLOAD,
57- Job._status != JobStatus.COMPLETED,
58- Job._status != JobStatus.FAILED)
59+ Job.status != JobStatus.COMPLETED,
60+ Job.status != JobStatus.FAILED)
61 if since is not None:
62 match = And(match, Job.date_created > since)
63 jobs = store.using(BranchJob, Job).find((BranchJob), match)
64
65=== modified file 'lib/lp/code/model/branchmergeproposal.py'
66--- lib/lp/code/model/branchmergeproposal.py 2011-09-14 10:19:43 +0000
67+++ lib/lp/code/model/branchmergeproposal.py 2011-10-07 13:40:29 +0000
68@@ -206,7 +206,7 @@
69 BranchMergeProposalJob.job_type ==
70 BranchMergeProposalJobType.UPDATE_PREVIEW_DIFF,
71 BranchMergeProposalJob.job == Job.id,
72- Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
73+ Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
74 job = jobs.order_by(Job.scheduled_start, Job.date_created).first()
75 if job is not None:
76 return BranchMergeProposalJobFactory.create(job)
77
78=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
79--- lib/lp/code/model/branchmergeproposaljob.py 2011-09-26 20:00:50 +0000
80+++ lib/lp/code/model/branchmergeproposaljob.py 2011-10-07 13:40:29 +0000
81@@ -779,7 +779,7 @@
82 TargetBranch = ClassAlias(Branch)
83 clauses = [
84 BranchMergeProposalJob.job == Job.id,
85- Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
86+ Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
87 BranchMergeProposalJob.branch_merge_proposal ==
88 BranchMergeProposal.id, BranchMergeProposal.source_branch ==
89 SourceBranch.id, BranchMergeProposal.target_branch ==
90@@ -794,7 +794,7 @@
91 # the date_created, then job type. This should give us all creation
92 # jobs before comment jobs.
93 jobs = jobs.order_by(
94- Desc(Job._status), Job.date_created,
95+ Desc(Job.status), Job.date_created,
96 Desc(BranchMergeProposalJob.job_type))
97 # Now only return one job for any given merge proposal.
98 ready_jobs = []
99
100=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
101--- lib/lp/code/model/tests/test_branchjob.py 2011-10-06 16:26:36 +0000
102+++ lib/lp/code/model/tests/test_branchjob.py 2011-10-07 13:40:29 +0000
103@@ -1227,7 +1227,7 @@
104 job = self._makeRosettaUploadJob()
105 job.job.start()
106 job.job.complete()
107- job.job._status = JobStatus.FAILED
108+ job.job.status = JobStatus.FAILED
109 unfinished_jobs = list(RosettaUploadJob.findUnfinishedJobs(
110 self.branch))
111 self.assertEqual([], unfinished_jobs)
112
113=== modified file 'lib/lp/code/scripts/tests/test_scan_branches.py'
114--- lib/lp/code/scripts/tests/test_scan_branches.py 2010-10-26 15:47:24 +0000
115+++ lib/lp/code/scripts/tests/test_scan_branches.py 2011-10-07 13:40:29 +0000
116@@ -76,7 +76,7 @@
117 result = store.find(
118 BranchJob,
119 BranchJob.jobID == Job.id,
120- Job._status == JobStatus.WAITING,
121+ Job.status == JobStatus.WAITING,
122 BranchJob.job_type == BranchJobType.REVISION_MAIL,
123 BranchJob.branch == db_branch)
124 self.assertEqual(result.count(), 1)
125
126=== modified file 'lib/lp/registry/model/persontransferjob.py'
127--- lib/lp/registry/model/persontransferjob.py 2011-08-12 14:39:51 +0000
128+++ lib/lp/registry/model/persontransferjob.py 2011-10-07 13:40:29 +0000
129@@ -370,7 +370,7 @@
130 conditions = [
131 PersonTransferJob.job_type == cls.class_job_type,
132 PersonTransferJob.job_id == Job.id,
133- Job._status.is_in(Job.PENDING_STATUSES)]
134+ Job.status.is_in(Job.PENDING_STATUSES)]
135 arg_conditions = []
136 if from_person is not None:
137 arg_conditions.append(
138
139=== modified file 'lib/lp/registry/tests/test_person_merge_job.py'
140--- lib/lp/registry/tests/test_person_merge_job.py 2011-07-30 09:14:44 +0000
141+++ lib/lp/registry/tests/test_person_merge_job.py 2011-10-07 13:40:29 +0000
142@@ -174,7 +174,7 @@
143 def test_find_only_pending_or_running(self):
144 # find() only returns jobs that are pending.
145 for status in JobStatus.items:
146- removeSecurityProxy(self.job.job)._status = status
147+ removeSecurityProxy(self.job.job).status = status
148 if status in Job.PENDING_STATUSES:
149 self.assertEqual([self.job], self.find())
150 else:
151
152=== modified file 'lib/lp/services/job/model/job.py'
153--- lib/lp/services/job/model/job.py 2011-09-21 13:06:53 +0000
154+++ lib/lp/services/job/model/job.py 2011-10-07 13:40:29 +0000
155@@ -48,9 +48,33 @@
156 """Invalid transition from one job status to another attempted."""
157
158 def __init__(self, current_status, requested_status):
159- Exception.__init__(
160- self, 'Transition from %s to %s is invalid.' %
161- (current_status, requested_status))
162+ super(InvalidTransition, self).__init__(
163+ 'Transition from %s to %s is invalid.' % (
164+ current_status, requested_status))
165+
166+
167+def validate_status_transition(job, attr, status):
168+ """Validate the status transition and maintain "invariants"."""
169+ assert attr == "status"
170+ if status not in job._valid_transitions[job.status]:
171+ raise InvalidTransition(job.status, status)
172+
173+ # General invariants.
174+ if status == JobStatus.RUNNING:
175+ job.date_started = datetime.datetime.now(UTC)
176+ job.date_finished = None
177+ job.attempt_count += 1
178+ if status in (JobStatus.COMPLETED, JobStatus.FAILED):
179+ job.date_finished = datetime.datetime.now(UTC)
180+
181+ # Transition-specific invariants.
182+ transition = job.status, status
183+ if transition == (JobStatus.RUNNING, JobStatus.WAITING):
184+ job.date_finished = datetime.datetime.now(UTC)
185+ if transition == (JobStatus.SUSPENDED, JobStatus.WAITING):
186+ job.lease_expires = None
187+
188+ return status
189
190
191 class Job(SQLBase):
192@@ -70,8 +94,9 @@
193
194 log = StringCol()
195
196- _status = EnumCol(enum=JobStatus, notNull=True, default=JobStatus.WAITING,
197- dbName='status')
198+ status = EnumCol(
199+ enum=JobStatus, notNull=True, default=JobStatus.WAITING,
200+ dbName='status', storm_validator=validate_status_transition)
201
202 attempt_count = IntCol(default=0)
203
204@@ -102,13 +127,6 @@
205 JobStatus.RUNNING,
206 JobStatus.SUSPENDED))
207
208- def _set_status(self, status):
209- if status not in self._valid_transitions[self._status]:
210- raise InvalidTransition(self._status, status)
211- self._status = status
212-
213- status = property(lambda x: x._status)
214-
215 @property
216 def is_pending(self):
217 """See `IJob`."""
218@@ -153,42 +171,35 @@
219
220 def start(self):
221 """See `IJob`."""
222- self._set_status(JobStatus.RUNNING)
223- self.date_started = datetime.datetime.now(UTC)
224- self.date_finished = None
225- self.attempt_count += 1
226+ self.status = JobStatus.RUNNING
227
228 def complete(self):
229 """See `IJob`."""
230- self._set_status(JobStatus.COMPLETED)
231- self.date_finished = datetime.datetime.now(UTC)
232+ self.status = JobStatus.COMPLETED
233
234 def fail(self):
235 """See `IJob`."""
236- self._set_status(JobStatus.FAILED)
237- self.date_finished = datetime.datetime.now(UTC)
238+ self.status = JobStatus.FAILED
239
240 def queue(self):
241 """See `IJob`."""
242- self._set_status(JobStatus.WAITING)
243- self.date_finished = datetime.datetime.now(UTC)
244+ self.status = JobStatus.WAITING
245
246 def suspend(self):
247 """See `IJob`."""
248- self._set_status(JobStatus.SUSPENDED)
249+ self.status = JobStatus.SUSPENDED
250
251 def resume(self):
252 """See `IJob`."""
253 if self.status is not JobStatus.SUSPENDED:
254- raise InvalidTransition(self._status, JobStatus.WAITING)
255- self._set_status(JobStatus.WAITING)
256- self.lease_expires = None
257+ raise InvalidTransition(self.status, JobStatus.WAITING)
258+ self.status = JobStatus.WAITING
259
260
261 Job.ready_jobs = Select(
262 Job.id,
263 And(
264- Job._status == JobStatus.WAITING,
265+ Job.status == JobStatus.WAITING,
266 Or(Job.lease_expires == None, Job.lease_expires < UTC_NOW),
267 Or(Job.scheduled_start == None, Job.scheduled_start <= UTC_NOW),
268 ))
269
270=== added directory 'lib/lp/services/job/testing'
271=== added file 'lib/lp/services/job/testing/__init__.py'
272--- lib/lp/services/job/testing/__init__.py 1970-01-01 00:00:00 +0000
273+++ lib/lp/services/job/testing/__init__.py 2011-10-07 13:40:29 +0000
274@@ -0,0 +1,37 @@
275+# Copyright 2011 Canonical Ltd. This software is licensed under the
276+# GNU Affero General Public License version 3 (see the file LICENSE).
277+
278+"""Testing resources for lp.services.job."""
279+
280+__metaclass__ = type
281+
282+from lp.services.job.model.job import Job
283+
284+
285+def find_transition(s_from, s_to):
286+ """Find a valid transition route between `s_from` and `s_to`."""
287+ if s_to == s_from:
288+ return ()
289+ else:
290+ valid_transitions = Job._valid_transitions.iteritems()
291+ for from_status, to_statuses in valid_transitions:
292+ if s_to in to_statuses:
293+ if s_from == from_status:
294+ return (s_to,)
295+ else:
296+ possible = find_transition(s_from, from_status)
297+ if possible is not None:
298+ return possible + (s_to,)
299+ else:
300+ return None
301+
302+
303+def prepare_job(desrired_status, job=None):
304+ """Creates a new job and transitions its status to `desrired_status`."""
305+ if job is None:
306+ job = Job()
307+ transitions = find_transition(
308+ job.status, desrired_status)
309+ for status in transitions:
310+ job.status = status
311+ return job
312
313=== modified file 'lib/lp/services/job/tests/test_job.py'
314--- lib/lp/services/job/tests/test_job.py 2011-09-21 13:06:53 +0000
315+++ lib/lp/services/job/tests/test_job.py 2011-10-07 13:40:29 +0000
316@@ -8,6 +8,12 @@
317
318 import pytz
319 from storm.locals import Store
320+from zope.interface.interface import (
321+ Attribute,
322+ Method,
323+ )
324+from zope.security.interfaces import ForbiddenAttribute
325+from zope.security.proxy import ProxyFactory
326
327 from canonical.database.constants import UTC_NOW
328 from canonical.launchpad.interfaces.lpstorm import IStore
329@@ -22,6 +28,7 @@
330 Job,
331 LeaseHeld,
332 )
333+from lp.services.job.testing import prepare_job
334 from lp.testing import (
335 TestCase,
336 TestCaseWithFactory,
337@@ -100,17 +107,17 @@
338
339 def test_start_when_completed_is_invalid(self):
340 """When a job is completed, attempting to start is invalid."""
341- job = Job(_status=JobStatus.COMPLETED)
342+ job = prepare_job(JobStatus.COMPLETED)
343 self.assertRaises(InvalidTransition, job.start)
344
345 def test_start_when_failed_is_invalid(self):
346 """When a job is failed, attempting to start is invalid."""
347- job = Job(_status=JobStatus.FAILED)
348+ job = prepare_job(JobStatus.FAILED)
349 self.assertRaises(InvalidTransition, job.start)
350
351 def test_start_when_running_is_invalid(self):
352 """When a job is running, attempting to start is invalid."""
353- job = Job(_status=JobStatus.FAILED)
354+ job = prepare_job(JobStatus.FAILED)
355 self.assertRaises(InvalidTransition, job.start)
356
357 def test_complete(self):
358@@ -118,7 +125,7 @@
359
360 It should set date_finished and set the job status to COMPLETED.
361 """
362- job = Job(_status=JobStatus.RUNNING)
363+ job = Job(status=JobStatus.RUNNING)
364 self.assertEqual(None, job.date_finished)
365 job.complete()
366 self.assertNotEqual(None, job.date_finished)
367@@ -126,17 +133,17 @@
368
369 def test_complete_when_waiting_is_invalid(self):
370 """When a job is waiting, attempting to complete is invalid."""
371- job = Job(_status=JobStatus.WAITING)
372+ job = prepare_job(JobStatus.WAITING)
373 self.assertRaises(InvalidTransition, job.complete)
374
375 def test_complete_when_completed_is_invalid(self):
376 """When a job is completed, attempting to complete is invalid."""
377- job = Job(_status=JobStatus.COMPLETED)
378+ job = prepare_job(JobStatus.COMPLETED)
379 self.assertRaises(InvalidTransition, job.complete)
380
381 def test_complete_when_failed_is_invalid(self):
382 """When a job is failed, attempting to complete is invalid."""
383- job = Job(_status=JobStatus.FAILED)
384+ job = prepare_job(JobStatus.FAILED)
385 self.assertRaises(InvalidTransition, job.complete)
386
387 def test_fail(self):
388@@ -144,7 +151,7 @@
389
390 It should set date_finished and set the job status to FAILED.
391 """
392- job = Job(_status=JobStatus.RUNNING)
393+ job = Job(status=JobStatus.RUNNING)
394 self.assertEqual(None, job.date_finished)
395 job.fail()
396 self.assertNotEqual(None, job.date_finished)
397@@ -152,17 +159,17 @@
398
399 def test_fail_when_waiting_is_invalid(self):
400 """When a job is waiting, attempting to fail is invalid."""
401- job = Job(_status=JobStatus.WAITING)
402+ job = prepare_job(JobStatus.WAITING)
403 self.assertRaises(InvalidTransition, job.fail)
404
405 def test_fail_when_completed_is_invalid(self):
406 """When a job is completed, attempting to fail is invalid."""
407- job = Job(_status=JobStatus.COMPLETED)
408+ job = prepare_job(JobStatus.COMPLETED)
409 self.assertRaises(InvalidTransition, job.fail)
410
411 def test_fail_when_failed_is_invalid(self):
412 """When a job is failed, attempting to fail is invalid."""
413- job = Job(_status=JobStatus.FAILED)
414+ job = prepare_job(JobStatus.FAILED)
415 self.assertRaises(InvalidTransition, job.fail)
416
417 def test_queue(self):
418@@ -170,7 +177,7 @@
419
420 It should set date_finished, and set status to WAITING.
421 """
422- job = Job(_status=JobStatus.RUNNING)
423+ job = Job(status=JobStatus.RUNNING)
424 self.assertEqual(None, job.date_finished)
425 job.queue()
426 self.assertNotEqual(None, job.date_finished)
427@@ -178,22 +185,22 @@
428
429 def test_queue_when_completed_is_invalid(self):
430 """When a job is completed, attempting to queue is invalid."""
431- job = Job(_status=JobStatus.COMPLETED)
432+ job = prepare_job(JobStatus.COMPLETED)
433 self.assertRaises(InvalidTransition, job.queue)
434
435 def test_queue_when_waiting_is_invalid(self):
436 """When a job is waiting, attempting to queue is invalid."""
437- job = Job(_status=JobStatus.WAITING)
438+ job = prepare_job(JobStatus.WAITING)
439 self.assertRaises(InvalidTransition, job.queue)
440
441 def test_queue_when_failed_is_invalid(self):
442 """When a job is failed, attempting to queue is invalid."""
443- job = Job(_status=JobStatus.FAILED)
444+ job = prepare_job(JobStatus.FAILED)
445 self.assertRaises(InvalidTransition, job.queue)
446
447 def test_suspend(self):
448 """A job that is in the WAITING state can be suspended."""
449- job = Job(_status=JobStatus.WAITING)
450+ job = prepare_job(JobStatus.WAITING)
451 job.suspend()
452 self.assertEqual(
453 job.status,
454@@ -201,23 +208,23 @@
455
456 def test_suspend_when_running(self):
457 """When a job is running, attempting to suspend is valid."""
458- job = Job(_status=JobStatus.RUNNING)
459+ job = Job(status=JobStatus.RUNNING)
460 job.suspend()
461 self.assertEqual(JobStatus.SUSPENDED, job.status)
462
463 def test_suspend_when_completed(self):
464 """When a job is completed, attempting to suspend is invalid."""
465- job = Job(_status=JobStatus.COMPLETED)
466+ job = prepare_job(JobStatus.COMPLETED)
467 self.assertRaises(InvalidTransition, job.suspend)
468
469 def test_suspend_when_failed(self):
470 """When a job is failed, attempting to suspend is invalid."""
471- job = Job(_status=JobStatus.FAILED)
472+ job = prepare_job(JobStatus.FAILED)
473 self.assertRaises(InvalidTransition, job.suspend)
474
475 def test_resume(self):
476 """A job that is suspended can be resumed."""
477- job = Job(_status=JobStatus.SUSPENDED)
478+ job = Job(status=JobStatus.SUSPENDED)
479 job.resume()
480 self.assertEqual(
481 job.status,
482@@ -225,30 +232,30 @@
483
484 def test_resume_clears_lease_expiry(self):
485 """A job that resumes should null out the lease_expiry."""
486- job = Job(_status=JobStatus.SUSPENDED)
487+ job = Job(status=JobStatus.SUSPENDED)
488 job.lease_expires = UTC_NOW
489 job.resume()
490 self.assertIs(None, job.lease_expires)
491
492 def test_resume_when_running(self):
493 """When a job is running, attempting to resume is invalid."""
494- job = Job(_status=JobStatus.RUNNING)
495+ job = Job(status=JobStatus.RUNNING)
496 self.assertRaises(InvalidTransition, job.resume)
497
498 def test_resume_when_completed(self):
499 """When a job is completed, attempting to resume is invalid."""
500- job = Job(_status=JobStatus.COMPLETED)
501+ job = prepare_job(JobStatus.COMPLETED)
502 self.assertRaises(InvalidTransition, job.resume)
503
504 def test_resume_when_failed(self):
505 """When a job is failed, attempting to resume is invalid."""
506- job = Job(_status=JobStatus.FAILED)
507+ job = prepare_job(JobStatus.FAILED)
508 self.assertRaises(InvalidTransition, job.resume)
509
510 def test_is_pending(self):
511 """is_pending is True when the job can possibly complete."""
512 for status in JobStatus.items:
513- job = Job(_status=status)
514+ job = prepare_job(status)
515 self.assertEqual(
516 status in Job.PENDING_STATUSES, job.is_pending)
517
518@@ -272,7 +279,7 @@
519 def test_ready_jobs_started(self):
520 """Job.ready_jobs should not jobs that have been started."""
521 preexisting = self._sampleData()
522- job = Job(_status=JobStatus.RUNNING)
523+ job = Job(status=JobStatus.RUNNING)
524 self.assertEqual(
525 preexisting, list(Store.of(job).execute(Job.ready_jobs)))
526
527@@ -340,3 +347,46 @@
528 job = Job()
529 job.acquireLease(-300)
530 self.assertEqual(0, job.getTimeout())
531+
532+ def test_attribute_access(self):
533+ # Job's security proxy allows anyone to its attributes, but no one may
534+ # set them.
535+ job = ProxyFactory(Job())
536+
537+ def can_read(name):
538+ try:
539+ getattr(job, name)
540+ except ForbiddenAttribute:
541+ return False
542+ else:
543+ return True
544+
545+ def can_write(name):
546+ try:
547+ setattr(job, name, None)
548+ except ForbiddenAttribute:
549+ return False
550+ else:
551+ return True
552+
553+ attributes = set(
554+ name for name in IJob
555+ if isinstance(IJob[name], Attribute)
556+ and not isinstance(IJob[name], Method))
557+
558+ expected = dict.fromkeys(attributes, "r")
559+
560+ observed = {}
561+ for attribute in attributes:
562+ read = can_read(attribute)
563+ write = can_write(attribute)
564+ if read and write:
565+ observed[attribute] = "rw"
566+ elif read:
567+ observed[attribute] = "r"
568+ elif write:
569+ observed[attribute] = "w"
570+ else:
571+ observed[attribute] = "-"
572+
573+ self.assertEqual(expected, observed)
574
575=== modified file 'lib/lp/soyuz/model/distroseriesdifferencejob.py'
576--- lib/lp/soyuz/model/distroseriesdifferencejob.py 2011-09-09 19:34:10 +0000
577+++ lib/lp/soyuz/model/distroseriesdifferencejob.py 2011-10-07 13:40:29 +0000
578@@ -255,7 +255,7 @@
579 DistributionJob,
580 DistributionJob.job_type == cls.class_job_type,
581 Job.id == DistributionJob.job_id,
582- Job._status.is_in(Job.PENDING_STATUSES),
583+ Job.status.is_in(Job.PENDING_STATUSES),
584 DistributionJob.distroseries == derived_series)
585
586 parent_series_ids = set(
587
588=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
589--- lib/lp/soyuz/model/packagecopyjob.py 2011-09-13 13:41:36 +0000
590+++ lib/lp/soyuz/model/packagecopyjob.py 2011-10-07 13:40:29 +0000
591@@ -325,7 +325,7 @@
592 PackageCopyJob.job_type == cls.class_job_type,
593 PackageCopyJob.target_archive == target_archive,
594 Job.id == PackageCopyJob.job_id,
595- Job._status == JobStatus.WAITING)
596+ Job.status == JobStatus.WAITING)
597 jobs = jobs.order_by(PackageCopyJob.id)
598 return DecoratedResultSet(jobs, cls)
599
600@@ -337,7 +337,7 @@
601 Job.id == PackageCopyJob.job_id,
602 PackageCopyJob.job_type == cls.class_job_type,
603 PackageCopyJob.target_distroseries == target_series,
604- Job._status.is_in(Job.PENDING_STATUSES))
605+ Job.status.is_in(Job.PENDING_STATUSES))
606 raw_jobs = raw_jobs.order_by(PackageCopyJob.id)
607 return DecoratedResultSet(raw_jobs, cls)
608
609
610=== modified file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
611--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 2011-08-31 14:12:38 +0000
612+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 2011-10-07 13:40:29 +0000
613@@ -27,6 +27,7 @@
614 from lp.services.database import bulk
615 from lp.services.features.testing import FeatureFixture
616 from lp.services.job.interfaces.job import JobStatus
617+from lp.services.job.testing import prepare_job
618 from lp.soyuz.enums import (
619 ArchivePurpose,
620 PackagePublishingStatus,
621@@ -506,7 +507,7 @@
622 dsd = self.factory.makeDistroSeriesDifference()
623 job = create_job(
624 dsd.derived_series, dsd.source_package_name, dsd.parent_series)
625- removeSecurityProxy(job).job._status = JobStatus.COMPLETED
626+ prepare_job(JobStatus.COMPLETED, removeSecurityProxy(job))
627 self.assertEqual(
628 {},
629 self.getJobSource().getPendingJobsForDifferences(
630
631=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
632--- lib/lp/soyuz/tests/test_packagecopyjob.py 2011-08-30 14:24:13 +0000
633+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2011-10-07 13:40:29 +0000
634@@ -33,6 +33,8 @@
635 JobStatus,
636 SuspendJobException,
637 )
638+from lp.services.job.model.job import Job
639+from lp.services.job.testing import prepare_job
640 from lp.soyuz.adapters.overrides import SourceOverride
641 from lp.soyuz.enums import (
642 ArchivePurpose,
643@@ -234,7 +236,7 @@
644 def test_getActiveJobs_only_returns_waiting_jobs(self):
645 # getActiveJobs ignores jobs that aren't in the WAITING state.
646 job = self.makeJob()
647- removeSecurityProxy(job).job._status = JobStatus.RUNNING
648+ removeSecurityProxy(job).job.status = JobStatus.RUNNING
649 source = getUtility(IPlainPackageCopyJobSource)
650 self.assertContentEqual([], source.getActiveJobs(job.target_archive))
651
652@@ -480,21 +482,16 @@
653 def test_getPendingJobsPerPackage_only_returns_pending_jobs(self):
654 # getPendingJobsPerPackage ignores jobs that have already been
655 # run.
656- dsd = self.factory.makeDistroSeriesDifference()
657- job = self.makeJob(dsd)
658+ series = self.factory.makeDistroSeries()
659 job_source = getUtility(IPlainPackageCopyJobSource)
660- found_by_state = {}
661 for status in JobStatus.items:
662- removeSecurityProxy(job).job._status = status
663- result = job_source.getPendingJobsPerPackage(dsd.derived_series)
664- if len(result) > 0:
665- found_by_state[status] = result[dsd.source_package_name.name]
666- expected = {
667- JobStatus.WAITING: job,
668- JobStatus.RUNNING: job,
669- JobStatus.SUSPENDED: job,
670- }
671- self.assertEqual(expected, found_by_state)
672+ dsd = self.factory.makeDistroSeriesDifference(
673+ derived_series=series)
674+ job = self.makeJob(dsd)
675+ prepare_job(status, removeSecurityProxy(job))
676+ jobs = job_source.getPendingJobsPerPackage(series)
677+ statuses = set(job.status for job in jobs.itervalues())
678+ self.assertEqual(Job.PENDING_STATUSES, statuses)
679
680 def test_getPendingJobsPerPackage_distinguishes_jobs(self):
681 # getPendingJobsPerPackage associates the right job with the
682
683=== modified file 'lib/lp/translations/model/translationsharingjob.py'
684--- lib/lp/translations/model/translationsharingjob.py 2011-08-01 08:12:35 +0000
685+++ lib/lp/translations/model/translationsharingjob.py 2011-10-07 13:40:29 +0000
686@@ -232,7 +232,7 @@
687 (TranslationSharingJob.productseries_id ==
688 packaging.productseries.id),
689 TranslationSharingJob.job_type == cls.class_job_type,
690- Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
691+ Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
692 result.order_by(TranslationSharingJob.id)
693 job = result.first()
694 if job is None: