Merge lp:~mabac/svammel/auto-close-retried into lp:svammel

[Mattias Backman]

Hi,

This branch adds a script for closing bugs. It's supposed to cover the requested feature of being able to close a bug which was filed due to transient archive inconsistency.

From an email from Steve L:
"[]...sometimes a build fails due
to transient archive breakage, particularly in the main archive, and a
developer will request a retry - that build gets the same build id as the
failure, but spits out a successful build. [...]"

The script acually lets you close any logged bug when a successful build is found of the same or higher version for the package. This should cover the above case and also supports the following cases where you might want to close bugs.

* The user forgets to add an archive to cross check for successful armel builds. The user can then do 'close-bugs --archive <archive with successful builds> --logfile filed_bugs.log' to close the wrongly filed bugs.
 * The user completely messes up and wants to close all the bugs. Just add '--forceclose' to the command line to close all bugs found in the log file.

I feel that this is a somewhat different use case than running the file-failures script to file bugs, which is why I put this functionality in a separate script. I can make it part of the main flow if that's better.

By "closing" I mean setting the status of all connected bug_tasks to "Invalid". There may well be a better implementation of closing bugs, please let me know.

Thanks,

Mattias

[James Westby (james-w)]

24 + for bug_task in bug.bug_tasks:
25 + print_message(" Changing status for %s [%s ==> %s]" % \
26 + (bug_task.title, bug_task.status, BUG_CLOSED_STATUS))

Perhaps we should have the --dry-run support here too?

Ah, I see now it is done at a higher level, but that message
doesn't say what bug would be closed. Could we have it print
the ".web_link" of each task that would be closed?

I have no problems with the code, it looks good to me.

I do wonder about whether this should be a separate script (--force-close
can be), but I wonder if just having the script in cron clean up
automatically would be better. Or would you imagine having both scripts
in cron?

Also, closing bugs doesn't alter the log file, will that lead to
missing bugs if it goes fail->success->fail?

Thanks,

James

[Mattias Backman (mabac)]

> 24 + for bug_task in bug.bug_tasks:
> 25 + print_message(" Changing status for %s [%s ==> %s]" % \
> 26 + (bug_task.title, bug_task.status,
> BUG_CLOSED_STATUS))
>
> Perhaps we should have the --dry-run support here too?
>
> Ah, I see now it is done at a higher level, but that message
> doesn't say what bug would be closed. Could we have it print
> the ".web_link" of each task that would be closed?

Sure, I'll add that.

>
> I have no problems with the code, it looks good to me.
>
> I do wonder about whether this should be a separate script (--force-close
> can be), but I wonder if just having the script in cron clean up
> automatically would be better. Or would you imagine having both scripts
> in cron?

Well, this is where I'd like some input from Steve I guess. I don't really know if this is going to be running in cron, or if a developer would run the script with carefully selected archives and never analyze the same combination of archives more than once.

>
> Also, closing bugs doesn't alter the log file, will that lead to
> missing bugs if it goes fail->success->fail?

Yes, it will. I imagined that seeing a successful build once for a particular version in a rebuild archive would rule out that it is a porting issue. When you run the bug filer on a new rebuild archive I figured that you'd just point it to a blank log file and start from scratch. Or that any new failure would be in a higher version which would be filed.

Again, I'd like some input as to how svammel users would like to use it. There are more behaviors that I expect will have to be tweaked since I've made my own assumptions.

Thanks,

Mattias

[James Westby (james-w)]

On Thu, 24 Mar 2011 17:10:04 -0000, Mattias Backman <email address hidden> wrote:
> Well, this is where I'd like some input from Steve I guess. I don't
> really know if this is going to be running in cron, or if a developer
> would run the script with carefully selected archives and never
> analyze the same combination of archives more than once.

I imagine it would be done multiple times for a collection of
archives. This is because it can take a week or longer for a full
rebuild test, and we would want to start filing bugs for the early
failures before then.

> Yes, it will. I imagined that seeing a successful build once for a
> particular version in a rebuild archive would rule out that it is a
> porting issue. When you run the bug filer on a new rebuild archive I
> figured that you'd just point it to a blank log file and start from
> scratch. Or that any new failure would be in a higher version which
> would be filed.

Yeah, those are good points, so I agree that it's not a big issue.

> Again, I'd like some input as to how svammel users would like to use
> it. There are more behaviors that I expect will have to be tweaked
> since I've made my own assumptions.

There is a rebuild test due soon. Let's ensure that we have something
that Steve can use for filing bugs from that, and we will get good
specific feedback then if there are still issues.

Thanks,

James

[Mattias Backman (mabac)]

> I do wonder about whether this should be a separate script (--force-close
> can be), but I wonder if just having the script in cron clean up
> automatically would be better. Or would you imagine having both scripts
> in cron?

I've made this a part of the file-failures script now so it runs a clean-up pass after filing any new bugs. I'll leave the close-bugs script in there for now, will remove it if there seems to be no use for it.

Thanks,

Mattias

[lp:~mabac/svammel/auto-close-retried]

91. By Mattias Backman on 2011-03-25

Add bugtask weblink to dryrun message.

92. By Mattias Backman on 2011-03-25

Add clean up pass to file-failures.

93. By Mattias Backman on 2011-03-25

Fix import.

94. By Mattias Backman on 2011-03-25

Moved clean up out of loop.

[James Westby (james-w)]

This looks good now, I think this will provide the best user experience
for Steve.

Thanks,

James

review: Approve

[Steve Langasek (vorlon)]

On Thu, Mar 24, 2011 at 05:10:05PM -0000, Mattias Backman wrote:
> > I have no problems with the code, it looks good to me.

> > I do wonder about whether this should be a separate script (--force-close
> > can be), but I wonder if just having the script in cron clean up
> > automatically would be better. Or would you imagine having both scripts
> > in cron?

> Well, this is where I'd like some input from Steve I guess. I don't really
> know if this is going to be running in cron, or if a developer would run
> the script with carefully selected archives and never analyze the same
> combination of archives more than once.

I have no immediate plans to run the script from a cron job, but we will
definitely have occasion to run the script repeatedly against the same set
of archives - both to report new build failures over the course of a
rebuild, as James points out, and also because we would run this repeatedly
against the main archive. (The latter use might eventually be cronned.)

> > Also, closing bugs doesn't alter the log file, will that lead to
> > missing bugs if it goes fail->success->fail?

> Yes, it will. I imagined that seeing a successful build once for a
> particular version in a rebuild archive would rule out that it is a
> porting issue. When you run the bug filer on a new rebuild archive I
> figured that you'd just point it to a blank log file and start from
> scratch. Or that any new failure would be in a higher version which would
> be filed.

This makes sense to me. As long as a build failure of a *new* version is
tracked, or it's clear how to invoke the tool to get a blank log file when
pointing at a new rebuild archive, that's fine.

On Fri, Mar 25, 2011 at 07:50:55AM -0000, Mattias Backman wrote:
> > I do wonder about whether this should be a separate script (--force-close
> > can be), but I wonder if just having the script in cron clean up
> > automatically would be better. Or would you imagine having both scripts
> > in cron?

> I've made this a part of the file-failures script now so it runs a
> clean-up pass after filing any new bugs. I'll leave the close-bugs script
> in there for now, will remove it if there seems to be no use for it.

Thanks, yes, I prefer to have this done as part of a single command rather
than having to invoke close-bugs separately.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[Steve Langasek (vorlon)]

> By "closing" I mean setting the status of all connected
> bug_tasks to "Invalid". There may well be a better
> implementation of closing bugs, please let me know.

Optimally, a bug that's being closed because the package has subsequently built successfully in the same package version would be closed as "invalid" and one closed in a later package version would be "Fix released"; but that's nitpicking on my part.

More importantly, we should only be closing bug tasks corresponding to the package the bug was originally reported on. If a bug report gets addressed in the current package by applying a workaround, but a new task has been opened on, for example, gcc-linaro to flag that the compiler needs a fix for the underlying issue, we shouldn't touch that task.

review: Needs Fixing

[Mattias Backman (mabac)]

> > By "closing" I mean setting the status of all connected
> > bug_tasks to "Invalid". There may well be a better
> > implementation of closing bugs, please let me know.
>
> Optimally, a bug that's being closed because the package has subsequently
> built successfully in the same package version would be closed as "invalid"
> and one closed in a later package version would be "Fix released"; but that's
> nitpicking on my part.

I'll have a look at that and probably can change it easily.

>
> More importantly, we should only be closing bug tasks corresponding to the
> package the bug was originally reported on. If a bug report gets addressed in
> the current package by applying a workaround, but a new task has been opened
> on, for example, gcc-linaro to flag that the compiler needs a fix for the
> underlying issue, we shouldn't touch that task.

Thanks for clarifying, I'll fix that.

Closing bugs is only done when a later successful build on armel is found. I do not check for later failures on the other archs. This means that if we run the script on partially built archives where the armel build has failed and the builds are not yet started on the other archs there will be a bug that's not going to be closed even if the other builds fail. Will this be a problem?

Thanks,

Mattias

[lp:~mabac/svammel/auto-close-retried]

95. By Mattias Backman on 2011-03-29

Only close the bug_task we created, not tasks added for other packages.

96. By Mattias Backman on 2011-03-29

Add different statuses for closing bugs.

97. By Mattias Backman on 2011-03-29

pep8 style compliance fixes.

98. By Mattias Backman on 2011-03-29

Merged changes from trunk.

[Steve Langasek (vorlon)]

On Tue, Mar 29, 2011 at 08:58:29AM -0000, Mattias Backman wrote:
> > More importantly, we should only be closing bug tasks corresponding to the
> > package the bug was originally reported on. If a bug report gets addressed in
> > the current package by applying a workaround, but a new task has been opened
> > on, for example, gcc-linaro to flag that the compiler needs a fix for the
> > underlying issue, we shouldn't touch that task.

> Thanks for clarifying, I'll fix that.

> Closing bugs is only done when a later successful build on armel is found.
> I do not check for later failures on the other archs. This means that if
> we run the script on partially built archives where the armel build has
> failed and the builds are not yet started on the other archs there will be
> a bug that's not going to be closed even if the other builds fail. Will
> this be a problem?

No, I think that's fine as the x86 rebuild is obviously going to run much
faster than the armel one in the general case, and we would want to avoid
filing bugs where we know we don't have enough information yet to say if
they're cross-platform. But please document this limitation.

Thanks,
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[lp:~mabac/svammel/auto-close-retried]

99. By Mattias Backman on 2011-03-30

Merge James' fix to avoid shadowing the stlib logging module.

=== modified file 'bug_logging.py'
--- bug_logging.py	2011-03-23 11:12:12 +0000
+++ bug_logging.py	2011-03-30 07:41:31 +0000
@@ -72,6 +72,14 @@
                 self.log_file.close()
                 raise
 
+    def get_arch(self, arch):
+        entries = []
+        for entry in self.log_entries:
+            if entry.is_arch(arch):
+                entries.append(entry)
+        return entries
+
+
 class LogEntry(object):
     def __init__(self, package_name, package_version, build_arch, bugtask_url):
         self.package_name = package_name
@@ -88,3 +96,6 @@
         return (self.package_name == package_name and
                 self.package_version == package_version and
                 self.build_arch == build_arch)
+
+    def is_arch(self, build_arch):
+        return self.build_arch == build_arch

=== modified file 'bug_reporting.py'
--- bug_reporting.py	2011-03-22 10:17:01 +0000
+++ bug_reporting.py	2011-03-30 07:41:31 +0000
@@ -13,6 +13,10 @@
     )
 
 
+BUG_INVALID_STATUS = 'Invalid'
+BUG_FIXED_STATUS = 'Fix Released'
+
+
 def get_project_url(package_name, lp):
     """ Return the link to the project with the supplied name. """
 
@@ -63,3 +67,13 @@
     dupe_bugs = project.searchTasks(tags=dupe_tags, tags_combinator='All')
 
     return len(dupe_bugs[:1]) > 0
+
+
+def close_bug(url, close_status, lp):
+    """ Close the bugtask by setting status to Invalid."""
+
+    bug_task = lp.load(url)
+    print_message("  Changing status for %s [%s ==> %s]" % \
+                      (bug_task.title, bug_task.status, close_status))
+    bug_task.status = close_status
+    bug_task.lp_save()

=== modified file 'data_parsing.py'
--- data_parsing.py	2011-03-30 07:41:31 +0000
+++ data_parsing.py	2011-03-30 07:41:31 +0000
@@ -55,10 +55,10 @@
 
 
 class SPPH(object):
-    def __init__(self, spph):
+    def __init__(self, source_package_name, source_package_version):
         self.records = dict()
-        self.version = spph.source_package_version
-        self.package_name = spph.source_package_name
+        self.version = source_package_version
+        self.package_name = source_package_name
         self.url = get_base_api_url() + '+source/%s' % self.package_name
 
     def add_build_record(self, build_record):
@@ -136,7 +136,8 @@
             print_message("  Found build '%s'" % build.title)
 
             stored_spph = all_spph.get(csp.source_package_name)
-            new_spph = SPPH(csp)
+            new_spph = SPPH(csp.source_package_name,
+                            csp.source_package_version)
             if new_spph.is_higher_version_than(stored_spph):
                 all_spph[csp.source_package_name] = new_spph
                 new_spph.add_build_record(build)
@@ -146,6 +147,30 @@
     return all_spph
 
 
+def exists_successful_later_build(spph, archives, fail_arch):
+    for archive, archive_name in archives:
+        # Get list of successful builds for the same package
+        success_list = get_build_list(archive, SUCCESS_BUILD_STATE,
+                                      spph.package_name)
+        success_spph_list = []
+        for success_build in success_list:
+            csp = success_build.current_source_publication
+            success_spph = SPPH(csp.source_package_name,
+                                csp.source_package_version)
+            if success_build.arch_tag == fail_arch:
+                success_spph_list.append(success_spph)
+
+        candidate_success_spph = get_highest_version(success_spph_list)
+        if spph.is_higher_version_than(candidate_success_spph):
+            # Only care about the failed build if it is of a higher
+            # version than any successful build of the same package.
+            pass
+        else:
+            return candidate_success_spph.version
+
+    return None
+
+
 def filter_spph(all_spph_list, fail_arch, ok_archs, archives):
     """Filter out the build failures that are unique to the target arch.
 
@@ -159,68 +184,48 @@
     A subset of all_spph_list which represents the interesting packages.
     """
     filtered_list = []
+    print_message('Determining which build failures to file bugs for.')
     for spph in all_spph_list.values():
         if not spph.is_ftbfs(fail_arch):
-            print_message("Ignoring package '%s' version '%s' since it did " \
-                              "not fail on architecture '%s'." % \
+            print_message("  Ignoring package '%s' version '%s' since it " \
+                              "did not fail on architecture '%s'." % \
                               (spph.package_name, spph.version, fail_arch))
             continue
         also_failed = next((arch for arch in ok_archs
                                  if spph.is_ftbfs(arch)), None)
         if also_failed is not None:
-            print_message("Ignoring package '%s' version '%s' since it also " \
-                              "failed to build on architecture '%s'." % \
+            print_message("  Ignoring package '%s' version '%s' since it " \
+                              "also failed to build on architecture '%s'." % \
                               (spph.package_name, spph.version, also_failed))
             continue
 
-        for archive, archive_name in archives:
-            # Get list of successful builds for the same package
-            success_list = get_build_list(archive, SUCCESS_BUILD_STATE,
-                                          spph.package_name)
-            success_spph_list = []
-            for success_build in success_list:
-                success_spph = SPPH(success_build.current_source_publication)
-                if success_build.arch_tag == fail_arch:
-                    success_spph_list.append(success_spph)
-
-            candidate_success_spph = get_highest_version(success_spph_list)
-            if spph.is_higher_version_than(candidate_success_spph):
-                # Only care about the failed build if it is of a higher
-                # version than any successful build of the same package.
-                pass
-            else:
-                print_message("Failed build for package '%s' version '%s' " \
-                                  "overridden by successful build of " \
-                                  "version '%s' in archive '%s'." % \
-                                  (spph.package_name, spph.version,
-                                   candidate_success_spph.version,
-                                   archive_name))
-                spph = None
-                break
-
-        if spph is not None:
-            filtered_list.append(spph)
+        success_version = exists_successful_later_build(spph, archives,
+                                                        fail_arch)
+        if success_version is not None:
+            print_message("  Ignoring package '%s' version '%s' since build " \
+                              "of version '%s' succeded." % (spph.package_name,
+                                                             spph.version,
+                                                             success_version))
+            continue
+
+        filtered_list.append(spph)
 
     return filtered_list
 
 
-def get_build_status(archive_names, series_name, fail_platform, ok_platforms):
-    """Download live data from lp.
+def get_archives(archive_names, series_name):
+    """Get list of archive and series.
 
     Parameters:
     archive_names - List of archives. Eg ['primary', 'rebuild-x']
     series_name - Name of series. Eg 'natty'
-    fail_platform - Name of target platform.
-    ok_platforms - List of names of platforms which shouldn't have failed.
 
     Returns:
-    A list of SPPH objects representing source packages to file bugs against.
+    A list of (archive, archive_display_name) pairs.
     """
-
-    ubuntu = get_launchpad().distributions['ubuntu']
-
     archives = []
     series = None
+    ubuntu = get_launchpad().distributions['ubuntu']
     for archive_name in archive_names:
         try:
             archive = None
@@ -242,6 +247,23 @@
         except HTTPError:
             print 'Error: %s is not a valid archive.' % archive_name
             raise
+    return archives
+
+
+def get_build_status(archive_names, series_name, fail_platform, ok_platforms):
+    """Download live data from lp.
+
+    Parameters:
+    archive_names - List of archives. Eg ['primary', 'rebuild-x']
+    series_name - Name of series. Eg 'natty'
+    fail_platform - Name of target platform.
+    ok_platforms - List of names of platforms which shouldn't have failed.
+
+    Returns:
+    A list of SPPH objects representing source packages to file bugs against.
+    """
+
+    archives = get_archives(archive_names, series_name)
 
     print_message("Will be getting builds from archives " + ", ".join(
             name for (archive, name) in archives))

=== modified file 'file-failures.py'
--- file-failures.py	2011-03-30 07:41:31 +0000
+++ file-failures.py	2011-03-30 07:41:31 +0000
@@ -13,10 +13,16 @@
 from data_parsing import (
     get_tags_for_fail,
     get_build_status,
+    get_archives,
+    exists_successful_later_build,
+    SPPH,
     )
 from bug_reporting import (
     file_bug_by_url,
     bug_already_filed_by_url,
+    close_bug,
+    BUG_INVALID_STATUS,
+    BUG_FIXED_STATUS,
     )
 from config import (
     init,
@@ -43,21 +49,23 @@
         return repr(self.value)
 
 
+LP_APP_NAME = 'Svammel bug filer'
+
+
 parser = get_args_parser()
 args = parser.parse_args()
 set_verbose_messages(args.verbose or args.dryrun)
 
-init(args.serviceroot, 'testing', '~/.launchpadlib/cache/')
+init(args.serviceroot, LP_APP_NAME, '~/.launchpadlib/cache/')
 
 # We are interested in build failures for this platform:
-fail_platform = 'armel'
+FAIL_PLATFORM = 'armel'
 # unless there's a failed build for any of these:
-ok_platforms = ['i386', 'amd64']
+OK_PLATFORMS = ['i386', 'amd64']
 
 try:
     interesting_spph = get_build_status(args.archive, args.series,
-                                        fail_platform, ok_platforms)
-
+                                        FAIL_PLATFORM, OK_PLATFORMS)
 except:
     print 'Error: Could not get build status'
     raise
@@ -71,18 +79,18 @@
         continue
 
     if bug_log.get_entry(spph.package_name, spph.version,
-                         fail_platform) is not None:
+                         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
 
-    build_record = spph.get_arch(fail_platform)
+    build_record = spph.get_arch(FAIL_PLATFORM)
     if build_record is not None:
         build_log = get_file_contents(build_record.build_log_url)
     else:
         print "%s build record not available for %s version %s." % \
-            (fail_platform, spph.package_name, spph.version)
+            (FAIL_PLATFORM, spph.package_name, spph.version)
         build_log = ''
     bug_tags = get_tags_for_fail(build_log)
     interesting_log_part = get_interesting_part(build_log)
@@ -105,7 +113,29 @@
                               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())
+                LogEntry(spph.package_name, spph.version, FAIL_PLATFORM,
+                         bug.bug_tasks[0].self_link).to_string())
     else:
         print_message("  ... but didn't since dryrun is specified.")
+
+archives = get_archives(args.archive, args.series)
+for log_entry in bug_log.get_arch(FAIL_PLATFORM):
+    print_message("Checking for successful builds for package " \
+                      "%s version >= %s" % \
+                      (log_entry.package_name, log_entry.package_version))
+    success_version = exists_successful_later_build(
+        SPPH(log_entry.package_name, log_entry.package_version),
+        archives, FAIL_PLATFORM)
+    if success_version is not None:
+        if not args.dryrun:
+            if success_version == log_entry.package_version:
+                new_status = BUG_INVALID_STATUS
+            else:
+                new_status = BUG_FIXED_STATUS
+            close_bug(log_entry.bugtask_url, new_status, get_launchpad())
+        else:
+            bug = get_launchpad().load(log_entry.bugtask_url)
+            for bug_task in bug.bug_tasks:
+                print_message("  Had reason to close the bugtask %s but " \
+                                  "didn't since dryrun is specified." % \
+                                  bug_task.web_link)

=== modified file 'tests/test_bug_logging.py'
--- tests/test_bug_logging.py	2011-03-30 07:41:31 +0000
+++ tests/test_bug_logging.py	2011-03-30 07:41:31 +0000
@@ -22,6 +22,9 @@
     )
 
 
+TEST_FAIL_ARCH = 'armel'
+
+
 class TestLogEntry(TestCase):
 
     def setUp(self):
@@ -30,7 +33,7 @@
     def test_log_line_format(self):
         package_name = 'package'
         package_version = '1.0.1'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version, build_arch,
                                     bugtask_url)
@@ -42,7 +45,7 @@
     def test_log_line_matches(self):
         package_name = 'package'
         package_version = '1.0.1'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version, build_arch,
                              bugtask_url)
@@ -53,7 +56,7 @@
         package_name = 'package'
         package_version_logged = '1.0.1'
         package_version_to_match = '2'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version_logged, build_arch,
                              bugtask_url)
@@ -70,7 +73,7 @@
     def test_add_entry_does_so(self):
         package_name = 'package'
         package_version = '1.0.1'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version, build_arch,
                              bugtask_url)
@@ -85,7 +88,7 @@
     def test_stored_log_is_found(self):
         package_name = 'package'
         package_version = '1.0.1'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version, build_arch,
                              bugtask_url)
@@ -107,7 +110,7 @@
     def test_read_log_entry_from_file(self):
         package_name = 'packagename'
         package_version = '1.1.2'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version, build_arch,
                              bugtask_url)
@@ -125,7 +128,7 @@
     def test_write_log_entry_to_file(self):
         package_name = 'packagename'
         package_version = '1.2.3-1'
-        build_arch = 'armel'
+        build_arch = TEST_FAIL_ARCH
         bugtask_url = 'http://dummy'
         log_entry = LogEntry(package_name, package_version, build_arch,
                              bugtask_url)
@@ -148,3 +151,34 @@
                 __builtin__, 'open', mock_open))
         log_file_name = 'logfile'
         self.assertRaises(IOError, create_log, log_file_name)
+
+    def test_get_arch_returns_empty_list(self):
+        log = Log(None)
+        self.assertEquals([], log.get_arch(TEST_FAIL_ARCH))
+
+    def test_get_arch_returns_arch(self):
+        log = Log(None)
+        package_name = 'packagename'
+        package_version = '1.2.3-1'
+        build_arch = TEST_FAIL_ARCH
+        bugtask_url = 'http://dummy'
+        log_entry = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        log.add_entry(log_entry)
+        self.assertEquals([log_entry], log.get_arch(TEST_FAIL_ARCH))
+
+    def test_get_arch_returns_several_archs(self):
+        log = Log(None)
+        package_name = 'packagename'
+        package_version = '1.2.3-1'
+        build_arch = TEST_FAIL_ARCH
+        bugtask_url = 'http://dummy'
+        log_entry1 = LogEntry(package_name, package_version, build_arch,
+                             bugtask_url)
+        log_entry2 = LogEntry(package_name + 'new', package_version + 'later',
+                              build_arch, bugtask_url + '/not/same')
+        log.add_entry(log_entry1)
+        log.add_entry(log_entry2)
+        logs = log.get_arch(TEST_FAIL_ARCH)
+        self.assertIn(log_entry1, logs)
+        self.assertIn(log_entry2, logs)

=== modified file 'tests/test_fail_filer.py'
--- tests/test_fail_filer.py	2011-03-30 07:41:31 +0000
+++ tests/test_fail_filer.py	2011-03-30 07:41:31 +0000
@@ -74,11 +74,6 @@
 
 class TestFilter(TestCaseWithFixtures):
 
-    class MockPackage(object):
-        def __init__(self, name, version):
-            self.source_package_version = version
-            self.source_package_name = name
-
     class MockRecord(object):
         class CSP:
             def __init__(self, name, version):
@@ -113,7 +108,7 @@
 
     def test_filter_fail_record_not_filtered(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('packagefail', '1'))
+        spph = SPPH('packagefail', '1')
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                      FAILED_BUILD_STATE)
         spph.add_build_record(failrecord)
@@ -125,7 +120,7 @@
 
     def test_filter_success_record_filtered(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('packagesuccess', '1'))
+        spph = SPPH('packagesuccess', '1')
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                      SUCCESS_BUILD_STATE)
         spph.add_build_record(failrecord)
@@ -137,7 +132,7 @@
 
     def test_filter_only_other_arch_filtered(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('packageotherarch', '1'))
+        spph = SPPH('packageotherarch', '1')
         failrecord = self.MockRecord(TEST_OK_ARCHS[0], datetime.now(),
                                      FAILED_BUILD_STATE)
         spph.add_build_record(failrecord)
@@ -149,7 +144,7 @@
 
     def test_filter_no_arch_filtered(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('packagenobuilds', '1'))
+        spph = SPPH('packagenobuilds', '1')
         spph_list = dict()
         spph_list[spph.package_name] = spph
         filtered_list = filter_spph(spph_list, TEST_FAIL_ARCH, TEST_OK_ARCHS,
@@ -174,7 +169,7 @@
         self.init_dummy_config()
         packagename = 'packagefailandok'
         version = '1'
-        fail_spph = SPPH(self.MockPackage(packagename, version))
+        fail_spph = SPPH(packagename, version)
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                      FAILED_BUILD_STATE, packagename, version)
         successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
@@ -192,7 +187,7 @@
         self.init_dummy_config()
         packagename = 'packagename'
         version = '1'
-        fail_spph = SPPH(self.MockPackage(packagename, version))
+        fail_spph = SPPH(packagename, version)
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                      FAILED_BUILD_STATE, packagename, version)
         successrecord = self.MockRecord(TEST_OK_ARCHS[0], datetime.now(),
@@ -211,9 +206,10 @@
         packagename = 'packagename'
         low_version = '1'
         high_version = '1.2.3'
-        fail_spph = SPPH(self.MockPackage(packagename, low_version))
+        fail_spph = SPPH(packagename, low_version)
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
-                                     FAILED_BUILD_STATE, packagename, low_version)
+                                     FAILED_BUILD_STATE, packagename,
+                                     low_version)
         successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                         SUCCESS_BUILD_STATE, packagename,
                                         high_version)
@@ -230,9 +226,10 @@
         packagename = 'packagename'
         low_version = '1'
         high_version = '1.2.3'
-        fail_spph = SPPH(self.MockPackage(packagename, high_version))
+        fail_spph = SPPH(packagename, high_version)
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
-                                     FAILED_BUILD_STATE, packagename, high_version)
+                                     FAILED_BUILD_STATE, packagename,
+                                     high_version)
         successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                         SUCCESS_BUILD_STATE, packagename,
                                         low_version)
@@ -250,9 +247,10 @@
         low_version = '1'
         mid_version = '2.1.3-1'
         high_version = '3.2.3'
-        fail_spph = SPPH(self.MockPackage(packagename, mid_version))
+        fail_spph = SPPH(packagename, mid_version)
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
-                                     FAILED_BUILD_STATE, packagename, mid_version)
+                                     FAILED_BUILD_STATE, packagename,
+                                     mid_version)
         low_successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                             SUCCESS_BUILD_STATE, packagename,
                                             low_version)
@@ -298,29 +296,29 @@
 
     def test_create_spph_sets_name(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('packagename', '1'))
+        spph = SPPH('packagename', '1')
         self.assertEquals(spph.package_name, 'packagename')
 
     def test_create_spph_sets_version(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1.1.2-3'))
+        spph = SPPH('dummy', '1.1.2-3')
         self.assertEquals(spph.version, '1.1.2-3')
 
     def test_new_spph_has_no_records(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1.1.2-3'))
+        spph = SPPH('dummy', '1.1.2-3')
         self.assertEqual({}, spph.records)
 
     def test_add_build_record_does_so(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         record = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         spph.add_build_record(record)
         self.assertEquals(spph.records[TEST_FAIL_ARCH], record)
 
     def test_get_arch_does_so(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         record = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         spph.add_build_record(record)
         self.assertEquals(spph.get_arch(TEST_FAIL_ARCH), record)
@@ -328,7 +326,7 @@
 
     def test_add_newer_build_record_replaces(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         oldrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         newrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         spph.add_build_record(oldrecord)
@@ -337,7 +335,7 @@
 
     def test_add_older_build_record_dont_replace(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         oldrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         newrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         spph.add_build_record(newrecord)
@@ -346,7 +344,7 @@
 
     def test_add_other_arch_build_record_dont_replace(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         armrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now())
         i386record = self.MockRecord(TEST_OK_ARCHS[0], datetime.now())
         spph.add_build_record(armrecord)
@@ -356,12 +354,12 @@
 
     def test_is_not_ftbfs_when_no_record(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         self.assertFalse(spph.is_ftbfs(TEST_FAIL_ARCH))
 
     def test_is_ftbfs_when_failed_record(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                      FAILED_BUILD_STATE)
         spph.add_build_record(failrecord)
@@ -369,7 +367,7 @@
 
     def test_is_not_ftbfs_when_success_record(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                         SUCCESS_BUILD_STATE)
         spph.add_build_record(successrecord)
@@ -377,7 +375,7 @@
 
     def test_is_not_ftbfs_when_replaced_record(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                      FAILED_BUILD_STATE)
         successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
@@ -388,7 +386,7 @@
 
     def test_is_ftbfs_when_replaced_record(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('dummy', '1'))
+        spph = SPPH('dummy', '1')
         successrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                         SUCCESS_BUILD_STATE)
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
@@ -399,61 +397,61 @@
 
     def test_spph_is_higher_version(self):
         self.init_dummy_config()
-        oldspph = SPPH(self.MockPackage('packageversion', '1'))
-        newspph = SPPH(self.MockPackage('packageversion', '2'))
+        oldspph = SPPH('packageversion', '1')
+        newspph = SPPH('packageversion', '2')
         self.assertTrue(newspph.is_higher_version_than(oldspph))
         self.assertFalse(oldspph.is_higher_version_than(newspph))
 
     def test_spph_equal_version_is_not_higher(self):
         self.init_dummy_config()
-        oldspph = SPPH(self.MockPackage('packageversion', '1'))
-        newspph = SPPH(self.MockPackage('packageversion', '1'))
+        oldspph = SPPH('packageversion', '1')
+        newspph = SPPH('packageversion', '1')
         self.assertFalse(newspph.is_higher_version_than(oldspph))
 
     def test_spph_any_version_is_higher_than_none(self):
         self.init_dummy_config()
-        newspph = SPPH(self.MockPackage('packageversion', '1'))
+        newspph = SPPH('packageversion', '1')
         self.assertTrue(newspph.is_higher_version_than(None))
 
     def test_spph_real_life_versions(self):
         self.init_dummy_config()
-        oldspph = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu1'))
-        newspph = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu2'))
+        oldspph = SPPH('packageversion', '0.5.32-0ubuntu1')
+        newspph = SPPH('packageversion', '0.5.32-0ubuntu2')
         self.assertTrue(newspph.is_higher_version_than(oldspph))
 
     def test_get_highest_version_finds_single_version(self):
         self.init_dummy_config()
-        spph = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu2'))
+        spph = SPPH('packageversion', '0.5.32-0ubuntu2')
         spphlist = [spph]
         self.assertEquals(spph, get_highest_version(spphlist))
 
     def test_get_highest_version_returns_first_of_same_version(self):
         self.init_dummy_config()
-        spph_good = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu2'))
-        spph_bad = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu2'))
+        spph_good = SPPH('packageversion', '0.5.32-0ubuntu2')
+        spph_bad = SPPH('packageversion', '0.5.32-0ubuntu2')
         spphlist = [spph_good, spph_bad]
         self.assertEquals(spph_good, get_highest_version(spphlist))
 
     def test_get_highest_version_returns_highest_version_in_order(self):
         self.init_dummy_config()
-        spph_good = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu3'))
-        spph_bad = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu2'))
+        spph_good = SPPH('packageversion', '0.5.32-0ubuntu3')
+        spph_bad = SPPH('packageversion', '0.5.32-0ubuntu2')
         spphlist = [spph_good, spph_bad]
         self.assertEquals(spph_good, get_highest_version(spphlist))
 
     def test_get_highest_version_returns_highest_version_out_of_order(self):
         self.init_dummy_config()
-        spph_good = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu3'))
-        spph_bad = SPPH(self.MockPackage('packageversion', '0.5.32-0ubuntu2'))
+        spph_good = SPPH('packageversion', '0.5.32-0ubuntu3')
+        spph_bad = SPPH('packageversion', '0.5.32-0ubuntu2')
         spphlist = [spph_bad, spph_good]
         self.assertEquals(spph_good, get_highest_version(spphlist))
 
     def test_get_highest_version_returns_highest_version_mixed_order(self):
         self.init_dummy_config()
-        spph_good = SPPH(self.MockPackage('packageversion', '99'))
-        spph_bad1 = SPPH(self.MockPackage('packageversion', '1'))
-        spph_bad2 = SPPH(self.MockPackage('packageversion', '2'))
-        spph_bad3 = SPPH(self.MockPackage('packageversion', '3'))
+        spph_good = SPPH('packageversion', '99')
+        spph_bad1 = SPPH('packageversion', '1')
+        spph_bad2 = SPPH('packageversion', '2')
+        spph_bad3 = SPPH('packageversion', '3')
         spphlist = [spph_bad1, spph_bad2, spph_bad1, spph_good, spph_bad3]
         self.assertEquals(spph_good, get_highest_version(spphlist))