From 049dec352120f6469323b3f0ba301661898e449d Mon Sep 17 00:00:00 2001 From: Wing Fung Lau <4760060+hawflau@users.noreply.github.com> Date: Tue, 17 Jan 2023 13:49:40 -0800 Subject: [PATCH] Handled unhandled exception to show bug report message (#4565) * Handled unhandled exception to show bug report message * Fix linting issues * run black reformat * Update message text and add unit test * Run black reformat * add test cases * Update actual exitcode of UnhandledException to 1 to not break existing behavior * remove unused import * address feedback * Update messaging * Update Unit Tests GH Action to make sure Bug_report.md exists * Update bug report * Update Bug Report template * fix typo * Update wording * update test --- .github/ISSUE_TEMPLATE/Bug_report.md | 6 ++++ .github/workflows/build.yml | 1 + samcli/commands/exceptions.py | 42 +++++++++++++++++++++++++ samcli/lib/telemetry/metric.py | 22 ++++++++----- tests/unit/commands/test_exceptions.py | 33 +++++++++++++++++++ tests/unit/lib/telemetry/test_metric.py | 31 ++++++++++++------ 6 files changed, 118 insertions(+), 17 deletions(-) create mode 100644 tests/unit/commands/test_exceptions.py diff --git a/.github/ISSUE_TEMPLATE/Bug_report.md b/.github/ISSUE_TEMPLATE/Bug_report.md index ebc5732342..8d902ac1be 100644 --- a/.github/ISSUE_TEMPLATE/Bug_report.md +++ b/.github/ISSUE_TEMPLATE/Bug_report.md @@ -32,8 +32,14 @@ If you do find an existing Issue, re-open or add a comment to that Issue instead ### Additional environment details (Ex: Windows, Mac, Amazon Linux etc) + + 1. OS: 2. `sam --version`: 3. AWS region: +``` +# Paste the output of `sam --info` here +``` + `Add --debug flag to command you are running` diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 091d1f7d95..e2901ec348 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -25,5 +25,6 @@ jobs: - uses: actions/setup-python@v4 with: python-version: ${{ matrix.python }} + - run: test -f "./.github/ISSUE_TEMPLATE/Bug_report.md" # prevent Bug_report.md from being renamed or deleted - run: make init - run: make pr diff --git a/samcli/commands/exceptions.py b/samcli/commands/exceptions.py index 07229d5063..ea409c40e2 100644 --- a/samcli/commands/exceptions.py +++ b/samcli/commands/exceptions.py @@ -1,6 +1,9 @@ """ Class containing error conditions that are exposed to the user. """ +import traceback +from urllib.parse import quote +from typing import Optional, IO import click @@ -25,6 +28,45 @@ def __init__(self, message, wrapped_from=None): click.ClickException.__init__(self, message) +class UnhandledException(click.ClickException): + """ + Exception class to re-wrap any exception that is not a UserException. + Typically this means there is a bug in SAM CLI. + """ + + GH_ISSUE_SEARCH_URL = "https://github.com/aws/aws-sam-cli/issues?q=is%3Aissue+is%3Aopen+{title}" + GH_BUG_REPORT_URL = "https://github.com/aws/aws-sam-cli/issues/new?template=Bug_report.md&title={title}" + # NOTE (hawflau): actual exitcode is 1 to not break existing behavior. Only report 255 to telemetry + exit_code = 1 + + def __init__(self, command: str, exception: Exception) -> None: + self._command = command + self._exception = exception + self.__traceback__ = self._exception.__traceback__ + + click.ClickException.__init__(self, type(exception).__name__) + + def show(self, file: Optional[IO] = None) -> None: + """Overriding show to customize printing stack trace and message""" + if file is None: + file = click._compat.get_text_stderr() # pylint: disable=protected-access + + tb = "".join(traceback.format_tb(self.__traceback__)) + click.echo(f"\nTraceback:\n{tb}", file=file, err=True) + + encoded_title = quote(f"Bug: {self._command} - {type(self._exception).__name__}") + lookup_url = self.GH_ISSUE_SEARCH_URL.format(title=encoded_title) + create_issue_url = self.GH_BUG_REPORT_URL.format(title=encoded_title) + msg = ( + f'An unexpected error was encountered while executing "{self._command}".\n' + "Search for an existing issue:\n" + f"{lookup_url}\n" + "Or create a bug report:\n" + f"{create_issue_url}" + ) + click.secho(msg, file=file, err=True, fg="yellow") + + class CredentialsError(UserException): """ Exception class when credentials that have been passed are invalid. diff --git a/samcli/lib/telemetry/metric.py b/samcli/lib/telemetry/metric.py index 50c13a2982..86f97fe18a 100644 --- a/samcli/lib/telemetry/metric.py +++ b/samcli/lib/telemetry/metric.py @@ -18,7 +18,7 @@ from samcli.cli.context import Context from samcli.cli.global_config import GlobalConfig from samcli.commands._utils.experimental import get_all_experimental_statues -from samcli.commands.exceptions import UserException +from samcli.commands.exceptions import UserException, UnhandledException from samcli.lib.hook.hook_config import HookPackageConfig from samcli.lib.hook.hook_wrapper import INTERNAL_PACKAGES_ROOT from samcli.lib.hook.exceptions import InvalidHookPackageConfigException @@ -148,16 +148,24 @@ def wrapped(*args, **kwargs): # Execute the function and capture return value. This is returned back by the wrapper # First argument of all commands should be the Context return_value = func(*args, **kwargs) - except UserException as ex: + except ( + UserException, + click.BadOptionUsage, + click.BadArgumentUsage, + click.BadParameter, + click.UsageError, + ) as ex: # Capture exception information and re-raise it later so we can first send metrics. exception = ex - exit_code = ex.exit_code - if ex.wrapped_from is None: - exit_reason = type(ex).__name__ - else: + # TODO (hawflau): review exitcode meaning. We now understand exitcode 1 as errors fixable by users. + exit_code = 1 + if hasattr(ex, "wrapped_from") and ex.wrapped_from: exit_reason = ex.wrapped_from + else: + exit_reason = type(ex).__name__ except Exception as ex: - exception = ex + command = ctx.command_path if ctx else "" + exception = UnhandledException(command, ex) # Standard Unix practice to return exit code 255 on fatal/unhandled exit. exit_code = 255 exit_reason = type(ex).__name__ diff --git a/tests/unit/commands/test_exceptions.py b/tests/unit/commands/test_exceptions.py new file mode 100644 index 0000000000..62104cf0b6 --- /dev/null +++ b/tests/unit/commands/test_exceptions.py @@ -0,0 +1,33 @@ +import io + +from unittest import TestCase +from unittest.mock import patch, Mock + + +from samcli.commands.exceptions import UnhandledException + + +class TestUnhandledException(TestCase): + def test_show_must_print_traceback_and_message(self): + wrapped_exception = None + try: + raise Exception("my_exception") + except Exception as e: + wrapped_exception = e + + unhandled_exception = UnhandledException("test_command", wrapped_exception) + f = io.StringIO() + unhandled_exception.show(f) + + output = f.getvalue() + + self.assertIn("Traceback:", output) + self.assertIn('raise Exception("my_exception")', output) + self.assertIn( + 'An unexpected error was encountered while executing "test_command".\n' + "Search for an existing issue:\n" + "https://github.com/aws/aws-sam-cli/issues?q=is%3Aissue+is%3Aopen+Bug%3A%20test_command%20-%20Exception\n" + "Or create a bug report:\n" + "https://github.com/aws/aws-sam-cli/issues/new?template=Bug_report.md&title=Bug%3A%20test_command%20-%20Exception", + output, + ) diff --git a/tests/unit/lib/telemetry/test_metric.py b/tests/unit/lib/telemetry/test_metric.py index afabb22b04..fa002aebc2 100644 --- a/tests/unit/lib/telemetry/test_metric.py +++ b/tests/unit/lib/telemetry/test_metric.py @@ -1,14 +1,12 @@ import platform import time -from parameterized import parameterized - -import samcli - from unittest import TestCase from unittest.mock import patch, Mock, ANY, call import pytest +import click +from parameterized import parameterized from samcli.lib.hook.exceptions import InvalidHookPackageConfigException, InvalidHookWrapperException from samcli.lib.iac.plugins_interfaces import ProjectTypes @@ -28,7 +26,7 @@ ProjectDetails, _send_command_run_metrics, ) -from samcli.commands.exceptions import UserException +from samcli.commands.exceptions import UserException, UnhandledException class TestSendInstalledMetric(TestCase): @@ -184,6 +182,10 @@ def real_fn(): (KeyError("IO Error test"), "KeyError", 255), (UserException("Something went wrong", wrapped_from="CustomException"), "CustomException", 1), (UserException("Something went wrong"), "UserException", 1), + (click.BadOptionUsage("--my-opt", "Wrong option usage"), "BadOptionUsage", 1), + (click.BadArgumentUsage("Wrong argument usage"), "BadArgumentUsage", 1), + (click.BadParameter("Bad param"), "BadParameter", 1), + (click.UsageError("UsageError"), "UsageError", 1), ] ) @patch("samcli.lib.telemetry.metric.Context") @@ -194,12 +196,13 @@ def test_records_exceptions(self, exception, expected_exception, expected_code, def real_fn(): raise exception - with self.assertRaises(exception.__class__) as context: + expected_exception_class = exception.__class__ if expected_code != 255 else UnhandledException + with self.assertRaises(expected_exception_class) as context: track_command(real_fn)() self.assertEqual( context.exception, exception, - "Must re-raise the original exception object " "without modification", + "Must re-raise the original exception object without modification", ) run_metrics_mock.assert_called_with(self.context_mock, ANY, expected_exception, expected_code) @@ -257,21 +260,29 @@ def func(**kwargs): (KeyError, 255), (UserException, 1), (InvalidHookWrapperException, 1), + (click.BadOptionUsage, 1), + (click.BadArgumentUsage, 1), + (click.BadParameter, 1), + (click.UsageError, 1), ] ) @patch("samcli.lib.telemetry.metric.Context") @patch("samcli.lib.telemetry.metric._send_command_run_metrics") - def test_context_contains_exception(self, expected_exception, expected_code, run_metrics_mock, context_mock): - self.context_mock.exception = expected_exception("some exception") + def test_context_contains_exception(self, exception, exitcode, run_metrics_mock, context_mock): + if exception == click.BadOptionUsage: + self.context_mock.exception = exception("--opt", "some exception") + else: + self.context_mock.exception = exception("some exception") context_mock.get_current_context.return_value = self.context_mock mocked_func = Mock() + expected_exception = exception if exitcode == 1 else UnhandledException with self.assertRaises(expected_exception): track_command(mocked_func)() mocked_func.assert_not_called() - run_metrics_mock.assert_called_with(self.context_mock, ANY, expected_exception.__name__, expected_code) + run_metrics_mock.assert_called_with(self.context_mock, ANY, exception.__name__, exitcode) class TestSendCommandMetrics(TestCase):