Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(report): Capture unexpected errors in report screenshots. Fixes #21653 #21724

Merged
merged 5 commits into from
Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,12 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
SCREENSHOT_SELENIUM_HEADSTART = 3
# Wait for the chart animation, in seconds
SCREENSHOT_SELENIUM_ANIMATION_WAIT = 5
# Replace unexpected errors in screenshots with real error messages
SCREENSHOT_REPLACE_UNEXPECTED_ERRORS = False
# Max time to wait for error message modal to show up, in seconds
SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE = 5
# Max time to wait for error message modal to close, in seconds
SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE = 5

# ---------------------------------------------------
# Image and file configuration
Expand Down
70 changes: 69 additions & 1 deletion superset/utils/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import logging
from enum import Enum
from time import sleep
from typing import Any, Dict, Optional, Tuple, TYPE_CHECKING
from typing import Any, Dict, List, Optional, Tuple, TYPE_CHECKING

from flask import current_app
from selenium.common.exceptions import (
Expand Down Expand Up @@ -51,6 +51,62 @@ class DashboardStandaloneMode(Enum):
REPORT = 3


def find_unexpected_errors(driver: WebDriver) -> List[str]:
error_messages = []

try:
alert_divs = driver.find_elements(By.XPATH, "//div[@role = 'alert']")
logger.debug(
"%i alert elements have been found in the screenshot", len(alert_divs)
)

for alert_div in alert_divs:
# See More button
alert_div.find_element(By.XPATH, ".//*[@role = 'button']").click()

# wait for modal to show up
modal = WebDriverWait(
driver, current_app.config["SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE"]
).until(
EC.visibility_of_any_elements_located(
(By.CLASS_NAME, "ant-modal-content")
)
)[
0
]

err_msg_div = modal.find_element(By.CLASS_NAME, "ant-modal-body")

# collect error message
error_messages.append(err_msg_div.text)

# close modal after collecting error messages
modal.find_element(By.CLASS_NAME, "ant-modal-close").click()

# wait until the modal becomes invisible
WebDriverWait(
driver, current_app.config["SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE"]
).until(EC.invisibility_of_element(modal))

# Use HTML so that error messages are shown in the same style (color)
error_as_html = err_msg_div.get_attribute("innerHTML").replace("'", "\\'")

try:
# Even if some errors can't be updated in the screenshot,
# keep all the errors in the server log and do not fail the loop
driver.execute_script(
f"arguments[0].innerHTML = '{error_as_html}'", alert_div
)
except WebDriverException:
logger.warning(
"Failed to update error messages using alert_div", exc_info=True
)
except WebDriverException:
logger.warning("Failed to capture unexpected errors", exc_info=True)

return error_messages


class WebDriverProxy:
def __init__(self, driver_type: str, window: Optional[WindowSize] = None):
self._driver_type = driver_type
Expand Down Expand Up @@ -141,7 +197,19 @@ def get_screenshot(
url,
user.username,
)

if current_app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]:
unexpected_errors = find_unexpected_errors(driver)
if unexpected_errors:
logger.warning(
"%i errors found in the screenshot. URL: %s. Errors are: %s",
len(unexpected_errors),
url,
unexpected_errors,
)

img = element.screenshot_as_png

except TimeoutException:
logger.warning("Selenium timed out requesting url %s", url, exc_info=True)
except StaleElementReferenceException:
Expand Down
70 changes: 68 additions & 2 deletions tests/integration_tests/thumbnails_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
import urllib.request
from io import BytesIO
from unittest import skipUnless
from unittest.mock import ANY, call, patch
from unittest.mock import ANY, call, MagicMock, patch

import pytest
from flask_testing import LiveServerTestCase
from sqlalchemy.sql import func

Expand All @@ -30,7 +31,7 @@
from superset.models.slice import Slice
from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot
from superset.utils.urls import get_url_host, get_url_path
from superset.utils.webdriver import WebDriverProxy
from superset.utils.webdriver import find_unexpected_errors, WebDriverProxy
from tests.integration_tests.conftest import with_feature_flags
from tests.integration_tests.test_app import app

Expand Down Expand Up @@ -62,6 +63,71 @@ def test_get_async_dashboard_screenshot(self):
self.assertEqual(response.getcode(), 202)


class TestWebDriverScreenshotErrorDetector(SupersetTestCase):
@patch("superset.utils.webdriver.WebDriverWait")
@patch("superset.utils.webdriver.firefox")
@patch("superset.utils.webdriver.find_unexpected_errors")
def test_not_call_find_unexpected_errors_if_feature_disabled(
self, mock_find_unexpected_errors, mock_firefox, mock_webdriver_wait
):
webdriver_proxy = WebDriverProxy("firefox")
user = security_manager.get_user_by_username(
app.config["THUMBNAIL_SELENIUM_USER"]
)
url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1)
webdriver_proxy.get_screenshot(url, "grid-container", user=user)

assert not mock_find_unexpected_errors.called

@patch("superset.utils.webdriver.WebDriverWait")
@patch("superset.utils.webdriver.firefox")
@patch("superset.utils.webdriver.find_unexpected_errors")
def test_call_find_unexpected_errors_if_feature_enabled(
self, mock_find_unexpected_errors, mock_firefox, mock_webdriver_wait
):
app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = True
webdriver_proxy = WebDriverProxy("firefox")
user = security_manager.get_user_by_username(
app.config["THUMBNAIL_SELENIUM_USER"]
)
url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1)
webdriver_proxy.get_screenshot(url, "grid-container", user=user)

assert mock_find_unexpected_errors.called
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please set the config back to it's expected state
app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False


app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False

def test_find_unexpected_errors_no_alert(self):
webdriver = MagicMock()

webdriver.find_elements.return_value = []

unexpected_errors = find_unexpected_errors(driver=webdriver)
assert len(unexpected_errors) == 0

assert "alert" in webdriver.find_elements.call_args_list[0][0][1]

@patch("superset.utils.webdriver.WebDriverWait")
def test_find_unexpected_errors(self, mock_webdriver_wait):
webdriver = MagicMock()
alert_div = MagicMock()

webdriver.find_elements.return_value = [alert_div]
alert_div.find_elements.return_value = MagicMock()

unexpected_errors = find_unexpected_errors(driver=webdriver)
assert len(unexpected_errors) == 1

# attempt to find alerts
assert "alert" in webdriver.find_elements.call_args_list[0][0][1]
# attempt to click on "See more" buttons
assert "button" in alert_div.find_element.call_args_list[0][0][1]
# Wait for error modal to show up and to hide
assert 2 == len(mock_webdriver_wait.call_args_list)
# replace the text in alert div, eg, "unexpected errors"
assert alert_div == webdriver.execute_script.call_args_list[0][0][1]


class TestWebDriverProxy(SupersetTestCase):
@patch("superset.utils.webdriver.WebDriverWait")
@patch("superset.utils.webdriver.firefox")
Expand Down