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

Proposed by Diogo Matsubara on 2010-12-10
Status: Merged
Approved by: Diogo Matsubara on 2010-12-15
Approved revision: 398
Merged at revision: 384
Proposed branch: lp:~matsubara/tarmac/bundle-lander
Merge into: lp:~launchpad/tarmac/lp-tarmac
Diff against target: 834 lines (+661/-35)
7 files modified
tarmac/bin/commands.py (+20/-0)
tarmac/pidfile.py (+77/-0)
tarmac/plugins/bundlecommand.py (+215/-30)
tarmac/plugins/tests/test_bundlecommand.py (+280/-0)
tarmac/tests/mock.py (+17/-0)
tarmac/tests/test_commands.py (+21/-5)
tarmac/tests/test_pidfile.py (+31/-0)
To merge this branch: bzr merge lp:~matsubara/tarmac/bundle-lander
Reviewer Review Type Date Requested Status
Gary Poster (community) 2010-12-10 Approve on 2010-12-15
Review via email: mp+43396@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Gary Poster (gary) 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.
 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) wrote :

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

Diogo Matsubara (matsubara) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tarmac/bin/commands.py'
2--- tarmac/bin/commands.py 2010-12-15 16:32:58 +0000
3+++ tarmac/bin/commands.py 2010-12-15 16:55:13 +0000
4@@ -2,6 +2,7 @@
5 import httplib2
6 import logging
7 import os
8+import time
9
10 from bzrlib.commands import Command
11 from bzrlib.help import help_commands
12@@ -14,6 +15,7 @@
13 from tarmac.log import set_up_debug_logging, set_up_logging
14 from tarmac.exceptions import TarmacCommandError, TarmacMergeError
15 from tarmac.plugin import load_plugins
16+from tarmac.pidfile import get_pid, make_pidfile
17
18
19 class TarmacCommand(Command):
20@@ -235,6 +237,24 @@
21 """
22
23 def _do_merges(self, branch_url, imply_commit_message):
24+ existing_pid = get_pid()
25+ if existing_pid is None:
26+ make_pidfile()
27+ else:
28+ # Check how long the lock has been there.
29+ lock_creation_time = os.stat(self.config.PID_FILE)[-1]
30+ now = time.time()
31+ locked_period = now - lock_creation_time
32+ # If the locked_period is greater than the allowed, it probably
33+ # means the test suite is stuck.
34+ message = 'Tarmac lock file in place for %d seconds, PID: %d'
35+ if locked_period > int(self.config.allowed_lock_period):
36+ self.logger.info(message % (locked_period, get_pid()))
37+ raise TarmacCommandError(message % (locked_period, get_pid()))
38+ else:
39+ self.logger.info(message % (locked_period, get_pid()))
40+ return
41+
42 target = Branch.create(
43 self.launchpad.branches.getByUrl(url=branch_url), self.config,
44 imply_commit_message=imply_commit_message)
45
46=== added file 'tarmac/pidfile.py'
47--- tarmac/pidfile.py 1970-01-01 00:00:00 +0000
48+++ tarmac/pidfile.py 2010-12-15 16:55:13 +0000
49@@ -0,0 +1,77 @@
50+# Copyright 2009 Canonical Ltd. This software is licensed under the
51+# GNU Affero General Public License version 3 (see the file LICENSE).
52+
53+# Code from lib/canonical/lazr/pidfile.py modified to be used here.
54+
55+__metaclass__ = type
56+
57+import tempfile
58+import os
59+import atexit
60+import sys
61+from signal import signal, SIGTERM
62+
63+from tarmac.config import TarmacConfig
64+
65+
66+def make_pidfile():
67+ """Write the current process id to a PID file.
68+
69+ Also installs an atexit handler to remove the file on process termination.
70+
71+ Also installs a SIGTERM signal handler to remove the file on SIGTERM.
72+ If you install your own handler, you will want to call remove_pidfile
73+ inside it.
74+ """
75+ pidfile = TarmacConfig().PID_FILE
76+ if os.path.exists(pidfile):
77+ raise RuntimeError("PID file %s already exists. Already running?" %
78+ pidfile)
79+
80+ atexit.register(remove_pidfile)
81+ def remove_pidfile_handler(*ignored):
82+ sys.exit(-1 * SIGTERM)
83+ signal(SIGTERM, remove_pidfile_handler)
84+
85+ fd, tempname = tempfile.mkstemp(dir=os.path.dirname(pidfile))
86+ outf = os.fdopen(fd, 'w')
87+ outf.write(str(os.getpid())+'\n')
88+ outf.flush()
89+ outf.close()
90+ os.rename(tempname, pidfile)
91+
92+
93+def remove_pidfile():
94+ """Remove the PID file.
95+
96+ This should only be needed if you are overriding the default SIGTERM
97+ signal handler.
98+ """
99+ pidfile = TarmacConfig().PID_FILE
100+ pid = get_pid()
101+ if pid is None:
102+ return
103+ if pid == os.getpid():
104+ os.unlink(pidfile)
105+
106+
107+def get_pid():
108+ """Return the PID as an integer, or None
109+
110+ May raise a ValueError if the PID file is corrupt.
111+
112+ This method will only be needed by service or monitoring scripts.
113+
114+ Currently no checking is done to ensure that the process is actually
115+ running, is healthy, or died horribly a while ago and its PID being
116+ used by something else. What we have is probably good enough.
117+ """
118+ pidfile = TarmacConfig().PID_FILE
119+ try:
120+ pid = open(pidfile).read()
121+ return int(pid)
122+ except IOError:
123+ return None
124+ except ValueError:
125+ raise ValueError("Invalid PID %s" % repr(pid))
126+
127
128=== modified file 'tarmac/plugins/bundlecommand.py'
129--- tarmac/plugins/bundlecommand.py 2010-11-29 13:51:42 +0000
130+++ tarmac/plugins/bundlecommand.py 2010-12-15 16:55:13 +0000
131@@ -20,16 +20,101 @@
132 os = None
133 subprocess = None
134
135+import datetime
136+from email import MIMEMultipart, MIMEText
137+from email.mime.application import MIMEApplication
138+import gzip
139+from StringIO import StringIO
140+import tempfile
141+import unittest
142+
143+import bzrlib.config
144+
145 from bzrlib.lazy_import import lazy_import
146+from bzrlib.smtp_connection import SMTPConnection
147 lazy_import(globals(), '''
148 import os
149 import subprocess
150 ''')
151
152+import subunit
153+
154 from tarmac.exceptions import TarmacMergeError
155 from tarmac.hooks import TarmacHookPoint, tarmac_hooks
156 from tarmac.plugins import TarmacPlugin
157
158+_double_line = '=' * 70
159+_single_line = '-' * 70
160+
161+def format_error(flavor, test, error):
162+ return '\n'.join(
163+ [_double_line,
164+ '%s: %s' % (flavor, test),
165+ _single_line,
166+ error,
167+ ''])
168+
169+
170+class TestResultFormatter:
171+
172+ def __init__(self, start_time, end_time, result, proposal):
173+ self.result = result
174+ self.start_time = start_time
175+ self.source_branch = proposal.source_branch.display_name
176+ self.source_revno = proposal.source_branch.revision_count
177+ self.target_branch = proposal.target_branch.display_name
178+ self.target_revno = proposal.target_branch.revision_count
179+ self.num_tests = self.result.testsRun
180+ self.duration = end_time - start_time
181+ self.num_failures = len(self.result.failures)
182+ self.num_errors = len(self.result.errors)
183+
184+ def _format_test_list(self, header, tests):
185+ if not tests:
186+ return []
187+ tests = [' ' + test.id() for test, error in tests]
188+ return [header, '-' * len(header)] + tests + ['']
189+
190+ def get_full_result(self):
191+ return '\n'.join(self.full_result)
192+
193+ def format_result(self, attached_log=False):
194+ output = [
195+ 'Tests started at approximately %s' % self.start_time,
196+ ]
197+ output.append('Source: %s r%s' % (
198+ self.source_branch, self.source_revno))
199+ output.append('Target: %s r%s' % (
200+ self.target_branch, self.target_revno))
201+ output.extend([
202+ '',
203+ '%s tests run in %s, %s failures, %s errors' % (
204+ self.num_tests, self.duration, self.num_failures,
205+ self.num_errors),
206+ '',
207+ ])
208+
209+ bad_tests = (
210+ self._format_test_list('Failing tests', self.result.failures) +
211+ self._format_test_list('Tests with errors', self.result.errors))
212+ output.extend(bad_tests)
213+
214+ if bad_tests:
215+ email_summary = StringIO()
216+ for test, error in self.result.failures:
217+ email_summary.write(
218+ format_error('FAILURE', test, error))
219+ for test, error in self.result.errors:
220+ email_summary.write(
221+ format_error('ERROR', test, error))
222+ output.append(email_summary.getvalue())
223+
224+ # Include notification about the attached log. Useful when the output
225+ # is sent as an email.
226+ if attached_log:
227+ output.extend(['(See the attached file for the complete log)', ''])
228+ return '\n'.join(output)
229+
230
231 class VerifyCommandFailed(TarmacMergeError):
232 """Running the verify_command failed."""
233@@ -37,6 +122,12 @@
234
235 class BundleCommand(TarmacPlugin):
236
237+ def __init__(self, smtp_connection=None):
238+ super(BundleCommand, self).__init__()
239+ if smtp_connection is None:
240+ smtp_connection = SMTPConnection(bzrlib.config.GlobalConfig())
241+ self._smtp_connection = smtp_connection
242+
243 def run(self, command, target, collected_proposals):
244 """Plugin for running the registered bundle verify command on target.
245
246@@ -58,45 +149,84 @@
247 except AttributeError:
248 return
249
250+ start_time = datetime.datetime.utcnow()
251 self.logger.debug('Running test command: %s' % self.verify_command)
252 cwd = os.getcwd()
253 os.chdir(target.tree.abspath(''))
254- proc = subprocess.Popen(
255- self.verify_command,
256- shell=True,
257- stdout=subprocess.PIPE,
258- stderr=subprocess.PIPE)
259- stdout_value, stderr_value = proc.communicate()
260- return_code = proc.wait()
261- os.chdir(cwd)
262- self.logger.debug('Completed test command: %s' % self.verify_command)
263-
264- if return_code != 0:
265- for source, proposal in collected_proposals:
266- self.do_failed(proposal, stdout_value, stderr_value)
267- message = u'Test command "%s" failed.' % self.verify_command
268- comment = (
269- u'Bundle lander failed to land proposals: %s' %
270- collected_proposals)
271- raise VerifyCommandFailed(message, comment)
272- else:
273- self.logger.info('==========')
274- self.logger.info(stdout_value)
275-
276- def do_failed(self, proposal, stdout_value, stderr_value):
277+ output = tempfile.TemporaryFile()
278+ try:
279+ proc = subprocess.Popen(
280+ self.verify_command,
281+ shell=True,
282+ stdout=output,
283+ stderr=subprocess.STDOUT)
284+ return_code = proc.wait()
285+ output.seek(0)
286+ os.chdir(cwd)
287+ self.logger.debug('Completed test command: %s' % self.verify_command)
288+ end_time = datetime.datetime.utcnow()
289+
290+ if return_code != 0:
291+ for source, proposal in collected_proposals:
292+ self.do_failed(
293+ start_time, end_time, proposal, output)
294+ message = u'Test command "%s" failed.' % self.verify_command
295+ comment = (
296+ u'Bundle lander failed to land proposals: %s' %
297+ collected_proposals)
298+ raise VerifyCommandFailed(message, comment)
299+ else:
300+ self.logger.info('==========')
301+ self.logger.info(output.read())
302+ finally:
303+ output.close()
304+
305+ def do_failed(self, start_time, end_time, proposal, output):
306 '''Perform failure tests.
307
308- In this case, the output of the test command is posted as a comment,
309- and the merge proposal is then set to "Needs review" so that Tarmac
310- doesn't attempt to merge it again without human interaction.
311+ In this case, the full failure is emailed to the proposal subscribers,
312+ a summary of the test failure is posted as a comment and the
313+ proposal is then set to "Needs Review" so that Tarmac doesn't attempt
314+ to merge it again without human interaction.
315 '''
316+ # Make output into a Result object, and create a tgz of the
317+ # full log.
318+ # We will use these for the gzip.
319+ fd, path = tempfile.mkstemp()
320+ os.close(fd)
321+ gz = gzip.open(path, 'wb')
322+ # These are for creating the test result object.
323+ result = unittest.TestResult()
324+ discard = tempfile.TemporaryFile()
325+ # The temporary file gets things that subunit doesn't understand.
326+ # It prints them to stdout by default. We don't want the output on
327+ # stdout, so we get it and discard it. (Note that the things subunit
328+ # doesn't understand are still put in the full_result, so that the
329+ # email will include them.)
330+ subunit_server = subunit.TestProtocolServer(result, discard)
331+ for line in output:
332+ subunit_server.lineReceived(line)
333+ gz.write(line)
334+ gz.close()
335+ try:
336+ gzipped_log = open(path).read()
337+ finally:
338+ os.unlink(path)
339+ formatter = TestResultFormatter(
340+ start_time, end_time, result, proposal)
341+ output.seek(0)
342+
343+ emails = self.get_subscribers_emails(proposal)
344+ self.send_report_email(emails, formatter, gzipped_log)
345+
346+ summary = formatter.format_result()
347 message = u'Test command "%s" failed.' % self.verify_command
348 comment = (u'The attempt to merge %(source)s into %(target)s failed. '
349- u'Below is the output from the failed tests.\n\n'
350- u'%(output)s') % {
351+ u'Below is a summary from the failed tests.\n\n'
352+ u'%(summary)s') % {
353 'source': proposal.source_branch.display_name,
354 'target': proposal.target_branch.display_name,
355- 'output': u'\n'.join([stdout_value, stderr_value]),
356+ 'summary': summary,
357 }
358
359 self.logger.warn(
360@@ -111,9 +241,64 @@
361
362 proposal.createComment(subject=subject,
363 content=comment)
364- proposal.setStatus(status=u'Needs review')
365+ proposal.setStatus(status=u'Needs Review')
366 proposal.lp_save()
367
368+ def get_subscribers_emails(self, proposal):
369+ """Return a list of the proposal subscribers emails."""
370+ subscribers_emails = set()
371+ # Get all the reviewers.
372+ for vote in proposal.votes:
373+ subscribers_emails.add(
374+ vote.reviewer.preferred_email_address.email)
375+ # Get source branch subscribers.
376+ for subscriber in proposal.source_branch.subscribers:
377+ subscribers_emails.add(subscriber.preferred_email_address.email)
378+ return subscribers_emails
379+
380+ def send_report_email(self, emails, formatter, full_log_gz):
381+ """Send an email summarizing the test results.
382+
383+ :param emails: The list of emails to mail to.
384+ :param formatter: The TestResultFormatter object.
385+ :param full_test_result: The full log.
386+ """
387+ message = self._build_report_email(emails, formatter, full_log_gz)
388+ self._send_email(message)
389+
390+ def _build_report_email(self, emails, formatter, full_log_gz):
391+ """Build a MIME email summarizing the test results.
392+
393+ :param emails: The list of emails to mail to.
394+ :param formatter: The TestResultFormatter object.
395+ :param full_log_gz: A gzip of the full log.
396+ """
397+ message = MIMEMultipart.MIMEMultipart()
398+ message['To'] = ', '.join(emails)
399+ message['From'] = "Tarmac Bundle lander."
400+ subject = 'Test results: %s => %s: %s' % (
401+ formatter.source_branch, formatter.target_branch, "FAILURE")
402+ message['Subject'] = subject
403+
404+ body_text = formatter.format_result(attached_log=True)
405+ # Make the body.
406+ body = MIMEText.MIMEText(body_text, 'plain', 'utf8')
407+ body['Content-Disposition'] = 'inline'
408+ message.attach(body)
409+
410+ # Attach the gzipped log.
411+ zipped_log = MIMEApplication(full_log_gz, 'x-gzip')
412+ zipped_log.add_header(
413+ 'Content-Disposition', 'attachment',
414+ filename='%s-r%s.subunit.gz' % (
415+ formatter.source_branch, formatter.source_revno))
416+ message.attach(zipped_log)
417+ return message
418+
419+ def _send_email(self, message):
420+ """Actually send 'message'."""
421+ self._smtp_connection.send_email(message)
422+
423 # Create the hook here so we don't change the tarmac_hooks API with this very
424 # specific hook.
425 tarmac_hooks.create_hook(TarmacHookPoint('tarmac_bundle_merge',
426
427=== added file 'tarmac/plugins/tests/test_bundlecommand.py'
428--- tarmac/plugins/tests/test_bundlecommand.py 1970-01-01 00:00:00 +0000
429+++ tarmac/plugins/tests/test_bundlecommand.py 2010-12-15 16:55:13 +0000
430@@ -0,0 +1,280 @@
431+"""Tests for the BundleCommmand plug-in."""
432+
433+from datetime import datetime, timedelta
434+import doctest
435+import os
436+import unittest
437+
438+from testtools import TestCase, TestResult
439+from testtools.matchers import DocTestMatches
440+
441+from tarmac.bin.registry import CommandRegistry
442+from tarmac.plugins.bundlecommand import (
443+ BundleCommand, TestResultFormatter, VerifyCommandFailed)
444+from tarmac.tests import TarmacTestCase
445+from tarmac.tests.test_commands import FakeCommand
446+from tarmac.tests.mock import Thing, MockMergeProposal
447+
448+
449+class LoggingSMTPConnection(object):
450+ """An SMTPConnection double that logs sent email."""
451+
452+ def __init__(self, log):
453+ self._log = log
454+
455+ def send_email(self, message):
456+ self._log.append(message)
457+
458+
459+class Helpers:
460+
461+ def make_formatter(self):
462+ proposal = Thing(
463+ source_branch=Thing(
464+ revision_count=123,
465+ display_name="lp:project/source"),
466+ target_branch=Thing(
467+ revision_count=120,
468+ display_name="lp:project"))
469+ start_time = datetime.utcnow()
470+ end_time = start_time + timedelta(hours=1)
471+
472+ class SomeTest(TestCase):
473+ def test_ok(self):
474+ pass
475+ def test_fail(self):
476+ self.fail("oh no")
477+ def test_error(self):
478+ 1/0
479+ self.fail_test = SomeTest('test_fail')
480+ self.error_test = SomeTest('test_error')
481+ test = unittest.TestSuite(
482+ [self.fail_test, self.error_test, SomeTest('test_ok')])
483+ result = TestResult()
484+ test.run(result)
485+
486+ return TestResultFormatter(start_time, end_time, result, proposal)
487+
488+ def lp_save(self, *args, **kwargs):
489+ """Dummy lp_save method."""
490+ pass
491+
492+
493+class TestTestResultFormatter(TestCase, Helpers):
494+ """Tests for `TestResultFormatter`."""
495+
496+ expected_failure_summary_email = ("""\
497+Tests started at approximately %(start_time)s
498+Source: %(source_branch)s r%(source_revno)s
499+Target: %(target_branch)s r%(target_revno)s
500+<BLANKLINE>
501+%(num_tests)s tests run in %(duration)s, %(num_failures)s failures, %(num_errors)s errors
502+<BLANKLINE>
503+Failing tests
504+-------------
505+ %(fail_id)s
506+<BLANKLINE>
507+Tests with errors
508+-----------------
509+ %(error_id)s
510+<BLANKLINE>
511+======================================================================
512+FAILURE: test_fail...
513+----------------------------------------------------------------------
514+Text attachment: traceback
515+------------
516+Traceback (most recent call last):
517+...
518+------------
519+<BLANKLINE>
520+======================================================================
521+ERROR: test_error...
522+----------------------------------------------------------------------
523+Text attachment: traceback
524+------------
525+Traceback (most recent call last):
526+...
527+------------
528+<BLANKLINE>
529+<BLANKLINE>
530+(See the attached file for the complete log)
531+""")
532+
533+ expected_failure_summary = ("""\
534+Tests started at approximately %(start_time)s
535+Source: %(source_branch)s r%(source_revno)s
536+Target: %(target_branch)s r%(target_revno)s
537+<BLANKLINE>
538+%(num_tests)s tests run in %(duration)s, %(num_failures)s failures, %(num_errors)s errors
539+<BLANKLINE>
540+Failing tests
541+-------------
542+ %(fail_id)s
543+<BLANKLINE>
544+Tests with errors
545+-----------------
546+ %(error_id)s
547+<BLANKLINE>
548+======================================================================
549+FAILURE: test_fail...
550+----------------------------------------------------------------------
551+Text attachment: traceback
552+------------
553+Traceback (most recent call last):
554+...
555+------------
556+<BLANKLINE>
557+======================================================================
558+ERROR: test_error...
559+----------------------------------------------------------------------
560+Text attachment: traceback
561+------------
562+Traceback (most recent call last):
563+...
564+------------
565+<BLANKLINE>
566+""")
567+
568+ def test_format_result_with_attached_log(self):
569+ formatter = self.make_formatter()
570+ summary = formatter.format_result(attached_log=True)
571+ data = {
572+ 'start_time': formatter.start_time,
573+ 'source_branch': formatter.source_branch,
574+ 'source_revno': formatter.source_revno,
575+ 'target_branch': formatter.target_branch,
576+ 'target_revno': formatter.target_revno,
577+ 'num_tests': formatter.num_tests,
578+ 'duration': formatter.duration,
579+ 'num_failures': formatter.num_failures,
580+ 'num_errors': formatter.num_errors,
581+ 'fail_id': self.fail_test.id(),
582+ 'error_id': self.error_test.id()
583+ }
584+ self.assertThat(summary, DocTestMatches(
585+ self.expected_failure_summary_email % data, doctest.REPORT_NDIFF|
586+ doctest.ELLIPSIS))
587+
588+ def test_format_result_without_attached_log(self):
589+ formatter = self.make_formatter()
590+ summary = formatter.format_result(attached_log=False)
591+ data = {
592+ 'start_time': formatter.start_time,
593+ 'source_branch': formatter.source_branch,
594+ 'source_revno': formatter.source_revno,
595+ 'target_branch': formatter.target_branch,
596+ 'target_revno': formatter.target_revno,
597+ 'num_tests': formatter.num_tests,
598+ 'duration': formatter.duration,
599+ 'num_failures': formatter.num_failures,
600+ 'num_errors': formatter.num_errors,
601+ 'fail_id': self.fail_test.id(),
602+ 'error_id': self.error_test.id()
603+ }
604+ self.assertThat(summary, DocTestMatches(
605+ self.expected_failure_summary % data, doctest.REPORT_NDIFF|
606+ doctest.ELLIPSIS))
607+
608+
609+class BundleCommandTests(TarmacTestCase, Helpers):
610+ """Test the BundleCommand."""
611+
612+ def setUp(self):
613+ """Set up additional data we need for tests."""
614+ super(BundleCommandTests, self).setUp()
615+ self.collected_proposals = [
616+ (Thing(),
617+ MockMergeProposal(
618+ Thing(
619+ display_name='lp:project/source',
620+ revision_count=123,
621+ subscribers=[
622+ Thing(preferred_email_address=Thing(
623+ email='foo@example.com'))]),
624+ Thing(
625+ display_name='lp:project',
626+ revision_count=120),
627+ status="Approved",
628+ votes=[
629+ Thing(reviewer=Thing(
630+ preferred_email_address=Thing(
631+ email='foo.bar@example.com')))])),
632+ (Thing(),
633+ MockMergeProposal(
634+ Thing(
635+ display_name='lp:project/source2',
636+ revision_count=124,
637+ subscribers=[
638+ Thing(preferred_email_address=Thing(
639+ email='foo@example.com'))]),
640+ Thing(
641+ display_name='lp:project',
642+ revision_count=120),
643+ status="Approved",
644+ votes=[
645+ Thing(reviewer=Thing(
646+ preferred_email_address=Thing(
647+ email='foo.bar@example.com')))])),
648+ ]
649+ # Patch the plugin smtp_connection so emails are never actually sent.
650+ email_log = []
651+ smtp_connection = LoggingSMTPConnection(email_log)
652+ self.plugin = BundleCommand(smtp_connection=smtp_connection)
653+ registry = CommandRegistry(config=self.config)
654+ self.command = FakeCommand(registry)
655+
656+ def test_run(self):
657+ """Test that the plug-in run without errors."""
658+ target = Thing(config=Thing(
659+ bundle_verify_command="/bin/true"),
660+ tree=Thing(abspath=os.path.abspath))
661+ self.plugin.run(
662+ command=self.command, target=target,
663+ collected_proposals=self.collected_proposals)
664+
665+ def test_run_failure(self):
666+ """Test that a failure raises the correct exception."""
667+ # No email in the log.
668+ self.assertEqual(self.plugin._smtp_connection._log, [])
669+ # Proposal status are approved.
670+ proposal1, proposal2 = self.collected_proposals
671+ self.assertEqual(proposal1[1].queue_status, "Approved")
672+ self.assertEqual(proposal2[1].queue_status, "Approved")
673+
674+ target = Thing(config=Thing(
675+ bundle_verify_command="/bin/false"),
676+ tree=Thing(abspath=os.path.abspath))
677+ self.assertRaises(VerifyCommandFailed,
678+ self.plugin.run,
679+ command=self.command, target=target,
680+ collected_proposals=self.collected_proposals)
681+
682+ # There are two emails in the log. One for each proposal.
683+ email = self.plugin._smtp_connection._log.pop()
684+ self.assertEqual(
685+ email.get('Subject'),
686+ 'Test results: lp:project/source2 => lp:project: FAILURE')
687+ email = self.plugin._smtp_connection._log.pop()
688+ self.assertEqual(
689+ email.get('Subject'),
690+ 'Test results: lp:project/source => lp:project: FAILURE')
691+
692+ # Proposals status is updated.
693+ proposal1, proposal2 = self.collected_proposals
694+ self.assertEqual(proposal1[1].queue_status, "Needs Review")
695+ self.assertEqual(proposal2[1].queue_status, "Needs Review")
696+
697+ def test_send_report_email(self):
698+ """Test send_report_email()."""
699+ formatter = self.make_formatter()
700+
701+ emails = self.plugin.get_subscribers_emails(
702+ self.collected_proposals[0][1])
703+ self.plugin.send_report_email(
704+ emails, formatter, 'some gzip content')
705+ email = self.plugin._smtp_connection._log.pop()
706+ self.assertEqual(
707+ email.get('To'), 'foo@example.com, foo.bar@example.com')
708+ self.assertEqual(
709+ email.get('Subject'),
710+ "Test results: lp:project/source => lp:project: FAILURE")
711
712=== modified file 'tarmac/tests/mock.py'
713--- tarmac/tests/mock.py 2010-11-10 14:00:22 +0000
714+++ tarmac/tests/mock.py 2010-12-15 16:55:13 +0000
715@@ -58,6 +58,23 @@
716 self.landing_candidates = []
717
718
719+class MockMergeProposal(object):
720+ """A mock LP merge proposal."""
721+
722+ def __init__(self, source_branch, target_branch, status, votes=()):
723+ self.createComment = self.lp_save
724+ self.queue_status = status
725+ self.source_branch = source_branch
726+ self.target_branch = target_branch
727+ self.votes = votes
728+
729+ def lp_save(self, *args, **kwargs):
730+ pass
731+
732+ def setStatus(self, status="Needs Review"):
733+ self.queue_status = status
734+
735+
736 class cmd_mock(TarmacCommand):
737 '''A mock command.'''
738
739
740=== modified file 'tarmac/tests/test_commands.py'
741--- tarmac/tests/test_commands.py 2010-12-08 00:14:43 +0000
742+++ tarmac/tests/test_commands.py 2010-12-15 16:55:13 +0000
743@@ -1,18 +1,19 @@
744 '''Tests for tarmac.bin.commands.py.'''
745 from cStringIO import StringIO
746 import os
747-import shutil
748 import sys
749
750 from tarmac.bin import commands
751 from tarmac.bin.registry import CommandRegistry
752 from tarmac.branch import Branch
753 from tarmac.config import TarmacConfig
754-from tarmac.exceptions import TarmacMergeError, UnapprovedChanges
755+from tarmac.exceptions import (
756+ TarmacCommandError, TarmacMergeError, UnapprovedChanges)
757 from tarmac.hooks import TarmacHookPoint, tarmac_hooks
758+from tarmac.pidfile import make_pidfile
759 from tarmac.plugins import TarmacPlugin
760 from tarmac.tests import TarmacTestCase, BranchTestCase
761-from tarmac.tests.mock import MockLPBranch, Thing
762+from tarmac.tests.mock import Thing
763
764
765 class FakeCommand(commands.TarmacCommand):
766@@ -329,6 +330,8 @@
767
768 self.launchpad = Thing(branches=Thing(getByUrl=self.getBranchByUrl))
769 self.error = None
770+ # Add the allowed_lock_period option to the config.
771+ self.config.set('Tarmac', 'allowed_lock_period', '10')
772 registry = CommandRegistry(config=self.config)
773 registry.register_command('bundle_merge', commands.cmd_bundle_merge)
774
775@@ -412,8 +415,21 @@
776 self.assertEqual(
777 rev.properties.get('merge_url'),
778 u'http://code.launchpad.net/proposal2')
779- #XXX need to grab the previuous revision as well and do the
780- # assertions.
781+
782+ previous_to_last_revno = target.revno() - 1
783+ previous_to_last_rev = target.get_rev_id(previous_to_last_revno)
784+ rev = target.repository.get_revision(previous_to_last_rev)
785+ self.assertEqual(rev.get_summary(), u"Commit this.")
786+ self.assertEqual(rev.committer, "Tarmac")
787+ self.assertEqual(
788+ rev.properties.get('merge_url'),
789+ u'http://code.launchpad.net/proposal1')
790+
791+ def test_run_locked_fails(self):
792+ make_pidfile()
793+ self.config.set('Tarmac', 'allowed_lock_period', '0')
794+ self.assertRaises(
795+ TarmacCommandError, self.command.run, launchpad=self.launchpad)
796
797 def test_plugin_merge_errors_are_handled(self):
798 # Setup
799
800=== added file 'tarmac/tests/test_pidfile.py'
801--- tarmac/tests/test_pidfile.py 1970-01-01 00:00:00 +0000
802+++ tarmac/tests/test_pidfile.py 2010-12-15 16:55:13 +0000
803@@ -0,0 +1,31 @@
804+
805+import os
806+
807+from tarmac.pidfile import make_pidfile, get_pid, remove_pidfile
808+from tarmac.tests import TarmacTestCase
809+
810+
811+class TestPidFile(TarmacTestCase):
812+ """Tests for the functions in the pidfile.py module."""
813+
814+ def test_make_pidfile(self):
815+ self.assertFalse(os.path.exists(self.config.PID_FILE))
816+ make_pidfile()
817+ self.assertTrue(os.path.exists(self.config.PID_FILE))
818+
819+ def test_make_pidfile_raises_if_exists(self):
820+ self.assertFalse(os.path.exists(self.config.PID_FILE))
821+ make_pidfile()
822+ self.assertRaises(RuntimeError, make_pidfile)
823+
824+ def test_get_pid(self):
825+ self.assertFalse(os.path.exists(self.config.PID_FILE))
826+ make_pidfile()
827+ self.assertEqual(os.getpid(), get_pid())
828+
829+ def test_remove_pidfile(self):
830+ self.assertFalse(os.path.exists(self.config.PID_FILE))
831+ make_pidfile()
832+ self.assertTrue(os.path.exists(self.config.PID_FILE))
833+ remove_pidfile()
834+ self.assertFalse(os.path.exists(self.config.PID_FILE))

Subscribers

People subscribed via source and target branches

to all changes: