From 5085bae6bba91de60cfe17b639ea95eda2588829 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Tue, 4 Oct 2022 15:20:23 -0700 Subject: [PATCH 1/5] Add an option to find unexpected errors in report screenshots and replace unexpected errors with real error messages in screenshots --- superset/config.py | 6 ++ superset/utils/webdriver.py | 65 ++++++++++++++++++++- tests/integration_tests/thumbnails_tests.py | 54 +++++++++++++++++ 3 files changed, 124 insertions(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index f163997c6ee4b..a597678e599e5 100644 --- a/superset/config.py +++ b/superset/config.py @@ -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 diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 93e8957f21769..1e4b4c72d10bf 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -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, Optional, Tuple, TYPE_CHECKING, List from flask import current_app from selenium.common.exceptions import ( @@ -141,7 +141,17 @@ def get_screenshot( url, user.username, ) + + if current_app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]: + unexpected_errors = self.find_unexpected_errors(driver) + if unexpected_errors: + logger.warning( + f"{len(unexpected_errors)} errors found in the screenshot. " + f"URL: {url} " + f"Errors are: {unexpected_errors}") + img = element.screenshot_as_png + except TimeoutException: logger.warning("Selenium timed out requesting url %s", url, exc_info=True) except StaleElementReferenceException: @@ -155,3 +165,56 @@ def get_screenshot( finally: self.destroy(driver, current_app.config["SCREENSHOT_SELENIUM_RETRIES"]) return img + + def find_unexpected_errors(self, driver: WebDriver) -> List[str]: + error_messages = [] + + try: + alert_divs = driver.find_elements(By.XPATH, "//div[@role = 'alert']") + logger.debug( + f"{len(alert_divs)} alert elements have been found in the screenshot") + + 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 diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 5eabc4da00090..594e48b1cde13 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -62,6 +62,60 @@ 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.WebDriverProxy.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.WebDriverProxy.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 + + app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False + + @patch("superset.utils.webdriver.WebDriverWait") + @patch("superset.utils.webdriver.firefox") + def test_find_unexpected_errors(self, mock_webdriver, 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"] + ) + webdriver = webdriver_proxy.auth(user) + url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1) + webdriver_proxy.get_screenshot(url, "grid-container", user=user) + + # there is no error in example dashboards, unexpected_errors should return 0 + # and no error should be raised + unexpected_errors = webdriver_proxy.find_unexpected_errors(driver=webdriver) + assert len(unexpected_errors) == 0 + + class TestWebDriverProxy(SupersetTestCase): @patch("superset.utils.webdriver.WebDriverWait") @patch("superset.utils.webdriver.firefox") From 6561b5498ece26f5340fcced606a8a302f4e1bc6 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Thu, 6 Oct 2022 17:04:55 -0700 Subject: [PATCH 2/5] fix pylint warnings --- superset/utils/webdriver.py | 116 ++++++++++---------- tests/integration_tests/thumbnails_tests.py | 4 +- 2 files changed, 61 insertions(+), 59 deletions(-) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 1e4b4c72d10bf..ebac1038240b2 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -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 @@ -143,12 +199,11 @@ def get_screenshot( ) if current_app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]: - unexpected_errors = self.find_unexpected_errors(driver) + unexpected_errors = find_unexpected_errors(driver) if unexpected_errors: logger.warning( - f"{len(unexpected_errors)} errors found in the screenshot. " - f"URL: {url} " - f"Errors are: {unexpected_errors}") + "%i errors found in the screenshot. URL: %s. Errors are: %s", + len(unexpected_errors), url, unexpected_errors) img = element.screenshot_as_png @@ -165,56 +220,3 @@ def get_screenshot( finally: self.destroy(driver, current_app.config["SCREENSHOT_SELENIUM_RETRIES"]) return img - - def find_unexpected_errors(self, driver: WebDriver) -> List[str]: - error_messages = [] - - try: - alert_divs = driver.find_elements(By.XPATH, "//div[@role = 'alert']") - logger.debug( - f"{len(alert_divs)} alert elements have been found in the screenshot") - - 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 diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 594e48b1cde13..b6966c417002b 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -30,7 +30,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 WebDriverProxy, find_unexpected_errors from tests.integration_tests.conftest import with_feature_flags from tests.integration_tests.test_app import app @@ -112,7 +112,7 @@ def test_find_unexpected_errors(self, mock_webdriver, mock_webdriver_wait): # there is no error in example dashboards, unexpected_errors should return 0 # and no error should be raised - unexpected_errors = webdriver_proxy.find_unexpected_errors(driver=webdriver) + unexpected_errors = find_unexpected_errors(driver=webdriver) assert len(unexpected_errors) == 0 From d3f6d1cd0564a5a619bfa72c03e81e849b957092 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Thu, 6 Oct 2022 17:24:42 -0700 Subject: [PATCH 3/5] fix function references after pylint update --- tests/integration_tests/thumbnails_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index b6966c417002b..9a01212bf66ed 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -66,7 +66,7 @@ class TestWebDriverScreenshotErrorDetector(SupersetTestCase): @patch("superset.utils.webdriver.WebDriverWait") @patch("superset.utils.webdriver.firefox") - @patch("superset.utils.webdriver.WebDriverProxy.find_unexpected_errors") + @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 ): @@ -81,7 +81,7 @@ def test_not_call_find_unexpected_errors_if_feature_disabled( @patch("superset.utils.webdriver.WebDriverWait") @patch("superset.utils.webdriver.firefox") - @patch("superset.utils.webdriver.WebDriverProxy.find_unexpected_errors") + @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 ): From ff877d934b2eaf94b685f44e759fd66b7d62223c Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Thu, 6 Oct 2022 17:50:59 -0700 Subject: [PATCH 4/5] fix pre-commit errors --- superset/utils/webdriver.py | 37 +++++++++++---------- tests/integration_tests/thumbnails_tests.py | 3 +- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index ebac1038240b2..991790339a053 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -20,7 +20,7 @@ import logging from enum import Enum from time import sleep -from typing import Any, Dict, Optional, Tuple, TYPE_CHECKING, List +from typing import Any, Dict, List, Optional, Tuple, TYPE_CHECKING from flask import current_app from selenium.common.exceptions import ( @@ -57,8 +57,7 @@ def find_unexpected_errors(driver: WebDriver) -> List[str]: 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) + "%i alert elements have been found in the screenshot", len(alert_divs) ) for alert_div in alert_divs: @@ -66,12 +65,15 @@ def find_unexpected_errors(driver: WebDriver) -> List[str]: 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( + 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] + )[ + 0 + ] err_msg_div = modal.find_element(By.CLASS_NAME, "ant-modal-body") @@ -82,25 +84,23 @@ def find_unexpected_errors(driver: WebDriver) -> List[str]: 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) - ) + 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("'", "\\'") + 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 + f"arguments[0].innerHTML = '{error_as_html}'", alert_div ) except WebDriverException: - logger.warning("Failed to update error messages using alert_div", - exc_info=True) + 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) @@ -203,7 +203,10 @@ def get_screenshot( if unexpected_errors: logger.warning( "%i errors found in the screenshot. URL: %s. Errors are: %s", - len(unexpected_errors), url, unexpected_errors) + len(unexpected_errors), + url, + unexpected_errors, + ) img = element.screenshot_as_png diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 9a01212bf66ed..c8e0ec928dd36 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -30,7 +30,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, find_unexpected_errors +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 @@ -63,7 +63,6 @@ def test_get_async_dashboard_screenshot(self): class TestWebDriverScreenshotErrorDetector(SupersetTestCase): - @patch("superset.utils.webdriver.WebDriverWait") @patch("superset.utils.webdriver.firefox") @patch("superset.utils.webdriver.find_unexpected_errors") From 2c1e87f260df8150ce2ec505d83006fe2100c566 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Wed, 26 Oct 2022 17:55:53 -0700 Subject: [PATCH 5/5] Add a test case where alert_div presents --- tests/integration_tests/thumbnails_tests.py | 41 ++++++++++++++------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index c8e0ec928dd36..81557c7d89b54 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -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 @@ -96,24 +97,36 @@ def test_call_find_unexpected_errors_if_feature_enabled( app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False - @patch("superset.utils.webdriver.WebDriverWait") - @patch("superset.utils.webdriver.firefox") - def test_find_unexpected_errors(self, mock_webdriver, mock_webdriver_wait): - app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = True + def test_find_unexpected_errors_no_alert(self): + webdriver = MagicMock() - webdriver_proxy = WebDriverProxy("firefox") - user = security_manager.get_user_by_username( - app.config["THUMBNAIL_SELENIUM_USER"] - ) - webdriver = webdriver_proxy.auth(user) - url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1) - webdriver_proxy.get_screenshot(url, "grid-container", user=user) + webdriver.find_elements.return_value = [] - # there is no error in example dashboards, unexpected_errors should return 0 - # and no error should be raised 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")