Merge lp:~matsubara/tarmac/bundle-lander into lp:~launchpad/tarmac/lp-tarmac

[Diogo Matsubara]

Implements a lock mechanism and a way to properly format the test results similar to the way they are formatted in ec2test and email those results to merge proposal subscribers.

This branch implements a simple lock mechanism for the bundle merge command and implements a way to properly format the test resuls similar to the way they are formatted in ec2test and email those results to merge proposal subscribers.
All tests pass.

[Gary Poster (gary)]

Lock file: here are some things that would be nice, in decreasing order of importance in my mind.
 1 lock file is actually a pid file. pid is reported in error message. This gives SAs a chance to quickly see if it is dead or not.
 2 if lock file exists and has been around for fewer than N seconds, exit with status code 0. If lock file has been around for greater than or equal to N seconds, exit with a non-0 status code. N should be configurable. This gives SAs a chance to care about serious problems, and ignore typical problems.
 3 if lock file exists, error includes how long it has been around. That's easy enough to do without (just look at the file yourself), but a nice convenience.
 4 if lock file exists, with pid, and identified pid doesn't exist, delete the pid file, mention what you are doing, and proceed to run tarmac.

FWIW, lib/canonical/lazr/pidfile.py has some simple code to steal for #1.

I think I'll say that I want #1 unless you convince me otherwise. I'd be happy if you threw in #2 and #3, but I'm fine with ignoring it for now. I don't think you should pursue #4 right now.

Email: cool. A small note is that "(See the attached file for the complete log)" will be included in the merge proposal output, and that will be incorrect. Might be mildly confusing. Would be nice to omit it in the merge proposal, and only include it when sending the mail.

Nice tests. It test_run_failure, is it possible/reasonable to show that the merge proposal was changed, and that emails were sent? That seems like it would be nice.

[Gary Poster (gary)]

And by the way, very nice branch. :-) Thank you.

[Diogo Matsubara (matsubara)]

On Mon, Dec 13, 2010 at 3:03 PM, Gary Poster <email address hidden> wrote:
> Lock file: here are some things that would be nice, in decreasing order of importance in my mind.
>  1 lock file is actually a pid file.  pid is reported in error message.  This gives SAs a chance to quickly see if it is dead or not.

I did this one by reusing Tarmac's PID_FILE in the config.

>  2 if lock file exists and has been around for fewer than N seconds, exit with status code 0.  If lock file has been around for greater than or equal to N seconds, exit with a non-0 status code.  N should be configurable.  This gives SAs a chance to care about serious problems, and ignore typical problems.

Done. SAs need to change the tarmac config file to look like:

allowed_lock_period = X where X is the amount of seconds allowed for
the lock to be considered OK.

If the lock file is in place for less than the amount configured,
tarmac will return and log the amount. If the lock file is in place
for more than the amount configured, tarmac will return an error and
show the PID and the lock file age.

>  3 if lock file exists, error includes how long it has been around.  That's easy enough to do without (just look at the file yourself), but a nice convenience.

Done. It'll log the lock file age and pid as explained above.

>  4 if lock file exists, with pid, and identified pid doesn't exist, delete the pid file, mention what you are doing, and proceed to run tarmac.

Didn't do this one.

>
> FWIW, lib/canonical/lazr/pidfile.py has some simple code to steal for #1.
>
> I think I'll say that I want #1 unless you convince me otherwise.  I'd be happy if you threw in #2 and #3, but I'm fine with ignoring it for now.  I don't think you should pursue #4 right now.
>
> Email: cool.  A small note is that "(See the attached file for the complete log)" will be included in the merge proposal output, and that will be incorrect.  Might be mildly confusing.  Would be nice to omit it in the merge proposal, and only include it when sending the mail.

Done.

>
> Nice tests.  It test_run_failure, is it possible/reasonable to show that the merge proposal was changed, and that emails were sent?  That seems like it would be nice.

Done.

Thanks for the review.
--
Diogo M. Matsubara

[Diogo Matsubara (matsubara)]

Hi Gary,

based in our conversation on IRC I added tests to the lock file mechanism and to the functions in the pidfile.py module. I removed an XXX in the bundle merge command test to make it more complete.

Let me know what you think and if this is ok to be merged.

Thanks for the detailed review and help.

[Gary Poster (gary)]

Hi Diogo. We talked about a couple of things that will probably be important for merging this into tarmac trunk, but since we are not doing that now, I'm not worrying about it yet. Thank you!

Here are the notes I have that might be pertinent when we consider merging this to tarmac trunk.

 - We are reusing some sort of tarmac pid configuration value that didn't seem to actually be used in tarmac. Therefore, we are not sure if what we are doing makes sense in the larger context.

 - We are only using the lock for certain commands, not help. Does that make sense, or should we make the lock pervasive, or pervasive for everything other than help? I don't know, and it's not necessary to figure out the answer yet, but it will be when a merge is possible.

review: Approve

[Diogo Matsubara (matsubara)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2010-12-15 16:32:58 +0000
+++ tarmac/bin/commands.py	2010-12-15 16:55:13 +0000
@@ -2,6 +2,7 @@
 import httplib2
 import logging
 import os
+import time
 
 from bzrlib.commands import Command
 from bzrlib.help import help_commands
@@ -14,6 +15,7 @@
 from tarmac.log import set_up_debug_logging, set_up_logging
 from tarmac.exceptions import TarmacCommandError, TarmacMergeError
 from tarmac.plugin import load_plugins
+from tarmac.pidfile import get_pid, make_pidfile
 
 
 class TarmacCommand(Command):
@@ -235,6 +237,24 @@
     """
 
     def _do_merges(self, branch_url, imply_commit_message):
+        existing_pid = get_pid()
+        if existing_pid is None:
+            make_pidfile()
+        else:
+            # Check how long the lock has been there.
+            lock_creation_time = os.stat(self.config.PID_FILE)[-1]
+            now = time.time()
+            locked_period = now - lock_creation_time
+            # If the locked_period is greater than the allowed, it probably 
+            # means the test suite is stuck.
+            message = 'Tarmac lock file in place for %d seconds, PID: %d'
+            if locked_period > int(self.config.allowed_lock_period):
+                self.logger.info(message % (locked_period, get_pid()))
+                raise TarmacCommandError(message % (locked_period, get_pid()))
+            else:
+                self.logger.info(message % (locked_period, get_pid()))
+                return
+
         target = Branch.create(
             self.launchpad.branches.getByUrl(url=branch_url), self.config,
             imply_commit_message=imply_commit_message)
 
=== added file 'tarmac/pidfile.py'
--- tarmac/pidfile.py	1970-01-01 00:00:00 +0000
+++ tarmac/pidfile.py	2010-12-15 16:55:13 +0000
@@ -0,0 +1,77 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# Code from lib/canonical/lazr/pidfile.py modified to be used here.
+
+__metaclass__ = type
+
+import tempfile
+import os
+import atexit
+import sys
+from signal import signal, SIGTERM
+
+from tarmac.config import TarmacConfig
+
+
+def make_pidfile():
+    """Write the current process id to a PID file.
+
+    Also installs an atexit handler to remove the file on process termination.
+
+    Also installs a SIGTERM signal handler to remove the file on SIGTERM.
+    If you install your own handler, you will want to call remove_pidfile
+    inside it.
+    """
+    pidfile = TarmacConfig().PID_FILE
+    if os.path.exists(pidfile):
+        raise RuntimeError("PID file %s already exists. Already running?" %
+                pidfile)
+
+    atexit.register(remove_pidfile)
+    def remove_pidfile_handler(*ignored):
+        sys.exit(-1 * SIGTERM)
+    signal(SIGTERM, remove_pidfile_handler)
+
+    fd, tempname = tempfile.mkstemp(dir=os.path.dirname(pidfile))
+    outf = os.fdopen(fd, 'w')
+    outf.write(str(os.getpid())+'\n')
+    outf.flush()
+    outf.close()
+    os.rename(tempname, pidfile)
+
+
+def remove_pidfile():
+    """Remove the PID file.
+
+    This should only be needed if you are overriding the default SIGTERM
+    signal handler.
+    """
+    pidfile = TarmacConfig().PID_FILE
+    pid = get_pid()
+    if pid is None:
+        return
+    if pid == os.getpid():
+        os.unlink(pidfile)
+
+
+def get_pid():
+    """Return the PID as an integer, or None
+
+    May raise a ValueError if the PID file is corrupt.
+
+    This method will only be needed by service or monitoring scripts.
+
+    Currently no checking is done to ensure that the process is actually
+    running, is healthy, or died horribly a while ago and its PID being
+    used by something else. What we have is probably good enough.
+    """
+    pidfile = TarmacConfig().PID_FILE
+    try:
+        pid = open(pidfile).read()
+        return int(pid)
+    except IOError:
+        return None
+    except ValueError:
+        raise ValueError("Invalid PID %s" % repr(pid))
+
 
=== modified file 'tarmac/plugins/bundlecommand.py'
--- tarmac/plugins/bundlecommand.py	2010-11-29 13:51:42 +0000
+++ tarmac/plugins/bundlecommand.py	2010-12-15 16:55:13 +0000
@@ -20,16 +20,101 @@
 os = None
 subprocess = None
 
+import datetime
+from email import MIMEMultipart, MIMEText
+from email.mime.application import MIMEApplication
+import gzip
+from StringIO import StringIO
+import tempfile
+import unittest
+
+import bzrlib.config
+
 from bzrlib.lazy_import import lazy_import
+from bzrlib.smtp_connection import SMTPConnection
 lazy_import(globals(), '''
     import os
     import subprocess
     ''')
 
+import subunit
+
 from tarmac.exceptions import TarmacMergeError
 from tarmac.hooks import TarmacHookPoint, tarmac_hooks
 from tarmac.plugins import TarmacPlugin
 
+_double_line = '=' * 70
+_single_line = '-' * 70
+
+def format_error(flavor, test, error):
+    return '\n'.join(
+        [_double_line,
+         '%s: %s' % (flavor, test),
+         _single_line,
+         error,
+         ''])
+
+
+class TestResultFormatter:
+
+    def __init__(self, start_time, end_time, result, proposal):
+        self.result = result
+        self.start_time = start_time
+        self.source_branch = proposal.source_branch.display_name
+        self.source_revno = proposal.source_branch.revision_count
+        self.target_branch = proposal.target_branch.display_name
+        self.target_revno = proposal.target_branch.revision_count
+        self.num_tests = self.result.testsRun
+        self.duration = end_time - start_time
+        self.num_failures = len(self.result.failures)
+        self.num_errors = len(self.result.errors)
+
+    def _format_test_list(self, header, tests):
+        if not tests:
+            return []
+        tests = ['  ' + test.id() for test, error in tests]
+        return [header, '-' * len(header)] + tests + ['']
+
+    def get_full_result(self):
+        return '\n'.join(self.full_result)
+
+    def format_result(self, attached_log=False):
+        output = [
+            'Tests started at approximately %s' % self.start_time,
+            ]
+        output.append('Source: %s r%s' % (
+            self.source_branch, self.source_revno))
+        output.append('Target: %s r%s' % (
+            self.target_branch, self.target_revno))
+        output.extend([
+            '',
+            '%s tests run in %s, %s failures, %s errors' % (
+                self.num_tests, self.duration, self.num_failures,
+                self.num_errors),
+            '',
+            ])
+
+        bad_tests = (
+            self._format_test_list('Failing tests', self.result.failures) +
+            self._format_test_list('Tests with errors', self.result.errors))
+        output.extend(bad_tests)
+
+        if bad_tests:
+            email_summary = StringIO()
+            for test, error in self.result.failures:
+                email_summary.write(
+                    format_error('FAILURE', test, error))
+            for test, error in self.result.errors:
+                email_summary.write(
+                    format_error('ERROR', test, error))
+            output.append(email_summary.getvalue())
+
+        # Include notification about the attached log. Useful when the output
+        # is sent as an email.
+        if attached_log:
+            output.extend(['(See the attached file for the complete log)', ''])
+        return '\n'.join(output)
+
 
 class VerifyCommandFailed(TarmacMergeError):
     """Running the verify_command failed."""
@@ -37,6 +122,12 @@
 
 class BundleCommand(TarmacPlugin):
 
+    def __init__(self, smtp_connection=None):
+        super(BundleCommand, self).__init__()
+        if smtp_connection is None:
+            smtp_connection = SMTPConnection(bzrlib.config.GlobalConfig())
+        self._smtp_connection = smtp_connection
+
     def run(self, command, target, collected_proposals):
         """Plugin for running the registered bundle verify command on target.
 
@@ -58,45 +149,84 @@
             except AttributeError:
                 return
 
+        start_time = datetime.datetime.utcnow()
         self.logger.debug('Running test command: %s' % self.verify_command)
         cwd = os.getcwd()
         os.chdir(target.tree.abspath(''))
-        proc = subprocess.Popen(
-            self.verify_command,
-            shell=True,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE)
-        stdout_value, stderr_value = proc.communicate()
-        return_code = proc.wait()
-        os.chdir(cwd)
-        self.logger.debug('Completed test command: %s' % self.verify_command)
-
-        if return_code != 0:
-            for source, proposal in collected_proposals:
-                self.do_failed(proposal, stdout_value, stderr_value)
-            message = u'Test command "%s" failed.' % self.verify_command
-            comment = (
-                u'Bundle lander failed to land proposals: %s' %
-                collected_proposals)
-            raise VerifyCommandFailed(message, comment)
-        else:
-            self.logger.info('==========')
-            self.logger.info(stdout_value)
-
-    def do_failed(self, proposal, stdout_value, stderr_value):
+        output = tempfile.TemporaryFile()
+        try:
+            proc = subprocess.Popen(
+                self.verify_command,
+                shell=True,
+                stdout=output,
+                stderr=subprocess.STDOUT)
+            return_code = proc.wait()
+            output.seek(0)
+            os.chdir(cwd)
+            self.logger.debug('Completed test command: %s' % self.verify_command)
+            end_time = datetime.datetime.utcnow()
+
+            if return_code != 0:
+                for source, proposal in collected_proposals:
+                    self.do_failed(
+                        start_time, end_time, proposal, output)
+                message = u'Test command "%s" failed.' % self.verify_command
+                comment = (
+                    u'Bundle lander failed to land proposals: %s' %
+                    collected_proposals)
+                raise VerifyCommandFailed(message, comment)
+            else:
+                self.logger.info('==========')
+                self.logger.info(output.read())
+        finally:
+            output.close()
+
+    def do_failed(self, start_time, end_time, proposal, output):
         '''Perform failure tests.
 
-        In this case, the output of the test command is posted as a comment,
-        and the merge proposal is then set to "Needs review" so that Tarmac
-        doesn't attempt to merge it again without human interaction.
+        In this case, the full failure is emailed to the proposal subscribers,
+        a summary of the test failure is posted as a comment and the
+        proposal is then set to "Needs Review" so that Tarmac doesn't attempt
+        to merge it again without human interaction.
         '''
+        # Make output into a Result object, and create a tgz of the
+        # full log.
+        # We will use these for the gzip.
+        fd, path = tempfile.mkstemp()
+        os.close(fd)
+        gz = gzip.open(path, 'wb')
+        # These are for creating the test result object.
+        result = unittest.TestResult()
+        discard = tempfile.TemporaryFile()
+        # The temporary file gets things that subunit doesn't understand.
+        # It prints them to stdout by default.  We don't want the output on
+        # stdout, so we get it and discard it.  (Note that the things subunit
+        # doesn't understand are still put in the full_result, so that the
+        # email will include them.)
+        subunit_server = subunit.TestProtocolServer(result, discard)
+        for line in output:
+            subunit_server.lineReceived(line)
+            gz.write(line)
+        gz.close()
+        try:
+            gzipped_log = open(path).read()
+        finally:
+            os.unlink(path)
+        formatter = TestResultFormatter(
+            start_time, end_time, result, proposal)
+        output.seek(0)
+
+        emails = self.get_subscribers_emails(proposal)
+        self.send_report_email(emails, formatter, gzipped_log)
+
+        summary = formatter.format_result()
         message = u'Test command "%s" failed.' % self.verify_command
         comment = (u'The attempt to merge %(source)s into %(target)s failed. '
-                   u'Below is the output from the failed tests.\n\n'
-                   u'%(output)s') % {
+                   u'Below is a summary from the failed tests.\n\n'
+                   u'%(summary)s') % {
             'source': proposal.source_branch.display_name,
             'target': proposal.target_branch.display_name,
-            'output': u'\n'.join([stdout_value, stderr_value]),
+            'summary': summary,
             }
 
         self.logger.warn(
@@ -111,9 +241,64 @@
 
         proposal.createComment(subject=subject,
                                content=comment)
-        proposal.setStatus(status=u'Needs review')
+        proposal.setStatus(status=u'Needs Review')
         proposal.lp_save()
 
+    def get_subscribers_emails(self, proposal):
+        """Return a list of the proposal subscribers emails."""
+        subscribers_emails = set()
+        # Get all the reviewers.
+        for vote in proposal.votes:
+            subscribers_emails.add(
+                vote.reviewer.preferred_email_address.email)
+        # Get source branch subscribers.
+        for subscriber in proposal.source_branch.subscribers:
+            subscribers_emails.add(subscriber.preferred_email_address.email)
+        return subscribers_emails
+
+    def send_report_email(self, emails, formatter, full_log_gz):
+        """Send an email summarizing the test results.
+
+        :param emails: The list of emails to mail to.
+        :param formatter: The TestResultFormatter object.
+        :param full_test_result: The full log.
+        """
+        message = self._build_report_email(emails, formatter, full_log_gz)
+        self._send_email(message)
+
+    def _build_report_email(self, emails, formatter, full_log_gz):
+        """Build a MIME email summarizing the test results.
+
+        :param emails: The list of emails to mail to.
+        :param formatter: The TestResultFormatter object.
+        :param full_log_gz: A gzip of the full log.
+        """
+        message = MIMEMultipart.MIMEMultipart()
+        message['To'] = ', '.join(emails)
+        message['From'] = "Tarmac Bundle lander."
+        subject = 'Test results: %s => %s: %s' % (
+            formatter.source_branch, formatter.target_branch, "FAILURE")
+        message['Subject'] = subject
+
+        body_text = formatter.format_result(attached_log=True)
+        # Make the body.
+        body = MIMEText.MIMEText(body_text, 'plain', 'utf8')
+        body['Content-Disposition'] = 'inline'
+        message.attach(body)
+
+        # Attach the gzipped log.
+        zipped_log = MIMEApplication(full_log_gz, 'x-gzip')
+        zipped_log.add_header(
+            'Content-Disposition', 'attachment',
+            filename='%s-r%s.subunit.gz' % (
+                formatter.source_branch, formatter.source_revno))
+        message.attach(zipped_log)
+        return message
+
+    def _send_email(self, message):
+        """Actually send 'message'."""
+        self._smtp_connection.send_email(message)
+
 # Create the hook here so we don't change the tarmac_hooks API with this very
 # specific hook.
 tarmac_hooks.create_hook(TarmacHookPoint('tarmac_bundle_merge',
 
=== added file 'tarmac/plugins/tests/test_bundlecommand.py'
--- tarmac/plugins/tests/test_bundlecommand.py	1970-01-01 00:00:00 +0000
+++ tarmac/plugins/tests/test_bundlecommand.py	2010-12-15 16:55:13 +0000
@@ -0,0 +1,280 @@
+"""Tests for the BundleCommmand plug-in."""
+
+from datetime import datetime, timedelta
+import doctest
+import os
+import unittest
+
+from testtools import TestCase, TestResult
+from testtools.matchers import DocTestMatches
+
+from tarmac.bin.registry import CommandRegistry
+from tarmac.plugins.bundlecommand import (
+    BundleCommand, TestResultFormatter, VerifyCommandFailed)
+from tarmac.tests import TarmacTestCase
+from tarmac.tests.test_commands import FakeCommand
+from tarmac.tests.mock import Thing, MockMergeProposal
+
+
+class LoggingSMTPConnection(object):
+    """An SMTPConnection double that logs sent email."""
+
+    def __init__(self, log):
+        self._log = log
+
+    def send_email(self, message):
+        self._log.append(message)
+
+
+class Helpers:
+
+    def make_formatter(self):
+        proposal = Thing(
+            source_branch=Thing(
+                revision_count=123,
+                display_name="lp:project/source"),
+            target_branch=Thing(
+                revision_count=120,
+                display_name="lp:project"))
+        start_time = datetime.utcnow()
+        end_time = start_time + timedelta(hours=1)
+
+        class SomeTest(TestCase):
+            def test_ok(self):
+                pass
+            def test_fail(self):
+                self.fail("oh no")
+            def test_error(self):
+                1/0
+        self.fail_test = SomeTest('test_fail')
+        self.error_test = SomeTest('test_error')
+        test = unittest.TestSuite(
+            [self.fail_test, self.error_test, SomeTest('test_ok')])
+        result = TestResult()
+        test.run(result)
+
+        return TestResultFormatter(start_time, end_time, result, proposal)
+
+    def lp_save(self, *args, **kwargs):
+        """Dummy lp_save method."""
+        pass
+
+
+class TestTestResultFormatter(TestCase, Helpers):
+    """Tests for `TestResultFormatter`."""
+
+    expected_failure_summary_email = ("""\
+Tests started at approximately %(start_time)s
+Source: %(source_branch)s r%(source_revno)s
+Target: %(target_branch)s r%(target_revno)s
+<BLANKLINE>
+%(num_tests)s tests run in %(duration)s, %(num_failures)s failures, %(num_errors)s errors
+<BLANKLINE>
+Failing tests
+-------------
+  %(fail_id)s
+<BLANKLINE>
+Tests with errors
+-----------------
+  %(error_id)s
+<BLANKLINE>
+======================================================================
+FAILURE: test_fail...
+----------------------------------------------------------------------
+Text attachment: traceback
+------------
+Traceback (most recent call last):
+...
+------------
+<BLANKLINE>
+======================================================================
+ERROR: test_error...
+----------------------------------------------------------------------
+Text attachment: traceback
+------------
+Traceback (most recent call last):
+...
+------------
+<BLANKLINE>
+<BLANKLINE>
+(See the attached file for the complete log)
+""")
+
+    expected_failure_summary = ("""\
+Tests started at approximately %(start_time)s
+Source: %(source_branch)s r%(source_revno)s
+Target: %(target_branch)s r%(target_revno)s
+<BLANKLINE>
+%(num_tests)s tests run in %(duration)s, %(num_failures)s failures, %(num_errors)s errors
+<BLANKLINE>
+Failing tests
+-------------
+  %(fail_id)s
+<BLANKLINE>
+Tests with errors
+-----------------
+  %(error_id)s
+<BLANKLINE>
+======================================================================
+FAILURE: test_fail...
+----------------------------------------------------------------------
+Text attachment: traceback
+------------
+Traceback (most recent call last):
+...
+------------
+<BLANKLINE>
+======================================================================
+ERROR: test_error...
+----------------------------------------------------------------------
+Text attachment: traceback
+------------
+Traceback (most recent call last):
+...
+------------
+<BLANKLINE>
+""")
+
+    def test_format_result_with_attached_log(self):
+        formatter = self.make_formatter()
+        summary = formatter.format_result(attached_log=True)
+        data = {
+            'start_time': formatter.start_time,
+            'source_branch': formatter.source_branch,
+            'source_revno': formatter.source_revno,
+            'target_branch': formatter.target_branch,
+            'target_revno': formatter.target_revno,
+            'num_tests': formatter.num_tests,
+            'duration': formatter.duration,
+            'num_failures': formatter.num_failures,
+            'num_errors': formatter.num_errors,
+            'fail_id': self.fail_test.id(),
+            'error_id': self.error_test.id()
+            }
+        self.assertThat(summary, DocTestMatches(
+            self.expected_failure_summary_email % data, doctest.REPORT_NDIFF|
+            doctest.ELLIPSIS))
+
+    def test_format_result_without_attached_log(self):
+        formatter = self.make_formatter()
+        summary = formatter.format_result(attached_log=False)
+        data = {
+            'start_time': formatter.start_time,
+            'source_branch': formatter.source_branch,
+            'source_revno': formatter.source_revno,
+            'target_branch': formatter.target_branch,
+            'target_revno': formatter.target_revno,
+            'num_tests': formatter.num_tests,
+            'duration': formatter.duration,
+            'num_failures': formatter.num_failures,
+            'num_errors': formatter.num_errors,
+            'fail_id': self.fail_test.id(),
+            'error_id': self.error_test.id()
+            }
+        self.assertThat(summary, DocTestMatches(
+            self.expected_failure_summary % data, doctest.REPORT_NDIFF|
+            doctest.ELLIPSIS))
+
+
+class BundleCommandTests(TarmacTestCase, Helpers):
+    """Test the BundleCommand."""
+
+    def setUp(self):
+        """Set up additional data we need for tests."""
+        super(BundleCommandTests, self).setUp()
+        self.collected_proposals = [
+            (Thing(),
+            MockMergeProposal(
+                Thing(
+                    display_name='lp:project/source',
+                    revision_count=123,
+                    subscribers=[
+                        Thing(preferred_email_address=Thing(
+                            email='[e-mail]'))]),
+                Thing(
+                    display_name='lp:project',
+                    revision_count=120),
+                status="Approved",
+                votes=[
+                    Thing(reviewer=Thing(
+                        preferred_email_address=Thing(
+                        email='[e-mail]')))])),
+            (Thing(),
+            MockMergeProposal(
+                Thing(
+                    display_name='lp:project/source2',
+                    revision_count=124,
+                    subscribers=[
+                        Thing(preferred_email_address=Thing(
+                            email='[e-mail]'))]),
+                Thing(
+                    display_name='lp:project',
+                    revision_count=120),
+                status="Approved",
+                votes=[
+                    Thing(reviewer=Thing(
+                        preferred_email_address=Thing(
+                        email='[e-mail]')))])),
+            ]
+        # Patch the plugin smtp_connection so emails are never actually sent.
+        email_log = []
+        smtp_connection = LoggingSMTPConnection(email_log)
+        self.plugin = BundleCommand(smtp_connection=smtp_connection)
+        registry = CommandRegistry(config=self.config)
+        self.command = FakeCommand(registry)
+
+    def test_run(self):
+        """Test that the plug-in run without errors."""
+        target = Thing(config=Thing(
+            bundle_verify_command="/bin/true"),
+            tree=Thing(abspath=os.path.abspath))
+        self.plugin.run(
+            command=self.command, target=target,
+            collected_proposals=self.collected_proposals)
+
+    def test_run_failure(self):
+        """Test that a failure raises the correct exception."""
+        # No email in the log.
+        self.assertEqual(self.plugin._smtp_connection._log, [])
+        # Proposal status are approved.
+        proposal1, proposal2 = self.collected_proposals
+        self.assertEqual(proposal1[1].queue_status, "Approved")
+        self.assertEqual(proposal2[1].queue_status, "Approved")
+
+        target = Thing(config=Thing(
+            bundle_verify_command="/bin/false"),
+            tree=Thing(abspath=os.path.abspath))
+        self.assertRaises(VerifyCommandFailed,
+            self.plugin.run,
+            command=self.command, target=target,
+            collected_proposals=self.collected_proposals)
+
+        # There are two emails in the log. One for each proposal.
+        email = self.plugin._smtp_connection._log.pop()
+        self.assertEqual(
+            email.get('Subject'),
+            'Test results: lp:project/source2 => lp:project: FAILURE')
+        email = self.plugin._smtp_connection._log.pop()
+        self.assertEqual(
+            email.get('Subject'),
+            'Test results: lp:project/source => lp:project: FAILURE')
+
+        # Proposals status is updated.
+        proposal1, proposal2 = self.collected_proposals
+        self.assertEqual(proposal1[1].queue_status, "Needs Review")
+        self.assertEqual(proposal2[1].queue_status, "Needs Review")
+
+    def test_send_report_email(self):
+        """Test send_report_email()."""
+        formatter = self.make_formatter()
+
+        emails = self.plugin.get_subscribers_emails(
+            self.collected_proposals[0][1])
+        self.plugin.send_report_email(
+            emails, formatter, 'some gzip content')
+        email = self.plugin._smtp_connection._log.pop()
+        self.assertEqual(
+            email.get('To'), '[e-mail], [e-mail]')
+        self.assertEqual(
+            email.get('Subject'),
+            "Test results: lp:project/source => lp:project: FAILURE")
 
=== modified file 'tarmac/tests/mock.py'
--- tarmac/tests/mock.py	2010-11-10 14:00:22 +0000
+++ tarmac/tests/mock.py	2010-12-15 16:55:13 +0000
@@ -58,6 +58,23 @@
             self.landing_candidates = []
 
 
+class MockMergeProposal(object):
+    """A mock LP merge proposal."""
+
+    def __init__(self, source_branch, target_branch, status, votes=()):
+        self.createComment = self.lp_save
+        self.queue_status = status
+        self.source_branch = source_branch
+        self.target_branch = target_branch
+        self.votes = votes
+
+    def lp_save(self, *args, **kwargs):
+        pass
+
+    def setStatus(self, status="Needs Review"):
+        self.queue_status = status
+
+
 class cmd_mock(TarmacCommand):
     '''A mock command.'''
 
 
=== modified file 'tarmac/tests/test_commands.py'
--- tarmac/tests/test_commands.py	2010-12-08 00:14:43 +0000
+++ tarmac/tests/test_commands.py	2010-12-15 16:55:13 +0000
@@ -1,18 +1,19 @@
 '''Tests for tarmac.bin.commands.py.'''
 from cStringIO import StringIO
 import os
-import shutil
 import sys
 
 from tarmac.bin import commands
 from tarmac.bin.registry import CommandRegistry
 from tarmac.branch import Branch
 from tarmac.config import TarmacConfig
-from tarmac.exceptions import TarmacMergeError, UnapprovedChanges
+from tarmac.exceptions import (
+    TarmacCommandError, TarmacMergeError, UnapprovedChanges)
 from tarmac.hooks import TarmacHookPoint, tarmac_hooks
+from tarmac.pidfile import make_pidfile
 from tarmac.plugins import TarmacPlugin
 from tarmac.tests import TarmacTestCase, BranchTestCase
-from tarmac.tests.mock import MockLPBranch, Thing
+from tarmac.tests.mock import Thing
 
 
 class FakeCommand(commands.TarmacCommand):
@@ -329,6 +330,8 @@
 
         self.launchpad = Thing(branches=Thing(getByUrl=self.getBranchByUrl))
         self.error = None
+        # Add the allowed_lock_period option to the config.
+        self.config.set('Tarmac', 'allowed_lock_period', '10')
         registry = CommandRegistry(config=self.config)
         registry.register_command('bundle_merge', commands.cmd_bundle_merge)
 
@@ -412,8 +415,21 @@
         self.assertEqual(
             rev.properties.get('merge_url'),
             u'http://code.launchpad.net/proposal2')
-        #XXX need to grab the previuous revision as well and do the
-        # assertions.
+
+        previous_to_last_revno = target.revno() - 1
+        previous_to_last_rev = target.get_rev_id(previous_to_last_revno)
+        rev = target.repository.get_revision(previous_to_last_rev)
+        self.assertEqual(rev.get_summary(), u"Commit this.")
+        self.assertEqual(rev.committer, "Tarmac")
+        self.assertEqual(
+            rev.properties.get('merge_url'),
+            u'http://code.launchpad.net/proposal1')
+
+    def test_run_locked_fails(self):
+        make_pidfile()
+        self.config.set('Tarmac', 'allowed_lock_period', '0')
+        self.assertRaises(
+            TarmacCommandError, self.command.run, launchpad=self.launchpad)
 
     def test_plugin_merge_errors_are_handled(self):
         # Setup
 
=== added file 'tarmac/tests/test_pidfile.py'
--- tarmac/tests/test_pidfile.py	1970-01-01 00:00:00 +0000
+++ tarmac/tests/test_pidfile.py	2010-12-15 16:55:13 +0000
@@ -0,0 +1,31 @@
+
+import os
+
+from tarmac.pidfile import make_pidfile, get_pid, remove_pidfile
+from tarmac.tests import TarmacTestCase
+
+
+class TestPidFile(TarmacTestCase):
+    """Tests for the functions in the pidfile.py module."""
+
+    def test_make_pidfile(self):
+        self.assertFalse(os.path.exists(self.config.PID_FILE))
+        make_pidfile()
+        self.assertTrue(os.path.exists(self.config.PID_FILE))
+
+    def test_make_pidfile_raises_if_exists(self):
+        self.assertFalse(os.path.exists(self.config.PID_FILE))
+        make_pidfile()
+        self.assertRaises(RuntimeError, make_pidfile)
+
+    def test_get_pid(self):
+        self.assertFalse(os.path.exists(self.config.PID_FILE))
+        make_pidfile()
+        self.assertEqual(os.getpid(), get_pid())
+
+    def test_remove_pidfile(self):
+        self.assertFalse(os.path.exists(self.config.PID_FILE))
+        make_pidfile()
+        self.assertTrue(os.path.exists(self.config.PID_FILE))
+        remove_pidfile()
+        self.assertFalse(os.path.exists(self.config.PID_FILE))