launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/launchpad/format_version_3_debbugs_1029294 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1029294 in Launchpad itself: "SummaryVersionError in bugwatch script updating debbugs bug"
  https://bugs.launchpad.net/launchpad/+bug/1029294

For more details, see:
https://code.launchpad.net/~gz/launchpad/format_version_3_debbugs_1029294/+merge/118989

Adds some testing for bug watches against the debian bug tracker, and accept summaries using Format-Version: 3 without throwing.

The only difference between format version 2 and 3 is that free text headers use raw UTF-8 rather than RFC 1522 encoding. The affected headers are:

    my @rfc1522_fields = qw(originator subject done forwarded owner);

Some of these are used by the import code, but there is no decoding done anyway at the moment. In addition, the methods that handle fields where the header contents are used are currently dead code, as bug imports are disabled.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/externalbugtracker/tests/test_debbugs.py
  lib/lp/bugs/scripts/debbugs.py

./lib/lp/bugs/scripts/debbugs.py
      35: local variable 'description' is assigned to but never used
      62: W602 deprecated form of raising exception
      65: E501 line too long (86 characters)
      65: W602 deprecated form of raising exception
      84: E302 expected 2 blank lines, found 1
      84: E701 multiple statements on one line (colon)
      85: E302 expected 2 blank lines, found 0
      85: E701 multiple statements on one line (colon)
      86: E302 expected 2 blank lines, found 0
      86: E701 multiple statements on one line (colon)
      87: E302 expected 2 blank lines, found 0
      87: E701 multiple statements on one line (colon)
      88: E302 expected 2 blank lines, found 0
      88: E701 multiple statements on one line (colon)
      89: E302 expected 2 blank lines, found 0
      89: E701 multiple statements on one line (colon)
      90: E302 expected 2 blank lines, found 0
      90: E701 multiple statements on one line (colon)
      91: E302 expected 2 blank lines, found 0
      91: E701 multiple statements on one line (colon)
      92: E302 expected 2 blank lines, found 0
      92: E701 multiple statements on one line (colon)
      93: E302 expected 2 blank lines, found 0
      93: E701 multiple statements on one line (colon)
      94: E302 expected 2 blank lines, found 0
      94: E701 multiple statements on one line (colon)
      96: E302 expected 2 blank lines, found 1
     103: E501 line too long (160 characters)
     156: W602 deprecated form of raising exception
     162: W602 deprecated form of raising exception
     166: W602 deprecated form of raising exception
     169: E501 line too long (95 characters)
     169: W602 deprecated form of raising exception
     181: E231 missing whitespace after ','
     191: E501 line too long (87 characters)
     197: W602 deprecated form of raising exception
     246: W602 deprecated form of raising exception
     274: W391 blank line at end of file

Did not change any of these lines, and don't particularly want to be in the blame by fixing them. Much of the code could be deleted anyway unless we want to re-enable imports later?

-- 
https://code.launchpad.net/~gz/launchpad/format_version_3_debbugs_1029294/+merge/118989
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/launchpad/format_version_3_debbugs_1029294 into lp:launchpad.

=== added file 'lib/lp/bugs/externalbugtracker/tests/test_debbugs.py'
--- lib/lp/bugs/externalbugtracker/tests/test_debbugs.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_debbugs.py	2012-08-09 16:07:58 +0000
@@ -0,0 +1,161 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the Debian bugtracker"""
+
+import email.message
+import errno
+import os
+
+from testtools.matchers import (
+    Equals,
+    IsInstance,
+    raises,
+    )
+
+from lp.bugs.externalbugtracker.debbugs import (
+    DebBugs,
+    DebBugsDatabaseNotFound,
+    )
+from lp.bugs.interfaces.bugtask import (
+    BugTaskImportance,
+    BugTaskStatus,
+    )
+from lp.bugs.scripts.debbugs import (
+    SummaryParseError,
+    SummaryVersionError,
+    )
+
+from lp.testing import (
+    TestCase,
+    )
+
+
+class TestDebBugs(TestCase):
+
+    def setUp(self):
+        super(TestDebBugs, self).setUp()
+        self.tempdir = self.makeTemporaryDirectory()
+
+    def get_tracker(self):
+        return DebBugs("http://testing.invalid";, db_location=self.tempdir)
+
+    def make_bug_summary(self, bug_id, format_version=2, headers=None):
+        """Create a bug summary file on disk"""
+        bucket_name = "%02d" % (int(bug_id) % 100,)
+        bucket = os.path.join(self.tempdir, "db-h", bucket_name)
+        try:
+            os.makedirs(bucket)
+        except OSError as e:
+            if e.errno != errno.EEXIST:
+                raise
+        m = email.message.Message()
+        # For whatever reason Date is a unix timestamp not an email datestamp
+        m["Date"] = "1000000000"
+        if format_version > 1:
+            m["Format-Version"] = str(format_version)
+        m["Message-Id"] = "<%s@testing.invalid>" % (bug_id,)
+        if headers is not None:
+            for h in headers:
+                m[h] = headers[h]
+        with open(os.path.join(bucket, "%s.summary" % (bug_id,)), "wb") as f:
+            f.write(m.as_string())
+
+    def test_no_db_dir(self):
+        err = self.assertRaises(DebBugsDatabaseNotFound, self.get_tracker)
+        self.assertThat(err.url, Equals(self.tempdir))
+
+    def check_status(self, bug_id, expect_status):
+        tracker = self.get_tracker()
+        remote_status = tracker.getRemoteStatus(bug_id)
+        lp_status = tracker.convertRemoteStatus(remote_status)
+        self.assertThat(lp_status, Equals(expect_status))
+
+    def test_status_missing(self):
+        self.make_bug_summary("1")
+        self.check_status("1", BugTaskStatus.NEW)
+
+    def test_status_upstream(self):
+        self.make_bug_summary("1", headers={"Tags": "upstream"})
+        self.check_status("1", BugTaskStatus.CONFIRMED)
+
+    def test_status_forwarded_to(self):
+        self.make_bug_summary("1", headers={
+            "Forwarded-To": "https://bugs.launchpad.net/ubuntu/+bug/1";,
+            })
+        self.check_status("1", BugTaskStatus.CONFIRMED)
+
+    def test_status_moreinfo(self):
+        self.make_bug_summary("1", headers={"Tags": "moreinfo"})
+        self.check_status("1", BugTaskStatus.INCOMPLETE)
+
+    def test_status_wontfix(self):
+        self.make_bug_summary("1", headers={"Tags": "upstream wontfix"})
+        self.check_status("1", BugTaskStatus.WONTFIX)
+
+    def test_status_done(self):
+        self.make_bug_summary("1", headers={
+            "Done": "A Maintainer <a.maintainer@xxxxxxxxxxx>",
+            })
+        self.check_status("1", BugTaskStatus.FIXRELEASED)
+
+    def test_severity_missing(self):
+        """Without severity set importance is set to unknown"""
+        self.make_bug_summary("1")
+        tracker = self.get_tracker()
+        severity = tracker.getRemoteImportance("1")
+        importance = tracker.convertRemoteImportance(severity)
+        self.assertThat(importance, Equals(BugTaskImportance.UNKNOWN))
+
+    def test_severity_ignored(self):
+        """Severity exists in debbugs but is ignored by launchpad"""
+        self.make_bug_summary("1", headers={"Severity": "normal"})
+        tracker = self.get_tracker()
+        severity = tracker.getRemoteImportance("1")
+        importance = tracker.convertRemoteImportance(severity)
+        self.assertThat(importance, Equals(BugTaskImportance.UNKNOWN))
+
+    def test_format_version_1(self):
+        """Initial format without version marker is rejected"""
+        self.make_bug_summary("1", format_version=1)
+        tracker = self.get_tracker()
+        self.assertThat(lambda: tracker.getRemoteStatus("1"),
+            raises(SummaryParseError))
+        tracker.getRemoteImportance("1")
+
+    def test_format_version_3(self):
+        """Updated format with different escaping is not rejected"""
+        self.make_bug_summary("1", format_version=3)
+        tracker = self.get_tracker()
+        tracker.getRemoteStatus("1")
+        tracker.getRemoteImportance("1")
+
+    def test_format_version_4(self):
+        """A hypothetical summary format version 4 is rejected"""
+        self.make_bug_summary("1", format_version=4)
+        tracker = self.get_tracker()
+        self.assertThat(lambda: tracker.getRemoteStatus("1"),
+            raises(SummaryVersionError))
+        tracker.getRemoteImportance("1")
+
+    def test_non_ascii_v2(self):
+        """Format-Version 2 RFC 1522 encoding on headers should not break"""
+        self.make_bug_summary("1", headers={
+            "Submitter": "=?UTF-8?Q?Jes=C3=BAs?= <jesus@xxxxxxxxxxx>",
+            "Subject": "Add =?UTF-8?Q?Jes=C3=BAs?= as a Debian Maintainer",
+            "Package": "debian-maintainers",
+            })
+        tracker = self.get_tracker()
+        self.assertThat(tracker.getRemoteStatus("1"), IsInstance(str))
+        self.assertThat(tracker.getRemoteImportance("1"), IsInstance(str))
+
+    def test_non_ascii_v3(self):
+        """Format-Version 2 UTF-8 encoding on headers should not break"""
+        self.make_bug_summary("1", format_version=3, headers={
+            "Submitter": "Jes\xc3\xbas <jesus@xxxxxxxxxxx>",
+            "Subject": "Add Jes\xc3\xbas as a Debian Maintainer",
+            "Package": "debian-maintainers",
+            })
+        tracker = self.get_tracker()
+        self.assertThat(tracker.getRemoteStatus("1"), IsInstance(str))
+        self.assertThat(tracker.getRemoteImportance("1"), IsInstance(str))

=== modified file 'lib/lp/bugs/scripts/debbugs.py'
--- lib/lp/bugs/scripts/debbugs.py	2012-06-29 08:40:05 +0000
+++ lib/lp/bugs/scripts/debbugs.py	2012-08-09 16:07:58 +0000
@@ -165,7 +165,7 @@
         if version is None:
             raise SummaryParseError, "%s: Missing Format-Version" % summary
 
-        if version != '2':
+        if version not in ('2', '3'):
             raise SummaryVersionError, "%s: I don't understand version %s" % (summary, version)
 
         bug.originator = message['submitter']