From b4ce1ff65e583bdcc88cdc64deb2d6811aedf176 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 16 Jun 2026 10:01:17 -0700 Subject: [PATCH] test(reports): cover SlackResponse.data scope path and backoff recovery Close two gaps in the Slack v1-deprecation tests surfaced in review: - should_use_v2_api scope detection was only exercised with a plain-dict response; the production-default path reads the error code via getattr(response, "data") on a real SlackResponse. Add a case using the existing MockResponse helper so that branch is covered. - The backoff switch to NotificationUnprocessableException is meant to let a transient failure retry and ultimately succeed; only persistent-fail (5x) and never-retry were tested. Add a fail-twice-then-succeed case asserting 3 attempts and the success gauge. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../reports/notifications/slack_tests.py | 36 +++++++++++++++++++ tests/unit_tests/utils/slack_test.py | 32 +++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index f1cb03d8cc7..44ede757734 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -756,6 +756,42 @@ def test_v2_send_retries_on_transient_slack_api_error( assert slack_client_mock.return_value.chat_postMessage.call_count == 5 +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_send_retries_then_succeeds_on_transient_failure( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data, +) -> None: + """The point of switching backoff to NotificationUnprocessableException is + that a *transient* failure now retries and the send ultimately succeeds — + behavior the old (dead) SlackApiError decorator never delivered. Fail twice, + then succeed: send() must return normally after exactly 3 attempts and still + record the success gauge. + """ + flask_global_mock.logs_context = {} + slack_client_mock.return_value.chat_postMessage.side_effect = [ + SlackApiError( + message="rate limited", response={"ok": False, "error": "ratelimited"} + ), + SlackApiError( + message="rate limited", response={"ok": False, "error": "ratelimited"} + ), + {"ok": True}, + ] + + content = _make_content(mock_header_data) + notification = _make_v2_notification(content, target="C12345") + + with patch( + "superset.extensions.stats_logger_manager.instance.gauge" + ) as statsd_mock: + notification.send() + + assert slack_client_mock.return_value.chat_postMessage.call_count == 3 + statsd_mock.assert_called_with("reports.slack.send.ok", 1) + + @patch("superset.reports.notifications.slackv2.g") @patch("superset.reports.notifications.slackv2.get_slack_client") def test_v2_send_does_not_retry_param_errors( diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index efcd88c4678..b2aa55a6ec7 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -325,6 +325,38 @@ class TestShouldUseV2Api: assert "channels:read" in c.args[0] assert "groups:read" in c.args[0] + def test_scope_missing_detected_via_slack_response_data_shape(self, mocker): + """The real Slack SDK sets `SlackApiError.response` to a `SlackResponse` + whose payload lives in `.data` — not a plain dict. This is the + production-default code path, so it must be exercised directly: + `should_use_v2_api` reads the error code via `getattr(response, "data")` + and the scope-missing branch must still fire. + """ + mocker.patch( + "superset.utils.slack.feature_flag_manager.is_feature_enabled", + return_value=True, + ) + mock_client = mocker.Mock() + # MockResponse mirrors SlackResponse: the error payload is on `.data`, + # exactly as the live SDK delivers it. + mock_client.conversations_list.side_effect = SlackApiError( + message="missing_scope", + response=MockResponse({"ok": False, "error": "missing_scope"}), + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + logger_mock = mocker.patch("superset.utils.slack.logger") + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + assert should_use_v2_api() is False + + deprecation_warnings = [ + w for w in caught if issubclass(w.category, DeprecationWarning) + ] + assert len(deprecation_warnings) == 1 + assert logger_mock.warning.call_count == 1 + assert "channels:read" in logger_mock.warning.call_args.args[0] + @pytest.mark.parametrize( "error_code", ["invalid_auth", "ratelimited", "fatal_error", "account_inactive", ""],