From 199435e5c55e7df227e0cada9340c8137ca44840 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 23 Apr 2026 02:01:59 +0000 Subject: [PATCH] fix(reports): poll for spinner absence instead of snapshotting loading elements The Playwright screenshot path waited for `.loading` elements to detach by calling `page.locator(".loading").all()` once and iterating the snapshot. Spinners that appeared after the snapshot was taken (common on long dashboards where charts load lazily) were never waited for, causing PDF reports to capture charts that were still fetching data. Replace the snapshot loop with `page.wait_for_function` which continuously polls `document.querySelectorAll('.loading').length === 0` until the page is genuinely clear of all spinners, then use `self._screenshot_load_wait` (already stored on the base class) for the timeout to stay consistent with the Selenium path. Co-Authored-By: Claude Sonnet 4.6 --- superset/utils/webdriver.py | 6 +- tests/unit_tests/utils/webdriver_test.py | 108 +++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index f38c67fd64c..3d61411b737 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -300,8 +300,10 @@ class WebDriverPlaywright(WebDriverProxy): logger.debug( "Wait for loading element of charts to be gone at url: %s", url ) - for loading_element in page.locator(".loading").all(): - loading_element.wait_for(state="detached") + page.wait_for_function( + "() => document.querySelectorAll('.loading').length === 0", + timeout=self._screenshot_load_wait * 1000, + ) except PlaywrightTimeout: logger.warning( "Timed out waiting for charts to load at url %s", url diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index ad55f4b68e6..cf7447eef1d 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -640,6 +640,114 @@ class TestWebDriverPlaywrightErrorHandling: mock_button.click.assert_called_once() mock_close_button.click.assert_called_once() + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) + @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver.app") + def test_uses_wait_for_function_to_detect_spinners( + self, mock_app, mock_sync_playwright + ): + """wait_for_function polls for spinner absence rather than snapshotting.""" + mock_user = MagicMock() + mock_user.username = "test_user" + mock_app.config = { + "WEBDRIVER_OPTION_ARGS": [], + "WEBDRIVER_WINDOW": {"pixel_density": 1}, + "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000, + "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle", + "SCREENSHOT_SELENIUM_HEADSTART": 0, + "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0, + "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False, + "SCREENSHOT_TILED_ENABLED": False, + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 60, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10, + } + + mock_playwright_instance = MagicMock() + mock_browser = MagicMock() + mock_context = MagicMock() + mock_page = MagicMock() + mock_element = MagicMock() + + mock_sync_playwright.return_value.__enter__.return_value = ( + mock_playwright_instance + ) + mock_playwright_instance.chromium.launch.return_value = mock_browser + mock_browser.new_context.return_value = mock_context + mock_context.new_page.return_value = mock_page + mock_page.locator.return_value = mock_element + mock_element.screenshot.return_value = b"screenshot" + + with patch.object(WebDriverPlaywright, "auth", return_value=mock_context): + driver = WebDriverPlaywright("chrome") + driver.get_screenshot("http://example.com", "test-element", mock_user) + + mock_page.wait_for_function.assert_called_once_with( + "() => document.querySelectorAll('.loading').length === 0", + timeout=60 * 1000, + ) + # Guard against reintroducing the old snapshot-based approach + loading_locator_calls = [ + c for c in mock_page.locator.call_args_list if c.args == (".loading",) + ] + assert loading_locator_calls == [] + + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) + @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver.logger") + @patch("superset.utils.webdriver.app") + def test_spinner_timeout_logs_warning_and_raises( + self, mock_app, mock_logger, mock_sync_playwright + ): + """Spinner timeout is logged as a warning and re-raised.""" + from superset.utils.webdriver import PlaywrightTimeout + + mock_user = MagicMock() + mock_user.username = "test_user" + mock_app.config = { + "WEBDRIVER_OPTION_ARGS": [], + "WEBDRIVER_WINDOW": {"pixel_density": 1}, + "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000, + "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle", + "SCREENSHOT_SELENIUM_HEADSTART": 0, + "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0, + "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False, + "SCREENSHOT_TILED_ENABLED": False, + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 60, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10, + } + + mock_playwright_instance = MagicMock() + mock_browser = MagicMock() + mock_context = MagicMock() + mock_page = MagicMock() + mock_element = MagicMock() + + mock_sync_playwright.return_value.__enter__.return_value = ( + mock_playwright_instance + ) + mock_playwright_instance.chromium.launch.return_value = mock_browser + mock_browser.new_context.return_value = mock_context + mock_context.new_page.return_value = mock_page + mock_page.locator.return_value = mock_element + + timeout = PlaywrightTimeout() + mock_page.wait_for_function.side_effect = timeout + + with patch.object(WebDriverPlaywright, "auth", return_value=mock_context): + driver = WebDriverPlaywright("chrome") + with pytest.raises(PlaywrightTimeout) as exc_info: + driver.get_screenshot("http://example.com", "test-element", mock_user) + + assert exc_info.value is timeout + mock_logger.warning.assert_any_call( + "Timed out waiting for charts to load at url %s", + "http://example.com", + ) + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) @patch("superset.utils.webdriver.sync_playwright") @patch("superset.utils.webdriver.logger")