From 253570784cdcc85d82142128ce33e3b9d8f8db14 Mon Sep 17 00:00:00 2001 From: David Lord Date: Thu, 30 Jul 2020 18:36:55 -0700 Subject: [PATCH] cleaner message when CLI can't load app When loading the app fails for the --help command, only the error message is shown, then the help text. The full traceback is shown for other exceptions. Also show the message when loading fails while getting a command, instead of only "command not found". The error message goes to stderr to match other error behavior, and is in red with an extra newline to make it more obvious next to the help text. Also fixes an issue with the test_apps fixture that caused an imported app to still be importable after the test was over and the path was reset. Now the module cache is reset as well. --- CHANGES.rst | 2 ++ src/flask/cli.py | 46 ++++++++++++++++++++++------------------------ tests/conftest.py | 12 +++++++++--- tests/test_cli.py | 37 ++++++++++++++++++++++++++----------- 4 files changed, 59 insertions(+), 38 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 73ff5f2e..43a6b5e7 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,6 +12,8 @@ Unreleased - Passing ``script_info`` to app factory functions is deprecated. This was not portable outside the ``flask`` command. Use ``click.get_current_context().obj`` if it's needed. :issue:`3552` +- The CLI shows better error messages when the app failed to load + when looking up commands. :issue:`2741` - Add :meth:`sessions.SessionInterface.get_cookie_name` to allow setting the session cookie name dynamically. :pr:`3369` - Add :meth:`Config.from_file` to load config using arbitrary file diff --git a/src/flask/cli.py b/src/flask/cli.py index caf0dfea..e73f7b78 100644 --- a/src/flask/cli.py +++ b/src/flask/cli.py @@ -536,43 +536,41 @@ class FlaskGroup(AppGroup): def get_command(self, ctx, name): self._load_plugin_commands() + # Look up built-in and plugin commands, which should be + # available even if the app fails to load. + rv = super().get_command(ctx, name) - # We load built-in commands first as these should always be the - # same no matter what the app does. If the app does want to - # override this it needs to make a custom instance of this group - # and not attach the default commands. - # - # This also means that the script stays functional in case the - # application completely fails. - rv = AppGroup.get_command(self, ctx, name) if rv is not None: return rv info = ctx.ensure_object(ScriptInfo) + + # Look up commands provided by the app, showing an error and + # continuing if the app couldn't be loaded. try: - rv = info.load_app().cli.get_command(ctx, name) - if rv is not None: - return rv - except NoAppException: - pass + return info.load_app().cli.get_command(ctx, name) + except NoAppException as e: + click.secho(f"Error: {e.format_message()}\n", err=True, fg="red") def list_commands(self, ctx): self._load_plugin_commands() - - # The commands available is the list of both the application (if - # available) plus the builtin commands. - rv = set(click.Group.list_commands(self, ctx)) + # Start with the built-in and plugin commands. + rv = set(super().list_commands(ctx)) info = ctx.ensure_object(ScriptInfo) + + # Add commands provided by the app, showing an error and + # continuing if the app couldn't be loaded. try: rv.update(info.load_app().cli.list_commands(ctx)) + except NoAppException as e: + # When an app couldn't be loaded, show the error message + # without the traceback. + click.secho(f"Error: {e.format_message()}\n", err=True, fg="red") except Exception: - # Here we intentionally swallow all exceptions as we don't - # want the help page to break if the app does not exist. - # If someone attempts to use the command we try to create - # the app again and this will give us the error. - # However, we will not do so silently because that would confuse - # users. - traceback.print_exc() + # When any other errors occurred during loading, show the + # full traceback. + click.secho(f"{traceback.format_exc()}\n", err=True, fg="red") + return sorted(rv) def main(self, *args, **kwargs): diff --git a/tests/conftest.py b/tests/conftest.py index d7a54a66..17ff2f3d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -73,9 +73,15 @@ def client(app): @pytest.fixture def test_apps(monkeypatch): - monkeypatch.syspath_prepend( - os.path.abspath(os.path.join(os.path.dirname(__file__), "test_apps")) - ) + monkeypatch.syspath_prepend(os.path.join(os.path.dirname(__file__), "test_apps")) + original_modules = set(sys.modules.keys()) + + yield + + # Remove any imports cached during the test. Otherwise "import app" + # will work in the next test even though it's no longer on the path. + for key in sys.modules.keys() - original_modules: + sys.modules.pop(key) @pytest.fixture(autouse=True) diff --git a/tests/test_cli.py b/tests/test_cli.py index f3f8aca1..5fb114a4 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -239,7 +239,7 @@ def test_locate_app_raises(test_apps, iname, aname): locate_app(info, iname, aname) -def test_locate_app_suppress_raise(): +def test_locate_app_suppress_raise(test_apps): info = ScriptInfo() app = locate_app(info, "notanapp.py", None, raise_if_not_found=False) assert app is None @@ -396,21 +396,36 @@ def test_flaskgroup_debug(runner, set_debug_flag): assert result.output == f"{not set_debug_flag}\n" -def test_print_exceptions(runner): - """Print the stacktrace if the CLI.""" +def test_no_command_echo_loading_error(): + from flask.cli import cli - def create_app(): - raise Exception("oh no") - return Flask("flaskgroup") + runner = CliRunner(mix_stderr=False) + result = runner.invoke(cli, ["missing"]) + assert result.exit_code == 2 + assert "FLASK_APP" in result.stderr + assert "Usage:" in result.stderr - @click.group(cls=FlaskGroup, create_app=create_app) - def cli(**params): - pass +def test_help_echo_loading_error(): + from flask.cli import cli + + runner = CliRunner(mix_stderr=False) result = runner.invoke(cli, ["--help"]) assert result.exit_code == 0 - assert "Exception: oh no" in result.output - assert "Traceback" in result.output + assert "FLASK_APP" in result.stderr + assert "Usage:" in result.stdout + + +def test_help_echo_exception(): + def create_app(): + raise Exception("oh no") + + cli = FlaskGroup(create_app=create_app) + runner = CliRunner(mix_stderr=False) + result = runner.invoke(cli, ["--help"]) + assert result.exit_code == 0 + assert "Exception: oh no" in result.stderr + assert "Usage:" in result.stdout class TestRoutes: