command_wrapper: Make error reporting more flexible

We allow to substitute stderr, stdout and error_reason in run_error now.
These changes the API for derived classses slightly so fix them up as
well.

3 files changed, 93 insertions, 16 deletions

diff --git a/gbp/command_wrappers.py b/gbp/command_wrappers.py
index 0880dc1..72dfee5 100644
--- a/gbp/command_wrappers.py
+++ b/gbp/command_wrappers.py
@@ -40,7 +40,7 @@ class Command(object):
                  capture_stdout=False):
         self.cmd = cmd
         self.args = args
-        self.run_error = "'%s' failed" % (" ".join([self.cmd] + self.args))
+        self.run_error = "'%s' failed: {err_reason}" % (" ".join([self.cmd] + self.args))
         self.shell = shell
         self.capture_stdout = capture_stdout
         self.capture_stderr = capture_stderr
@@ -96,11 +96,18 @@ class Command(object):
         return self.retcode
 
     def _log_err(self):
+        "Log an error message"
+        log.err(self._format_err())
+
+    def _format_err(self):
+        """Log an error message
+
+        This allows to replace stdout, stderr and err_reason in
+        the self.run_error.
         """
-        Log an error message allowing to use the captured stout/stderr
-        """
-        log.err("%s: %s" % (self.run_error,
-                            self.err_reason)
+        return self.run_error.format(stdout=self.stdout,
+                                     stderr=self.stderr,
+                                     err_reason=self.err_reason)
 
     def __call__(self, args=[], quiet=False):
         """Run the command and raise exception on errors
@@ -125,7 +132,7 @@ class Command(object):
         >>> Command("/foo/bar")(quiet=True)
         Traceback (most recent call last):
         ...
-        CommandExecFailed: execution failed: [Errno 2] No such file or directory
+        CommandExecFailed: '/foo/bar' failed: execution failed: [Errno 2] No such file or directory
         """
         try:
             ret = self.__call(args)
@@ -134,7 +141,7 @@ class Command(object):
         if ret:
             if not quiet:
               self._log_err()
-            raise CommandExecFailed(self.err_reason)
+            raise CommandExecFailed(self._format_err())
 
 
     def call(self, args, quiet=True):
@@ -207,7 +214,7 @@ class UnpackTarArchive(Command):
 
         Command.__init__(self, 'tar', exclude +
                          ['-C', dir, compression, '-xf', archive ])
-        self.run_error = 'Couldn\'t unpack "%s"' % self.archive
+        self.run_error = 'Couldn\'t unpack "%s": {err_reason}' % self.archive
 
 
 class PackTarArchive(Command):
@@ -222,7 +229,7 @@ class PackTarArchive(Command):
 
         Command.__init__(self, 'tar', exclude +
                          ['-C', dir, compression, '-cf', archive, dest])
-        self.run_error = 'Couldn\'t repack "%s"' % self.archive
+        self.run_error = 'Couldn\'t repack "%s": {err_reason}' % self.archive
 
 
 class CatenateTarArchive(Command):
@@ -240,7 +247,7 @@ class RemoveTree(Command):
     def __init__(self, tree):
         self.tree = tree
         Command.__init__(self, 'rm', [ '-rf', tree ])
-        self.run_error = 'Couldn\'t remove "%s"' % self.tree
+        self.run_error = 'Couldn\'t remove "%s": {err_reason}' % self.tree
 
 
 class Dch(Command):
@@ -250,7 +257,7 @@ class Dch(Command):
         if msg:
             args.append(msg)
         Command.__init__(self, 'dch', args)
-        self.run_error = "Dch failed."
+        self.run_error = "Dch failed: {err_reason}"
 
 
 class DpkgSourceExtract(Command):
@@ -262,7 +269,7 @@ class DpkgSourceExtract(Command):
         Command.__init__(self, 'dpkg-source', ['-x'])
 
     def __call__(self, dsc, output_dir):
-        self.run_error = 'Couldn\'t extract "%s"' % dsc
+        self.run_error = 'Couldn\'t extract "%s": {err_reason}' % dsc
         Command.__call__(self, [dsc, output_dir])
 
 
@@ -273,14 +280,14 @@ class UnpackZipArchive(Command):
         self.dir = dir
 
         Command.__init__(self, 'unzip', [ "-q", archive, '-d', dir ])
-        self.run_error = 'Couldn\'t unpack "%s"' % self.archive
+        self.run_error = 'Couldn\'t unpack "%s": {err_reason}' % self.archive
 
 
 class GitCommand(Command):
     "Mother/Father of all git commands"
     def __init__(self, cmd, args=[], **kwargs):
         Command.__init__(self, 'git', [cmd] + args, **kwargs)
-        self.run_error = "Couldn't run git %s" % cmd
+        self.run_error = "Couldn't run git %s: {err_reason}" % cmd
 
 
 # vim:et:ts=4:sw=4:et:sts=4:ai:set list listchars=tab\:»·,trail\:·:
diff --git a/gbp/pkg/pristinetar.py b/gbp/pkg/pristinetar.py
index 03f043d..1c4eb5b 100644
--- a/gbp/pkg/pristinetar.py
+++ b/gbp/pkg/pristinetar.py
@@ -63,7 +63,7 @@ class PristineTar(Command):
         @param archive: the name of the orig archive
         @type archive: C{str}
         """
-        self.run_error = 'Couldn\'t checkout "%s"' % os.path.basename(archive)
+        self.run_error = 'Couldn\'t checkout "%s": {err_reason}' % os.path.basename(archive)
         self.__call__(['checkout', archive])
 
     def commit(self, archive, upstream):
@@ -76,7 +76,7 @@ class PristineTar(Command):
         @param upstream: the upstream branch to diff against
         @type upstream: C{str}
         """
-        self.run_error = ("Couldn't commit to '%s' with upstream '%s'" %
+        self.run_error = ("Couldn't commit to '%s' with upstream '%s': {err_reason}" %
                           (self.branch, upstream))
         self.__call__(['commit', archive, upstream])
 
diff --git a/tests/21_test_command_wrappers.py b/tests/21_test_command_wrappers.py
new file mode 100644
index 0000000..4dfbb48
--- /dev/null
+++ b/tests/21_test_command_wrappers.py
@@ -0,0 +1,70 @@
+# vim: set fileencoding=utf-8 :
+"""Test L{gbp.command_wrappers.Command}'s tarball unpack"""
+
+import unittest
+import mock
+import functools
+
+from gbp.command_wrappers import Command, CommandExecFailed
+from . testutils import GbpLogTester
+
+
+def patch_popen(stdout='', stderr='', returncode=1):
+    """Decorator to easily set the return value of popen.communicate()"""
+    def patch_popen_decorator(func):
+        @functools.wraps(func)
+        def wrap(self):
+            with mock.patch('subprocess.Popen') as create_mock:
+                popen_mock = mock.Mock(**{'returncode': returncode,
+                                          'communicate.return_value': (stdout, stderr)})
+                create_mock.return_value = popen_mock
+                return func(self, create_mock)
+        return wrap
+    return patch_popen_decorator
+
+
+class TestCommandWrapperFailures(unittest.TestCase, GbpLogTester):
+    def setUp(self):
+        self.false = Command('/does/not/matter')
+        self.log_tester = GbpLogTester()
+        self.log_tester._capture_log(True)
+
+    def tearDown(self):
+        self.log_tester._capture_log(False)
+
+    @patch_popen(stdout='', stderr='', returncode=1)
+    def test_log_default_error_msg(self, create_mock):
+        with self.assertRaises(CommandExecFailed):
+            self.false.__call__()
+        self.log_tester._check_log(0, "gbp:error: '/does/not/matter' failed: it exited with 1")
+        self.assertEqual(self.false.retcode, 1)
+        self.assertEqual(self.false.stderr, '')
+        self.assertEqual(self.false.stdout, '')
+
+    @patch_popen(stdout='', stderr='we have a problem', returncode=1)
+    def test_log_use_stderr_for_err_message(self, create_mock):
+        self.false.capture_stderr = True
+        self.false.run_error = "Erpel {stderr}"
+        with self.assertRaises(CommandExecFailed):
+            self.false.__call__()
+        self.log_tester._check_log(0, "gbp:error: Erpel we have a problem")
+        self.assertEqual(self.false.retcode, 1)
+        self.assertEqual(self.false.stderr, 'we have a problem')
+        self.assertEqual(self.false.stdout, '')
+
+    @patch_popen(stdout='we have a problem', stderr='', returncode=1)
+    def test_log_use_stdout_for_err_message(self, create_mock):
+        self.false.capture_stdout = True
+        self.false.run_error = "Erpel {stdout}"
+        with self.assertRaises(CommandExecFailed):
+            self.false.__call__()
+        self.log_tester._check_log(0, "gbp:error: Erpel we have a problem")
+        self.assertEqual(self.false.retcode, 1)
+        self.assertEqual(self.false.stderr, '')
+        self.assertEqual(self.false.stdout, 'we have a problem')
+
+    @patch_popen(returncode=0)
+    def test_no_log_on_success(self, create_mock):
+        self.false.__call__()
+        self.log_tester._check_log_empty()
+        self.assertEqual(self.false.retcode, 0)