Merge lp:~mabac/svammel/log-filed-bugs into lp:svammel

[Mattias Backman]

Hi,

This branch adds the "nice to have" feature of avoiding to re-file bugs for build failures. There is already a check that there is no open bug on the failed software package with the default tags. It might however happen that a bug has been re-targeted or closed since we filed it and by logging the bug reports to file we avoid re-filing them.

For this to work it requires that the user supplies an existing log file with --logfile. If that parameter is not supplied at all, nothing will be logged.

The implementation assumes that having filed a bug against a particular version of a package makes reporting bugs for newer build failures on the same version uninteresting. That is regardless of which rebuild archive the first failure was found in, if the same log file is supplied. Failures on higher versions will always be reported though.

Thanks,

Mattias

[James Westby (james-w)]

Hi,

This looks great.

I'm not sure we should open the log file repeatedly, but it's just
a hunch, so I have no problem with this for now.

Thanks,

James

review: Approve

[lp:~mabac/svammel/log-filed-bugs]

76. By Mattias Backman on 2011-03-23

Improve tests.

[Mattias Backman (mabac)]

> Hi,
>
> This looks great.
>
> I'm not sure we should open the log file repeatedly, but it's just
> a hunch, so I have no problem with this for now.

I did think about that and weighed it against opening the file for writing once and keeping it open while iterating over the packages. It felt at bit greedy since it's not certain that there will be anything to write.

I let the script fail if there's a problem writing to the log so perhaps it would be better to grab access to the log file before even trying to file bugs.

Thanks,

Mattias

[lp:~mabac/svammel/log-filed-bugs]

77. By Mattias Backman on 2011-03-23

Wrap some long lines.

78. By Mattias Backman on 2011-03-23

Moved opening of log file to Log constructor.

[Mattias Backman (mabac)]

> > Hi,
> >
> > This looks great.
> >
> > I'm not sure we should open the log file repeatedly, but it's just
> > a hunch, so I have no problem with this for now.
>
> I did think about that and weighed it against opening the file for writing
> once and keeping it open while iterating over the packages. It felt at bit
> greedy since it's not certain that there will be anything to write.
>
> I let the script fail if there's a problem writing to the log so perhaps it
> would be better to grab access to the log file before even trying to file
> bugs.

I thought a bit about this and changed it to just open the file once since it was a small change. In case we want to lock the file during bug filing (which we don't do now) this helps. Other than that this just reduced the number of times to open/close the file.

Thanks,

Mattias

=== modified file 'bug_reporting.py'
--- bug_reporting.py	2011-03-14 20:33:45 +0000
+++ bug_reporting.py	2011-03-23 11:14:27 +0000
@@ -39,7 +39,7 @@
                             target=target_url,
                             tags=tags)
     if bug is not None:
-        print 'Filed bug %i: %s' % (bug.id, bug.self_link)
+        print 'Filed bug %i: %s' % (bug.id, bug.web_link)
     else:
         print "Error: unable to file bug '%s'." % title
 

=== modified file 'config.py'
--- config.py	2011-03-17 08:31:23 +0000
+++ config.py	2011-03-23 11:14:27 +0000
@@ -117,6 +117,9 @@
         help="The series from where to get builds, for instance, 'natty'. " \
             "Note that the series is only used when looking in the " \
             "primary archive.")
+    parser.add_argument(
+        '--logfile', required=False,
+        help="Path to file where reported bugs are logged.")
 
     return parser
 

=== modified file 'file-failures.py'
--- file-failures.py	2011-03-11 10:19:44 +0000
+++ file-failures.py	2011-03-23 11:14:27 +0000
@@ -30,6 +30,10 @@
     get_file_contents,
     get_interesting_part,
 )
+from logging import (
+    Log,
+    LogEntry,
+)
 
 
 class LaunchpadError(Exception):
@@ -59,6 +63,7 @@
     print 'Error: Could not get build status'
     raise
 
+bug_log = Log(args.logfile)
 for spph in interesting_spph:
 
     if bug_already_filed_by_url(spph.url, get_launchpad()):
@@ -66,6 +71,13 @@
             (spph.package_name, spph.version))
         continue
 
+    if bug_log.get_entry(spph.package_name, spph.version,
+                         fail_platform) is not None:
+        print_message("Bug already reported for %s version %s. Perhaps it " \
+                          "has been closed or retargeted." % \
+                          (spph.package_name, spph.version))
+        continue
+
     log = spph.get_arch(fail_platform)
     if log is not None:
         build_log = get_file_contents(log.build_log_url)
@@ -89,5 +101,9 @@
     print_message(
         "Will file bug for %s\n desc: %s\n title:  %s\n tags: %s" %
         (spph.package_name, bug_description[0:220], bug_title, bug_tags))
-    file_bug_by_url(spph.url, bug_description, bug_title,
-                    bug_tags, get_launchpad())
+    bug = file_bug_by_url(spph.url, bug_description, bug_title,
+                          bug_tags, get_launchpad())
+    if bug is not None and args.logfile is not None:
+        bug_log.write_entry(
+            LogEntry(spph.package_name, spph.version, fail_platform,
+                     bug.self_link).to_string())

=== added file 'logging.py'
--- logging.py	1970-01-01 00:00:00 +0000
+++ logging.py	2011-03-23 11:14:27 +0000
@@ -0,0 +1,90 @@
+###############################################################################
+# Copyright (c) 2011, 2011 Linaro
+# All rights reserved. This program and the accompanying materials
+# are made available under the terms of the Eclipse Public License v1.0
+# which accompanies this distribution, and is available at
+# http://www.eclipse.org/legal/epl-v10.html
+#
+# Contributors:
+#     Linaro Infrastructure Team - initial implementation
+###############################################################################
+from config import (
+    print_message,
+    )
+
+
+LOG_ITEM_SEPARATOR = ';'
+
+
+class Log(object):
+    def __init__(self, log_file_name=None):
+        self.log_entries = []
+        self.log_file_name = log_file_name
+        self.populate_from_file()
+
+        self.log_file = None
+        if self.log_file_name is not None:
+            try:
+                self.log_file = open(self.log_file_name, 'a')
+            except IOError:
+                print 'Error: Could not open log file for writing.'
+                raise
+
+    def __del__(self):
+        if self.log_file is not None:
+            self.log_file.close()
+
+    def populate_from_file(self):
+        if self.log_file_name is not None:
+            log_file = None
+            try:
+                log_file = open(self.log_file_name, 'r')
+                for line in log_file:
+                    log_items = line.rstrip('\n').split(LOG_ITEM_SEPARATOR)
+                    if len(log_items) != 4:
+                        # Skip invalid log line
+                        continue
+                    log_entry = LogEntry(log_items[0], log_items[1],
+                                         log_items[2], log_items[3])
+                    self.add_entry(log_entry)
+            except IOError:
+                print_message("Log file '%s' does not exist." % \
+                                  self.log_file_name)
+            finally:
+                if log_file is not None:
+                    log_file.close()
+
+    def add_entry(self, log_entry):
+        self.log_entries.append(log_entry)
+
+    def get_entry(self, package_name, package_version, build_arch):
+        for entry in self.log_entries:
+            if entry.matches(package_name, package_version, build_arch):
+                return entry
+        return None
+
+    def write_entry(self, log_string):
+        if self.log_file is not None:
+            try:
+                self.log_file.write(log_string + '\n')
+            except IOError:
+                print 'Error: Could not write log to file.'
+                self.log_file.close()
+                raise
+
+class LogEntry(object):
+    def __init__(self, package_name, package_version, build_arch, bugtask_url):
+        self.package_name = package_name
+        self.package_version = package_version
+        self.build_arch = build_arch
+        self.bugtask_url = bugtask_url
+
+    def to_string(self):
+        return LOG_ITEM_SEPARATOR.join(
+            [self.package_name, self.package_version, self.build_arch,
+             self.bugtask_url])
+
+    def matches(self, package_name, package_version, build_arch):
+        return (self.package_name == package_name and
+                self.package_version == package_version and
+                self.build_arch == build_arch)

=== modified file 'tests/__init__.py'
--- tests/__init__.py	2011-03-01 11:50:47 +0000
+++ tests/__init__.py	2011-03-23 11:14:27 +0000
@@ -14,6 +14,7 @@
 def test_suite():
     module_names = [
         'tests.test_fail_filer',
+        'tests.test_bug_logging',
         ]
 
     loader = unittest.TestLoader()

=== modified file 'tests/fixtures.py'
--- tests/fixtures.py	2011-03-01 11:50:47 +0000
+++ tests/fixtures.py	2011-03-23 11:14:27 +0000
@@ -10,6 +10,8 @@
 ###############################################################################
 from testtools import TestCase
 from launchpadlib.launchpad import Launchpad
+import tempfile
+import os
 
 
 class TestCaseWithFixtures(TestCase):

=== added file 'tests/test_bug_logging.py'
--- tests/test_bug_logging.py	1970-01-01 00:00:00 +0000
+++ tests/test_bug_logging.py	2011-03-23 11:14:27 +0000
@@ -0,0 +1,150 @@
+###############################################################################
+# Copyright (c) 2011, 2011 Linaro
+# All rights reserved. This program and the accompanying materials
+# are made available under the terms of the Eclipse Public License v1.0
+# which accompanies this distribution, and is available at
+# http://www.eclipse.org/legal/epl-v10.html
+#
+# Contributors:
+#     Linaro Infrastructure Team - initial implementation
+###############################################################################
+import __builtin__
+from testtools import TestCase
+
+from tests.fixtures import (
+    TestCaseWithFixtures,
+    MockSomethingFixture,
+    )
+from logging import (
+    LOG_ITEM_SEPARATOR,
+    Log,
+    LogEntry,
+    )
+
+
+class TestLogEntry(TestCase):
+
+    def setUp(self):
+        super(TestLogEntry, self).setUp()
+
+    def test_log_line_format(self):
+        package_name = 'package'
+        package_version = '1.0.1'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                                    bugtask_url)
+        self.assertEquals(log_entry.to_string(),
+                          package_name + LOG_ITEM_SEPARATOR + package_version +
+                          LOG_ITEM_SEPARATOR + build_arch +
+                          LOG_ITEM_SEPARATOR + bugtask_url)
+
+    def test_log_line_matches(self):
+        package_name = 'package'
+        package_version = '1.0.1'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        self.assertTrue(log_entry.matches(package_name, package_version,
+                                          build_arch))
+
+    def test_log_line_does_not_match_other_version(self):
+        package_name = 'package'
+        package_version_logged = '1.0.1'
+        package_version_to_match = '2'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version_logged, build_arch,
+                             bugtask_url)
+        self.assertFalse(
+            log_entry.matches(package_name, package_version_to_match,
+                              build_arch))
+
+
+class TestLog(TestCaseWithFixtures):
+
+    def setUp(self):
+        super(TestLog, self).setUp()
+
+    def test_add_entry_does_so(self):
+        package_name = 'package'
+        package_version = '1.0.1'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        log = Log()
+        log.add_entry(log_entry)
+        self.assertIn(log_entry, log.log_entries)
+
+    def test_get_nonexistent_logs_fails(self):
+        log = Log()
+        self.assertEquals(None, log.get_entry('package', 'version', 'arch'))
+
+    def test_stored_log_is_found(self):
+        package_name = 'package'
+        package_version = '1.0.1'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        log = Log()
+        log.add_entry(log_entry)
+        self.assertEquals(
+            log_entry, log.get_entry(package_name, package_version,
+                                     build_arch))
+
+    def test_no_entries_in_log_without_file(self):
+        log = Log(None)
+        self.assertEquals([], log.log_entries)
+
+    def test_no_entries_in_log_with_empty_file(self):
+        log_file = self.createTempFileAsFixture()
+        log = Log(log_file)
+        self.assertEquals([], log.log_entries)
+
+    def test_read_log_entry_from_file(self):
+        package_name = 'packagename'
+        package_version = '1.1.2'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        log_file_name = self.createTempFileAsFixture()
+        with open(log_file_name, 'w') as log_file_write:
+            log_file_write.write(log_entry.to_string())
+
+        log = Log(log_file_name)
+        log_entry_read = log.get_entry(package_name, package_version,
+                                       build_arch)
+        self.assertTrue(len(log.log_entries) == 1)
+        self.assertTrue(log_entry_read.matches(
+                package_name, package_version, build_arch))
+
+    def test_write_log_entry_to_file(self):
+        package_name = 'packagename'
+        package_version = '1.2.3-1'
+        build_arch = 'armel'
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        log_file_name = self.createTempFileAsFixture()
+        log_write = Log(log_file_name)
+        log_write.write_entry(log_entry.to_string())
+        del log_write
+        with open(log_file_name, 'r') as log_file:
+            log_line = log_file.readline().rstrip('\n')
+        self.assertEquals(log_entry.to_string(), log_line)
+
+    def test_failed_open_log_for_writing_raises(self):
+        def mock_open(self, string):
+            raise IOError("Test error.")
+
+        def create_log(file_name):
+            log_write = Log(file_name)
+
+        self.useFixture(MockSomethingFixture(
+                __builtin__, 'open', mock_open))
+        log_file_name = 'logfile'
+        self.assertRaises(IOError, create_log, log_file_name)