launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04026
[Merge] lp:~jtv/launchpad/bug-799680 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-799680 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #799680 in Launchpad itself: "Queue tool/page should show that a waiting package is a sync"
https://bugs.launchpad.net/launchpad/+bug/799680
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-799680/+merge/65495
= Summary =
Show the architecture for copy-job package upload as "sync," rather than falling back to "source" like we originally chose to do.
This shows up in two places: the "displayarchs" go on the +queue page, and a tag like "SB" (for "source & builds") shows up in output of the queue.py script.
We chose "X" as the marker for copy-job uploads. Why not "C"? Because we refer to these jobs as "syncs," and S was already taken. I padded it to keep the same format as we already have — the script is quite finicky about layout.
== Implementation details ==
I extracted generation of the tag, both to make the code more readable and to facilitate unit-testing.
You'll also note a slew of removed tests. These were duplicated in another copy of the same test case, all with the same names! So most of the tests weren't in fact getting run.
The copies that I kept, in the larger test case, had more modern helpers to create uploads of the desired types.
== Tests ==
{{{
./bin/test -vvc lp.soyuz.browser.tests.test_queue
./bin/test -vvc lp.soyuz -t xx-queue-pages
./bin/test -vvc lp.soyuz.scripts.tests.test_queue
}}}
== Demo and Q/A ==
A sync upload on the DistroSeries:+queue page should show up with "(sync)" as its architecture; the queue.py script should show it with the "X-" tag.
The entry on the +queue page still won't have a working "unfold" triangle, and its section/component still won't show up.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/model/queue.py
lib/lp/soyuz/scripts/tests/test_queue.py
lib/lp/soyuz/tests/test_packageupload.py
lib/lp/soyuz/scripts/queue.py
--
https://code.launchpad.net/~jtv/launchpad/bug-799680/+merge/65495
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-799680 into lp:launchpad.
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2011-06-16 10:43:48 +0000
+++ lib/lp/soyuz/model/queue.py 2011-06-22 13:17:30 +0000
@@ -662,7 +662,9 @@
def displayarchs(self):
"""See `IPackageUpload`"""
archs = []
- if self.contains_source or self.package_copy_job_id is not None:
+ if self.package_copy_job_id is not None:
+ archs.append('sync')
+ if self.contains_source:
archs.append('source')
for queue_build in self.builds:
archs.append(queue_build.build.distro_arch_series.architecturetag)
=== modified file 'lib/lp/soyuz/scripts/queue.py'
--- lib/lp/soyuz/scripts/queue.py 2011-06-15 07:54:46 +0000
+++ lib/lp/soyuz/scripts/queue.py 2011-06-22 13:17:30 +0000
@@ -235,32 +235,50 @@
self.display(self.__doc__)
raise QueueActionError(extended_info)
+ def _makeTag(self, queue_item):
+ """Compose an upload type tag for `queue_item`.
+
+ A source upload without binaries is tagged as "S-".
+ A binary upload without source is tagged as "-B."
+ An upload with both source and binaries is tagged as "SB".
+ An upload with a package copy job is tagged as "X-".
+ """
+ # XXX cprov 2006-07-31: source_tag and build_tag ('S' & 'B')
+ # are necessary simply to keep the format legaxy.
+ # We may discuss a more reasonable output format later
+ # and avoid extra boring code. The IDRQ.displayname should
+ # do should be enough.
+ if queue_item.package_copy_job is not None:
+ return "X-"
+
+ source_tag = {
+ True: 'S',
+ False: '-',
+ }
+ binary_tag = {
+ True: 'B',
+ False: '-',
+ }
+ return (
+ source_tag[bool(queue_item.contains_source)] +
+ binary_tag[bool(queue_item.contains_build)])
+
def displayItem(self, queue_item):
"""Display one line summary of the queue item provided."""
- source_tag = '-'
- build_tag = '-'
+ tag = self._makeTag(queue_item)
displayname = queue_item.displayname
version = queue_item.displayversion
age = DurationFormatterAPI(
datetime.now(pytz.timezone('UTC')) -
queue_item.date_created).approximateduration()
- # XXX cprov 2006-07-31: source_tag and build_tag ('S' & 'B')
- # are necessary simply to keep the format legaxy.
- # We may discuss a more reasonable output format later
- # and avoid extra boring code. The IDRQ.displayname should
- # do should be enough.
- if queue_item.contains_source:
- source_tag = 'S'
if queue_item.contains_build:
- build_tag = 'B'
displayname = "%s (%s)" % (queue_item.displayname,
queue_item.displayarchs)
- self.display("%8d | %s%s | %s | %s | %s" %
- (queue_item.id, source_tag, build_tag,
- displayname.ljust(20)[:20], version.ljust(20)[:20],
- age))
+ self.display("%8d | %s | %s | %s | %s" %
+ (queue_item.id, tag, displayname.ljust(20)[:20],
+ version.ljust(20)[:20], age))
def displayInfo(self, queue_item, only=None):
"""Displays additional information about the provided queue item.
=== modified file 'lib/lp/soyuz/scripts/tests/test_queue.py'
--- lib/lp/soyuz/scripts/tests/test_queue.py 2011-06-16 10:29:59 +0000
+++ lib/lp/soyuz/scripts/tests/test_queue.py 2011-06-22 13:17:30 +0000
@@ -10,6 +10,7 @@
import shutil
from StringIO import StringIO
import tempfile
+from testtools.matchers import StartsWith
from unittest import TestCase
from zope.component import getUtility
@@ -960,6 +961,14 @@
distro.name, suite, queue, terms, component.name,
section.name, priority_name, display)
+ def parseUploadSummaryLine(self, output_line):
+ """Parse an output line from `QueueAction.displayItem`.
+
+ :param output_line: A line of output text from `displayItem`.
+ :return: A tuple of displayed items: (id, tag, name, version, age).
+ """
+ return tuple(item.strip() for item in output_line.split('|'))
+
def test_display_actions_have_privileges_for_PackageCopyJob(self):
# The methods that display uploads have privileges to work with
# a PackageUpload that has a copy job.
@@ -994,15 +1003,52 @@
# displayItem can display a source package upload.
upload = self.factory.makeSourcePackageUpload()
action = self.makeQueueAction(upload)
+
action.displayItem(upload)
- self.assertNotEqual(0, action.display.call_count)
+
+ ((output, ), kwargs) = action.display.calls[0]
+ (upload_id, tag, name, version, age) = self.parseUploadSummaryLine(
+ output)
+ self.assertEqual(str(upload.id), upload_id)
+ self.assertEqual("S-", tag)
+ self.assertThat(upload.displayname, StartsWith(name))
+ self.assertThat(upload.package_version, StartsWith(version))
def test_displayItem_displays_PackageUpload_with_PackageCopyJob(self):
# displayItem can display a copy-job package upload.
upload = self.factory.makeCopyJobPackageUpload()
action = self.makeQueueAction(upload)
+
action.displayItem(upload)
- self.assertNotEqual(0, action.display.call_count)
+
+ ((output, ), kwargs) = action.display.calls[0]
+ (upload_id, tag, name, version, age) = self.parseUploadSummaryLine(
+ output)
+ self.assertEqual(str(upload.id), upload_id)
+ self.assertEqual("X-", tag)
+ self.assertThat(upload.displayname, StartsWith(name))
+ self.assertThat(upload.package_version, StartsWith(version))
+
+ def test_makeTag_returns_S_for_source_upload(self):
+ upload = self.factory.makeSourcePackageUpload()
+ self.assertEqual('S-', self.makeQueueAction(upload)._makeTag(upload))
+
+ def test_makeTag_returns_B_for_binary_upload(self):
+ upload = self.factory.makeBuildPackageUpload()
+ self.assertEqual('-B', self.makeQueueAction(upload)._makeTag(upload))
+
+ def test_makeTag_returns_SB_for_mixed_upload(self):
+ upload = self.factory.makeSourcePackageUpload()
+ upload.addBuild(self.factory.makeBinaryPackageBuild())
+ self.assertEqual('SB', self.makeQueueAction(upload)._makeTag(upload))
+
+ def test_makeTag_returns_X_for_copy_job_upload(self):
+ upload = self.factory.makeCopyJobPackageUpload()
+ self.assertEqual('X-', self.makeQueueAction(upload)._makeTag(upload))
+
+ def test_makeTag_returns_dashes_for_custom_upload(self):
+ upload = self.factory.makeCustomPackageUpload()
+ self.assertEqual('--', self.makeQueueAction(upload)._makeTag(upload))
def test_displayInfo_displays_PackageUpload_with_source(self):
# displayInfo can display a source package upload.
=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py 2011-06-16 13:14:49 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py 2011-06-22 13:17:30 +0000
@@ -8,7 +8,6 @@
import os
import shutil
-from storm.store import Store
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -443,10 +442,10 @@
self.assertEqual(job.package_name, upload.package_name)
self.assertEqual(job.package_version, upload.package_version)
- def test_displayarchs_for_copy_job_is_source(self):
+ def test_displayarchs_for_copy_job_is_sync(self):
# For copy jobs, displayarchs is "source."
upload, job = self.makeUploadWithPackageCopyJob()
- self.assertEqual('source', upload.displayarchs)
+ self.assertEqual('sync', upload.displayarchs)
def test_component_and_section_name(self):
# An upload with a copy job takes its component and section
@@ -788,75 +787,3 @@
[upload],
upload_set.getAll(
distroseries, name=spn.name, version=upload.displayversion))
-
-
-class TestPackageUploadWithPackageCopyJob(TestCaseWithFactory):
-
- layer = LaunchpadZopelessLayer
- dbuser = config.uploadqueue.dbuser
-
- def test_package_copy_job_property(self):
- # Test that we can set and get package_copy_job.
- pcj = removeSecurityProxy(
- self.factory.makePlainPackageCopyJob()).context
- pu = self.factory.makePackageUpload(package_copy_job=pcj)
- Store.of(pu).flush()
-
- self.assertEqual(pcj, pu.package_copy_job)
-
- def test_getByPackageCopyJobIDs(self):
- pcj = removeSecurityProxy(
- self.factory.makePlainPackageCopyJob()).context
- pu = self.factory.makePackageUpload(package_copy_job=pcj)
- result = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
- [pcj.id])
- self.assertEqual(pu, result.one())
-
- def test_overrideSource_with_copy_job(self):
- # The overrides should be stored in the job's metadata.
- plain_copy_job = self.factory.makePlainPackageCopyJob()
- pcj = removeSecurityProxy(plain_copy_job).context
- pu = self.factory.makePackageUpload(package_copy_job=pcj)
- component = getUtility(IComponentSet)['restricted']
- section = getUtility(ISectionSet)['games']
-
- expected_metadata = {
- 'component_override': component.name,
- 'section_override': section.name
- }
- expected_metadata.update(plain_copy_job.metadata)
-
- pu.overrideSource(component, section, allowed_components=[component])
-
- self.assertEqual(
- expected_metadata, plain_copy_job.metadata)
-
- def test_acceptFromQueue_with_copy_job(self):
- # acceptFromQueue should accept the upload and resume the copy
- # job.
- plain_copy_job = self.factory.makePlainPackageCopyJob()
- pcj = removeSecurityProxy(plain_copy_job).context
- pu = self.factory.makePackageUpload(package_copy_job=pcj)
- self.assertEqual(PackageUploadStatus.NEW, pu.status)
- plain_copy_job.suspend()
-
- pu.acceptFromQueue()
-
- self.assertEqual(PackageUploadStatus.ACCEPTED, pu.status)
- self.assertEqual(JobStatus.WAITING, plain_copy_job.status)
-
- def test_rejectFromQueue_with_copy_job(self):
- # rejectFromQueue will reject the upload and fail the copy job.
- plain_copy_job = self.factory.makePlainPackageCopyJob()
- pcj = removeSecurityProxy(plain_copy_job).context
- pu = self.factory.makePackageUpload(package_copy_job=pcj)
- plain_copy_job.suspend()
-
- pu.rejectFromQueue()
-
- self.assertEqual(PackageUploadStatus.REJECTED, pu.status)
- self.assertEqual(JobStatus.FAILED, plain_copy_job.status)
-
- # It cannot be resurrected after rejection.
- self.assertRaises(
- QueueInconsistentStateError, pu.acceptFromQueue, None)
