Add an errexit attribute to InteractiveApp to exit on command errors

This only affects interactive mode.

The desire is to enable a behaviour similar to the shell's set +errexit.
To do this required changing function returns in a number of places in
order to propogate the exit code back up to the app.

Change-Id: I1f2606cb43c8064e465e87d6801ed8d169daa26a

4 files changed, 55 insertions, 21 deletions

diff --git a/cliff/app.py b/cliff/app.py
index 94b3d76..3c1cdf4 100644
--- a/cliff/app.py
+++ b/cliff/app.py
@@ -324,8 +324,7 @@ class App(object):
                                                         self.stdin,
                                                         self.stdout,
                                                         )
-        self.interpreter.cmdloop()
-        return 0
+        return self.interpreter.cmdloop()
 
     def get_fuzzy_matches(self, cmd):
         """return fuzzy matches of unknown command
@@ -412,7 +411,8 @@ class App(object):
                 else:
                     self.LOG.error('Could not clean up: %s', err2)
             if self.options.debug:
-                raise
+                # 'raise' here gets caught and does not exit like we want
+                return result
         else:
             try:
                 self.clean_up(cmd, result, None)
diff --git a/cliff/interactive.py b/cliff/interactive.py
index f5afa1b..4341d09 100644
--- a/cliff/interactive.py
+++ b/cliff/interactive.py
@@ -41,7 +41,8 @@ class InteractiveApp(cmd2.Cmd):
     doc_header = "Shell commands (type help <topic>):"
     app_cmd_header = "Application commands (type help <topic>):"
 
-    def __init__(self, parent_app, command_manager, stdin, stdout):
+    def __init__(self, parent_app, command_manager, stdin, stdout,
+                 errexit=False):
         self.parent_app = parent_app
         if not hasattr(sys.stdin, 'isatty') or sys.stdin.isatty():
             self.prompt = '(%s) ' % parent_app.NAME
@@ -49,6 +50,7 @@ class InteractiveApp(cmd2.Cmd):
             # batch/pipe mode
             self.prompt = ''
         self.command_manager = command_manager
+        self.errexit = errexit
         cmd2.Cmd.__init__(self, 'tab', stdin=stdin, stdout=stdout)
 
     def _split_line(self, line):
@@ -69,7 +71,11 @@ class InteractiveApp(cmd2.Cmd):
         # since it already has the logic for executing
         # the subcommand.
         line_parts = self._split_line(line)
-        self.parent_app.run_subcommand(line_parts)
+        ret = self.parent_app.run_subcommand(line_parts)
+        if self.errexit:
+            # Only provide this if errexit is enabled,
+            # otherise keep old behaviour
+            return ret
 
     def completenames(self, text, line, begidx, endidx):
         """Tab-completion for command prefix without completer delimiter.
@@ -175,4 +181,7 @@ class InteractiveApp(cmd2.Cmd):
         return statement
 
     def cmdloop(self):
-        self._cmdloop()
+        # We don't want the cmd2 cmdloop() behaviour, just call the old one
+        # directly.  In part this is because cmd2.cmdloop() doe not return
+        # anything useful and we want to have a useful exit code.
+        return self._cmdloop()
diff --git a/cliff/tests/test_app.py b/cliff/tests/test_app.py
index 35f19c4..88e85b8 100644
--- a/cliff/tests/test_app.py
+++ b/cliff/tests/test_app.py
@@ -74,10 +74,27 @@ class TestInteractiveMode(base.TestBase):
             name='interactive_app_factory'
         )
         self.assertIsNone(app.interpreter)
-        app.run([])
+        ret = app.run([])
+        self.assertIsNotNone(app.interpreter)
+        cmdloop = app.interactive_app_factory.return_value.cmdloop
+        cmdloop.assert_called_once_with()
+        self.assertNotEqual(ret, 0)
+
+    def test_interactive_mode_cmdloop_error(self):
+        app, command = make_app()
+        cmdloop_mock = mock.MagicMock(
+            name='cmdloop',
+        )
+        cmdloop_mock.return_value = 1
+        app.interactive_app_factory = mock.MagicMock(
+            name='interactive_app_factory'
+        )
+        self.assertIsNone(app.interpreter)
+        ret = app.run([])
         self.assertIsNotNone(app.interpreter)
         cmdloop = app.interactive_app_factory.return_value.cmdloop
         cmdloop.assert_called_once_with()
+        self.assertNotEqual(ret, 0)
 
 
 class TestInitAndCleanup(base.TestBase):
@@ -99,14 +116,16 @@ class TestInitAndCleanup(base.TestBase):
     def test_clean_up_success(self):
         app, command = make_app()
         app.clean_up = mock.MagicMock(name='clean_up')
-        app.run(['mock'])
+        ret = app.run(['mock'])
         app.clean_up.assert_called_once_with(command.return_value, 0, None)
+        self.assertEqual(ret, 0)
 
     def test_clean_up_error(self):
         app, command = make_app()
 
         app.clean_up = mock.MagicMock(name='clean_up')
-        app.run(['error'])
+        ret = app.run(['error'])
+        self.assertNotEqual(ret, 0)
 
         app.clean_up.assert_called_once_with(mock.ANY, mock.ANY, mock.ANY)
         call_args = app.clean_up.call_args_list[0]
@@ -119,12 +138,8 @@ class TestInitAndCleanup(base.TestBase):
         app, command = make_app()
 
         app.clean_up = mock.MagicMock(name='clean_up')
-        try:
-            app.run(['--debug', 'error'])
-        except RuntimeError as err:
-            self.assertIs(err, app.clean_up.call_args_list[0][0][2])
-        else:
-            self.fail('Should have had an exception')
+        ret = app.run(['--debug', 'error'])
+        self.assertNotEqual(ret, 0)
 
         self.assertTrue(app.clean_up.called)
         call_args = app.clean_up.call_args_list[0]
@@ -157,7 +172,7 @@ class TestInitAndCleanup(base.TestBase):
             side_effect=RuntimeError('within clean_up'),
         )
         try:
-            app.run(['--debug', 'error'])
+            ret = app.run(['--debug', 'error'])
         except RuntimeError as err:
             if not hasattr(err, '__context__'):
                 # The exception passed to clean_up is not the exception
@@ -166,7 +181,7 @@ class TestInitAndCleanup(base.TestBase):
                 # with the new one as a __context__ attribute.
                 self.assertIsNot(err, app.clean_up.call_args_list[0][0][2])
         else:
-            self.fail('Should have had an exception')
+            self.assertNotEqual(ret, 0)
 
         self.assertTrue(app.clean_up.called)
         call_args = app.clean_up.call_args_list[0]
diff --git a/cliff/tests/test_interactive.py b/cliff/tests/test_interactive.py
index dc4df06..041fffd 100644
--- a/cliff/tests/test_interactive.py
+++ b/cliff/tests/test_interactive.py
@@ -24,13 +24,13 @@ class FakeApp(object):
 
 class TestInteractive(base.TestBase):
 
-    def make_interactive_app(self, *command_names):
+    def make_interactive_app(self, errexit, *command_names):
         fake_command_manager = [(x, None) for x in command_names]
         return InteractiveApp(FakeApp, fake_command_manager,
-                              stdin=None, stdout=None)
+                              stdin=None, stdout=None, errexit=errexit)
 
     def _test_completenames(self, expecteds, prefix):
-        app = self.make_interactive_app('hips', 'hippo', 'nonmatching')
+        app = self.make_interactive_app(False, 'hips', 'hippo', 'nonmatching')
         self.assertEqual(
             set(app.completenames(prefix, '', 0, 1)), set(expecteds))
 
@@ -58,7 +58,7 @@ class TestInteractive(base.TestBase):
     def _test_completedefault(self, expecteds, line, begidx):
         command_names = set(['show file', 'show folder', 'show  long',
                              'list all'])
-        app = self.make_interactive_app(*command_names)
+        app = self.make_interactive_app(False, *command_names)
         observeds = app.completedefault(None, line, begidx, None)
         self.assertEqual(set(expecteds), set(observeds))
         self.assertTrue(
@@ -78,3 +78,13 @@ class TestInteractive(base.TestBase):
 
     def test_no_completedefault(self):
         self._test_completedefault([], 'taz ', 4)
+
+    def test_no_errexit(self):
+        command_names = set(['show file', 'show folder', 'list all'])
+        app = self.make_interactive_app(False, *command_names)
+        self.assertFalse(app.errexit)
+
+    def test_errexit(self):
+        command_names = set(['show file', 'show folder', 'list all'])
+        app = self.make_interactive_app(True, *command_names)
+        self.assertTrue(app.errexit)