From 9d77e2cb386dbb1dac089e5f7508b3c3f56fcd5d Mon Sep 17 00:00:00 2001 From: Dean Troyer Date: Fri, 13 Oct 2017 16:52:15 -0500 Subject: 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 --- cliff/app.py | 6 +++--- cliff/interactive.py | 15 ++++++++++++--- cliff/tests/test_app.py | 37 ++++++++++++++++++++++++++----------- cliff/tests/test_interactive.py | 18 ++++++++++++++---- 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 ):" app_cmd_header = "Application commands (type help ):" - 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) -- cgit v1.2.1