diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py index dfd2e49f54f1..8deada447e7c 100644 --- a/superset/utils/screenshot_utils.py +++ b/superset/utils/screenshot_utils.py @@ -28,6 +28,11 @@ # Time to wait after scrolling for content to settle and load (in milliseconds) SCROLL_SETTLE_TIMEOUT_MS = 1000 +try: + from playwright.sync_api import TimeoutError as PlaywrightTimeout +except ImportError: + PlaywrightTimeout = Exception + if TYPE_CHECKING: try: from playwright.sync_api import Page @@ -80,7 +85,10 @@ def combine_screenshot_tiles(screenshot_tiles: list[bytes]) -> bytes: def take_tiled_screenshot( - page: "Page", element_name: str, tile_height: int + page: "Page", + element_name: str, + tile_height: int, + load_wait: int = 60, ) -> bytes | None: """ Take a tiled screenshot of a large dashboard by scrolling and capturing sections. @@ -89,6 +97,7 @@ def take_tiled_screenshot( page: Playwright page object element_name: CSS class name of the element to screenshot tile_height: Height of each tile in pixels + load_wait: Seconds to wait for charts to load per tile (default 30) Returns: Combined screenshot bytes or None if failed @@ -139,6 +148,29 @@ def take_tiled_screenshot( ) # Wait for scroll to settle and content to load page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS) + # Wait for any loading spinners visible in the current viewport to clear. + # Only check viewport-visible spinners to avoid blocking on + # virtualization placeholders rendered for off-screen charts. + try: + page.wait_for_function( + """() => { + const els = document.querySelectorAll('.loading'); + for (const el of els) { + const r = el.getBoundingClientRect(); + if (r.top < window.innerHeight && r.bottom > 0) { + return false; + } + } + return true; + }""", + timeout=load_wait * 1000, + ) + except PlaywrightTimeout: + logger.warning( + "Timed out waiting for visible spinners to clear on tile %s/%s", + i + 1, + num_tiles, + ) # Calculate what portion of the element we want to capture for this tile tile_start_in_element = i * tile_height diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index f38c67fd64c7..29a4afc10f9f 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -300,8 +300,19 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n 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( + """() => { + const els = document.querySelectorAll('.loading'); + for (const el of els) { + const r = el.getBoundingClientRect(); + if (r.top < window.innerHeight && r.bottom > 0) { + return false; + } + } + return true; + }""", + timeout=self._screenshot_load_wait * 1000, + ) except PlaywrightTimeout: logger.warning( "Timed out waiting for charts to load at url %s", url @@ -366,7 +377,12 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n page.set_viewport_size( {"height": tile_height, "width": viewport_width} ) - img = take_tiled_screenshot(page, element_name, tile_height) + img = take_tiled_screenshot( + page, + element_name, + tile_height, + load_wait=self._screenshot_load_wait, + ) if img is None: logger.warning( ( diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index f1b970bf83c8..8cdd71066973 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -320,3 +320,61 @@ def test_reset_scroll_position(self, mock_page): # Each wait should use the scroll settle timeout constant for call in mock_page.wait_for_timeout.call_args_list: assert call[0][0] == SCROLL_SETTLE_TIMEOUT_MS + + def test_per_tile_spinner_wait_uses_viewport_check(self, mock_page): + """wait_for_function polls viewport-visible spinners after each scroll.""" + with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"): + take_tiled_screenshot( + mock_page, "dashboard", tile_height=2000, load_wait=30 + ) + + # 3 tiles → 3 wait_for_function calls, one per tile + assert mock_page.wait_for_function.call_count == 3 + + # Each call uses viewport-scoped JS and the load_wait timeout + for call in mock_page.wait_for_function.call_args_list: + js = call[0][0] + assert "getBoundingClientRect" in js + assert "window.innerHeight" in js + assert call[1]["timeout"] == 30 * 1000 + + def test_per_tile_spinner_timeout_logs_warning_and_continues(self, mock_page): + """A per-tile spinner timeout logs a warning but still takes the screenshot.""" + from superset.utils.screenshot_utils import PlaywrightTimeout + + timeout = PlaywrightTimeout() + mock_page.wait_for_function.side_effect = timeout + + with patch("superset.utils.screenshot_utils.logger") as mock_logger: + with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"): + result = take_tiled_screenshot( + mock_page, "dashboard", tile_height=2000, load_wait=30 + ) + + # Screenshot should still proceed (non-fatal) + assert result is not None + # Warning logged for each tile that timed out + assert mock_logger.warning.call_count == 3 + mock_logger.warning.assert_any_call( + "Timed out waiting for visible spinners to clear on tile %s/%s", + 1, + 3, + ) + + def test_per_tile_non_timeout_exceptions_propagate(self, mock_page): + """Non-timeout exceptions from wait_for_function are not swallowed.""" + mock_page.wait_for_function.side_effect = RuntimeError("browser crashed") + + with pytest.raises(RuntimeError, match="browser crashed"): + take_tiled_screenshot( + mock_page, "dashboard", tile_height=2000, load_wait=30 + ) + + def test_load_wait_default_is_sixty_seconds(self): + """load_wait defaults to 60 to match SCREENSHOT_LOAD_WAIT config default.""" + import inspect + + from superset.utils.screenshot_utils import take_tiled_screenshot + + sig = inspect.signature(take_tiled_screenshot) + assert sig.parameters["load_wait"].default == 60 diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index ad55f4b68e60..a4564e9aabfb 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -640,6 +640,123 @@ def test_find_unexpected_errors_processes_alerts( 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( + """() => { + const els = document.querySelectorAll('.loading'); + for (const el of els) { + const r = el.getBoundingClientRect(); + if (r.top < window.innerHeight && r.bottom > 0) { + return false; + } + } + return true; + }""", + 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")