Merge lp:~rockstar/launchpad/use-suspended-job-status into lp:launchpad
- use-suspended-job-status
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Jeroen T. Vermeulen |
Approved revision: | not available |
Merged at revision: | not available |
Proposed branch: | lp:~rockstar/launchpad/use-suspended-job-status |
Merge into: | lp:launchpad |
Prerequisite: | lp:~rockstar/launchpad/job-suspended-status |
Diff against target: |
258 lines (+175/-3) 4 files modified
lib/lp/soyuz/configure.zcml (+1/-1) lib/lp/soyuz/interfaces/archive.py (+6/-0) lib/lp/soyuz/model/archive.py (+35/-1) lib/lp/soyuz/tests/test_archive.py (+133/-1) |
To merge this branch: | bzr merge lp:~rockstar/launchpad/use-suspended-job-status |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | code | Approve | |
Review via email: mp+17191@code.launchpad.net |
Commit message
Description of the change
Paul Hummer (rockstar) wrote : | # |
Jeroen T. Vermeulen (jtv) wrote : | # |
Went over this in IRL.
Some small superficial points—mainly indentation of query text, and XXXes to factor duplicated setup out into helpers.
Also, instead of a test method "assert that there are no builds with status X" it's probably more helpful to have one that returns whether there are any builds with status X, and use that for assertions directly in your tests. Two advantages:
* A failure shows up at the bottom of its stack trace, not in a generic method where you have to browse further up the trace to get to the semantic failure.
* You can show before/after comparisons very effectively. That can help separate bad test setup (which also sometimes tells you what you want to hear) from the change you're really testing for.
With those notes, r=me.
Jeroen
Preview Diff
1 | === modified file 'lib/lp/soyuz/configure.zcml' | |||
2 | --- lib/lp/soyuz/configure.zcml 2010-01-07 05:32:58 +0000 | |||
3 | +++ lib/lp/soyuz/configure.zcml 2010-01-12 04:24:18 +0000 | |||
4 | @@ -401,7 +401,7 @@ | |||
5 | 401 | <require | 401 | <require |
6 | 402 | permission="launchpad.Edit" | 402 | permission="launchpad.Edit" |
7 | 403 | interface="lp.soyuz.interfaces.archive.IArchiveEdit" | 403 | interface="lp.soyuz.interfaces.archive.IArchiveEdit" |
9 | 404 | set_attributes="description displayname enabled"/> | 404 | set_attributes="description displayname"/> |
10 | 405 | <require | 405 | <require |
11 | 406 | permission="launchpad.Commercial" | 406 | permission="launchpad.Commercial" |
12 | 407 | set_attributes="authorized_size buildd_secret | 407 | set_attributes="authorized_size buildd_secret |
13 | 408 | 408 | ||
14 | === modified file 'lib/lp/soyuz/interfaces/archive.py' | |||
15 | --- lib/lp/soyuz/interfaces/archive.py 2009-12-08 15:40:23 +0000 | |||
16 | +++ lib/lp/soyuz/interfaces/archive.py 2010-01-12 04:24:18 +0000 | |||
17 | @@ -1084,6 +1084,12 @@ | |||
18 | 1084 | :param component: An `IComponent` or textual component name. | 1084 | :param component: An `IComponent` or textual component name. |
19 | 1085 | """ | 1085 | """ |
20 | 1086 | 1086 | ||
21 | 1087 | def enable(): | ||
22 | 1088 | """Enable the archive.""" | ||
23 | 1089 | |||
24 | 1090 | def disable(): | ||
25 | 1091 | """Disable the archive.""" | ||
26 | 1092 | |||
27 | 1087 | 1093 | ||
28 | 1088 | class IArchive(IArchivePublic, IArchiveAppend, IArchiveEdit, IArchiveView): | 1094 | class IArchive(IArchivePublic, IArchiveAppend, IArchiveEdit, IArchiveView): |
29 | 1089 | """Main Archive interface.""" | 1095 | """Main Archive interface.""" |
30 | 1090 | 1096 | ||
31 | === modified file 'lib/lp/soyuz/model/archive.py' | |||
32 | --- lib/lp/soyuz/model/archive.py 2009-12-24 08:15:46 +0000 | |||
33 | +++ lib/lp/soyuz/model/archive.py 2010-01-12 04:24:18 +0000 | |||
34 | @@ -31,6 +31,7 @@ | |||
35 | 31 | from canonical.database.enumcol import EnumCol | 31 | from canonical.database.enumcol import EnumCol |
36 | 32 | from canonical.database.sqlbase import ( | 32 | from canonical.database.sqlbase import ( |
37 | 33 | cursor, quote, quote_like, sqlvalues, SQLBase) | 33 | cursor, quote, quote_like, sqlvalues, SQLBase) |
38 | 34 | from lp.services.job.interfaces.job import JobStatus | ||
39 | 34 | from lp.soyuz.adapters.packagelocation import PackageLocation | 35 | from lp.soyuz.adapters.packagelocation import PackageLocation |
40 | 35 | from canonical.launchpad.components.tokens import ( | 36 | from canonical.launchpad.components.tokens import ( |
41 | 36 | create_unique_token_for_table) | 37 | create_unique_token_for_table) |
42 | @@ -130,7 +131,8 @@ | |||
43 | 130 | purpose = EnumCol( | 131 | purpose = EnumCol( |
44 | 131 | dbName='purpose', unique=False, notNull=True, schema=ArchivePurpose) | 132 | dbName='purpose', unique=False, notNull=True, schema=ArchivePurpose) |
45 | 132 | 133 | ||
47 | 133 | enabled = BoolCol(dbName='enabled', notNull=True, default=True) | 134 | _enabled = BoolCol(dbName='enabled', notNull=True, default=True) |
48 | 135 | enabled = property(lambda x: x._enabled) | ||
49 | 134 | 136 | ||
50 | 135 | publish = BoolCol(dbName='publish', notNull=True, default=True) | 137 | publish = BoolCol(dbName='publish', notNull=True, default=True) |
51 | 136 | 138 | ||
52 | @@ -1238,6 +1240,38 @@ | |||
53 | 1238 | result_set.config(distinct=True).order_by(SourcePackageRelease.id) | 1240 | result_set.config(distinct=True).order_by(SourcePackageRelease.id) |
54 | 1239 | return result_set | 1241 | return result_set |
55 | 1240 | 1242 | ||
56 | 1243 | def _setBuildStatuses(self, status): | ||
57 | 1244 | """Update the pending Build Jobs' status for this archive.""" | ||
58 | 1245 | |||
59 | 1246 | query = """ | ||
60 | 1247 | UPDATE Job SET status = %s | ||
61 | 1248 | FROM Build, BuildPackageJob, BuildQueue | ||
62 | 1249 | WHERE | ||
63 | 1250 | -- insert self.id here | ||
64 | 1251 | Build.archive = %s | ||
65 | 1252 | AND BuildPackageJob.build = Build.id | ||
66 | 1253 | AND BuildPackageJob.job = BuildQueue.job | ||
67 | 1254 | AND Job.id = BuildQueue.job | ||
68 | 1255 | -- Build is in state BuildStatus.NEEDSBUILD (0) | ||
69 | 1256 | AND Build.buildstate = %s; | ||
70 | 1257 | """ % sqlvalues(status, self, BuildStatus.NEEDSBUILD) | ||
71 | 1258 | |||
72 | 1259 | store = Store.of(self) | ||
73 | 1260 | store.execute(query) | ||
74 | 1261 | |||
75 | 1262 | def enable(self): | ||
76 | 1263 | """See `IArchive`.""" | ||
77 | 1264 | assert self._enabled == False, "This archive is already enabled." | ||
78 | 1265 | self._enabled = True | ||
79 | 1266 | self._setBuildStatuses(JobStatus.WAITING) | ||
80 | 1267 | |||
81 | 1268 | def disable(self): | ||
82 | 1269 | """See `IArchive`.""" | ||
83 | 1270 | assert self._enabled == True, "This archive is already disabled." | ||
84 | 1271 | self._enabled = False | ||
85 | 1272 | self._setBuildStatuses(JobStatus.SUSPENDED) | ||
86 | 1273 | |||
87 | 1274 | |||
88 | 1241 | class ArchiveSet: | 1275 | class ArchiveSet: |
89 | 1242 | implements(IArchiveSet) | 1276 | implements(IArchiveSet) |
90 | 1243 | title = "Archives registered in Launchpad" | 1277 | title = "Archives registered in Launchpad" |
91 | 1244 | 1278 | ||
92 | === modified file 'lib/lp/soyuz/tests/test_archive.py' | |||
93 | --- lib/lp/soyuz/tests/test_archive.py 2009-11-06 21:06:38 +0000 | |||
94 | +++ lib/lp/soyuz/tests/test_archive.py 2010-01-12 04:24:18 +0000 | |||
95 | @@ -3,24 +3,30 @@ | |||
96 | 3 | 3 | ||
97 | 4 | """Test Archive features.""" | 4 | """Test Archive features.""" |
98 | 5 | 5 | ||
100 | 6 | from datetime import datetime | 6 | from datetime import datetime, timedelta |
101 | 7 | import pytz | 7 | import pytz |
102 | 8 | import unittest | 8 | import unittest |
103 | 9 | 9 | ||
104 | 10 | from zope.component import getUtility | 10 | from zope.component import getUtility |
105 | 11 | from zope.security.proxy import removeSecurityProxy | 11 | from zope.security.proxy import removeSecurityProxy |
106 | 12 | 12 | ||
107 | 13 | from canonical.database.sqlbase import sqlvalues | ||
108 | 14 | from canonical.launchpad.webapp.interfaces import ( | ||
109 | 15 | IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR) | ||
110 | 13 | from canonical.testing import LaunchpadZopelessLayer | 16 | from canonical.testing import LaunchpadZopelessLayer |
111 | 14 | 17 | ||
112 | 15 | from lp.registry.interfaces.distribution import IDistributionSet | 18 | from lp.registry.interfaces.distribution import IDistributionSet |
113 | 16 | from lp.registry.interfaces.person import IPersonSet | 19 | from lp.registry.interfaces.person import IPersonSet |
114 | 20 | from lp.services.job.interfaces.job import JobStatus | ||
115 | 17 | from lp.soyuz.interfaces.archive import IArchiveSet, ArchivePurpose | 21 | from lp.soyuz.interfaces.archive import IArchiveSet, ArchivePurpose |
116 | 18 | from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat | 22 | from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat |
117 | 19 | from lp.soyuz.interfaces.build import BuildStatus | 23 | from lp.soyuz.interfaces.build import BuildStatus |
118 | 20 | from lp.soyuz.interfaces.publishing import PackagePublishingStatus | 24 | from lp.soyuz.interfaces.publishing import PackagePublishingStatus |
119 | 25 | from lp.soyuz.model.build import Build | ||
120 | 21 | from lp.soyuz.tests.test_publishing import SoyuzTestPublisher | 26 | from lp.soyuz.tests.test_publishing import SoyuzTestPublisher |
121 | 22 | from lp.testing import TestCaseWithFactory | 27 | from lp.testing import TestCaseWithFactory |
122 | 23 | 28 | ||
123 | 29 | |||
124 | 24 | class TestGetPublicationsInArchive(TestCaseWithFactory): | 30 | class TestGetPublicationsInArchive(TestCaseWithFactory): |
125 | 25 | 31 | ||
126 | 26 | layer = LaunchpadZopelessLayer | 32 | layer = LaunchpadZopelessLayer |
127 | @@ -382,5 +388,131 @@ | |||
128 | 382 | self.assertIs( | 388 | self.assertIs( |
129 | 383 | self.primary_archive.debug_archive, None) | 389 | self.primary_archive.debug_archive, None) |
130 | 384 | 390 | ||
131 | 391 | |||
132 | 392 | class TestArchiveEnableDisable(TestCaseWithFactory): | ||
133 | 393 | """Test the enable and disable methods of Archive.""" | ||
134 | 394 | |||
135 | 395 | layer = LaunchpadZopelessLayer | ||
136 | 396 | |||
137 | 397 | def setUp(self): | ||
138 | 398 | #XXX: rockstar - 12 Jan 2010 - Bug #506255 - Tidy up these tests! | ||
139 | 399 | super(TestArchiveEnableDisable, self).setUp() | ||
140 | 400 | |||
141 | 401 | self.publisher = SoyuzTestPublisher() | ||
142 | 402 | self.publisher.prepareBreezyAutotest() | ||
143 | 403 | |||
144 | 404 | self.ubuntutest = getUtility(IDistributionSet)['ubuntutest'] | ||
145 | 405 | self.archive = getUtility(IArchiveSet).getByDistroPurpose( | ||
146 | 406 | self.ubuntutest, ArchivePurpose.PRIMARY) | ||
147 | 407 | |||
148 | 408 | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) | ||
149 | 409 | sample_data = store.find(Build) | ||
150 | 410 | for build in sample_data: | ||
151 | 411 | build.buildstate = BuildStatus.FULLYBUILT | ||
152 | 412 | store.flush() | ||
153 | 413 | |||
154 | 414 | # We test builds that target a primary archive. | ||
155 | 415 | self.builds = [] | ||
156 | 416 | self.builds.extend( | ||
157 | 417 | self.publisher.getPubSource( | ||
158 | 418 | sourcename="gedit", status=PackagePublishingStatus.PUBLISHED, | ||
159 | 419 | archive=self.archive).createMissingBuilds()) | ||
160 | 420 | self.builds.extend( | ||
161 | 421 | self.publisher.getPubSource( | ||
162 | 422 | sourcename="firefox", | ||
163 | 423 | status=PackagePublishingStatus.PUBLISHED, | ||
164 | 424 | archive=self.archive).createMissingBuilds()) | ||
165 | 425 | self.builds.extend( | ||
166 | 426 | self.publisher.getPubSource( | ||
167 | 427 | sourcename="apg", status=PackagePublishingStatus.PUBLISHED, | ||
168 | 428 | archive=self.archive).createMissingBuilds()) | ||
169 | 429 | self.builds.extend( | ||
170 | 430 | self.publisher.getPubSource( | ||
171 | 431 | sourcename="vim", status=PackagePublishingStatus.PUBLISHED, | ||
172 | 432 | archive=self.archive).createMissingBuilds()) | ||
173 | 433 | self.builds.extend( | ||
174 | 434 | self.publisher.getPubSource( | ||
175 | 435 | sourcename="gcc", status=PackagePublishingStatus.PUBLISHED, | ||
176 | 436 | archive=self.archive).createMissingBuilds()) | ||
177 | 437 | self.builds.extend( | ||
178 | 438 | self.publisher.getPubSource( | ||
179 | 439 | sourcename="bison", status=PackagePublishingStatus.PUBLISHED, | ||
180 | 440 | archive=self.archive).createMissingBuilds()) | ||
181 | 441 | self.builds.extend( | ||
182 | 442 | self.publisher.getPubSource( | ||
183 | 443 | sourcename="flex", status=PackagePublishingStatus.PUBLISHED, | ||
184 | 444 | archive=self.archive).createMissingBuilds()) | ||
185 | 445 | self.builds.extend( | ||
186 | 446 | self.publisher.getPubSource( | ||
187 | 447 | sourcename="postgres", | ||
188 | 448 | status=PackagePublishingStatus.PUBLISHED, | ||
189 | 449 | archive=self.archive).createMissingBuilds()) | ||
190 | 450 | # Set up the builds for test. | ||
191 | 451 | score = 1000 | ||
192 | 452 | duration = 0 | ||
193 | 453 | for build in self.builds: | ||
194 | 454 | score += 1 | ||
195 | 455 | duration += 60 | ||
196 | 456 | bq = build.buildqueue_record | ||
197 | 457 | bq.lastscore = score | ||
198 | 458 | bq.estimated_duration = timedelta(seconds=duration) | ||
199 | 459 | |||
200 | 460 | def _getBuildJobsByStatus(self, archive, status): | ||
201 | 461 | # Return the count for archive build jobs with the given status. | ||
202 | 462 | query = """ | ||
203 | 463 | SELECT COUNT(Job.id) | ||
204 | 464 | FROM Build, BuildPackageJob, BuildQueue, Job | ||
205 | 465 | WHERE | ||
206 | 466 | Build.archive = %s | ||
207 | 467 | AND BuildPackageJob.build = Build.id | ||
208 | 468 | AND BuildPackageJob.job = BuildQueue.job | ||
209 | 469 | AND Job.id = BuildQueue.job | ||
210 | 470 | AND Build.buildstate = %s | ||
211 | 471 | AND Job.status = %s; | ||
212 | 472 | """ % sqlvalues(archive, BuildStatus.NEEDSBUILD, status) | ||
213 | 473 | |||
214 | 474 | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) | ||
215 | 475 | return store.execute(query).get_one()[0] | ||
216 | 476 | |||
217 | 477 | def assertNoBuildJobsHaveStatus(self, archive, status): | ||
218 | 478 | # Check that that the jobs attached to this archive do not have this | ||
219 | 479 | # status. | ||
220 | 480 | self.assertEqual(self._getBuildJobsByStatus(archive, status), 0) | ||
221 | 481 | |||
222 | 482 | def assertHasBuildJobsWithStatus(self, archive, status): | ||
223 | 483 | # Check that that there are jobs attached to this archive that have | ||
224 | 484 | # the specified status. | ||
225 | 485 | self.assertEqual(self._getBuildJobsByStatus(archive, status), 8) | ||
226 | 486 | |||
227 | 487 | def test_enableArchive(self): | ||
228 | 488 | # Enabling an archive should set all the Archive's suspended builds to | ||
229 | 489 | # WAITING. | ||
230 | 490 | |||
231 | 491 | # Disable the archive, because it's currently enabled. | ||
232 | 492 | self.archive.disable() | ||
233 | 493 | self.assertHasBuildJobsWithStatus(self.archive, JobStatus.SUSPENDED) | ||
234 | 494 | self.archive.enable() | ||
235 | 495 | self.assertNoBuildJobsHaveStatus(self.archive, JobStatus.SUSPENDED) | ||
236 | 496 | self.assertTrue(self.archive.enabled) | ||
237 | 497 | |||
238 | 498 | def test_enableArchiveAlreadyEnabled(self): | ||
239 | 499 | # Enabling an already enabled Archive should raise an AssertionError. | ||
240 | 500 | self.assertRaises(AssertionError, self.archive.enable) | ||
241 | 501 | |||
242 | 502 | def test_disableArchive(self): | ||
243 | 503 | # Disabling an archive should set all the Archive's pending bulds to | ||
244 | 504 | # SUSPENDED. | ||
245 | 505 | self.assertHasBuildJobsWithStatus(self.archive, JobStatus.WAITING) | ||
246 | 506 | self.archive.disable() | ||
247 | 507 | self.assertNoBuildJobsHaveStatus(self.archive, JobStatus.WAITING) | ||
248 | 508 | self.assertFalse(self.archive.enabled) | ||
249 | 509 | |||
250 | 510 | def test_disableArchiveAlreadyDisabled(self): | ||
251 | 511 | # Disabling an already disabled Archive should raise an | ||
252 | 512 | # AssertionError. | ||
253 | 513 | self.archive.disable() | ||
254 | 514 | self.assertRaises(AssertionError, self.archive.disable) | ||
255 | 515 | |||
256 | 516 | |||
257 | 385 | def test_suite(): | 517 | def test_suite(): |
258 | 386 | return unittest.TestLoader().loadTestsFromName(__name__) | 518 | return unittest.TestLoader().loadTestsFromName(__name__) |
Hi Jeroen-
This branch just adds an IArchive.enable and IArchive.disable method to the
IArchive to appropriately tests it.
Paul