diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 5fd7d94418ab..75eeb1d2497b 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -563,18 +563,23 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n log_context=log_context, ) if not img: + # _get_screenshot() has no wait/readiness logic at + # all, so falling back to it here would risk + # silently delivering a screenshot of spinners or + # a blank dashboard. Fail the capture loudly + # (report error, thumbnail cache ERROR) instead of + # guessing at a "safer" fallback. logger.warning( - ( - "Tiled screenshot failed, " - "falling back to standard screenshot" - ) + "Tiled screenshot failed for url %s and no " + "safe fallback exists; failing the capture", + url, ) - img = WebDriverPlaywright._get_screenshot( - page, element, element_name + raise PlaywrightTimeout( + f"Tiled screenshot failed for url {url}" ) logger.debug( "Tiled screenshot result: %d bytes for url: %s", - len(img) if img else 0, + len(img), url, ) else: diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index a7dea405b8e5..c6fa18e9dc9b 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -930,10 +930,13 @@ def evaluate_side_effect(script): @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.logger") @patch("superset.utils.webdriver.take_tiled_screenshot") - def test_tiled_screenshot_failure_falls_back_to_standard_screenshot( + def test_tiled_screenshot_failure_raises_without_fallback( self, mock_take_tiled, mock_logger, mock_browser_manager ) -> None: - """When take_tiled_screenshot returns None, fall back to standard screenshot.""" + """When take_tiled_screenshot returns None, fail loudly instead of + falling back to an unguarded standard screenshot.""" + from superset.utils.webdriver import PlaywrightTimeout + mock_user = MagicMock() mock_user.username = "test_user" @@ -947,7 +950,8 @@ def test_tiled_screenshot_failure_falls_back_to_standard_screenshot( mock_context.new_page.return_value = mock_page mock_page.locator.return_value = mock_element mock_element.wait_for.return_value = None - # page.screenshot is used by _get_screenshot for the "standalone" element + # page.screenshot is used by _get_screenshot for the "standalone" element; + # it must never be reached by the failure path under test. mock_page.screenshot.return_value = b"fallback_screenshot" def evaluate_side_effect(script): @@ -983,14 +987,20 @@ def evaluate_side_effect(script): mock_auth.return_value = mock_context driver = WebDriverPlaywright("chrome") - result = driver.get_screenshot( - "http://example.com", "standalone", mock_user - ) + # match= keeps this assertion meaningful even when playwright + # is not installed and PlaywrightTimeout aliases bare Exception. + with pytest.raises( + PlaywrightTimeout, match="Tiled screenshot failed for url" + ): + driver.get_screenshot("http://example.com", "standalone", mock_user) - assert result == b"fallback_screenshot" mock_take_tiled.assert_called_once() + mock_page.screenshot.assert_not_called() + mock_element.screenshot.assert_not_called() mock_logger.warning.assert_any_call( - ("Tiled screenshot failed, falling back to standard screenshot"), + "Tiled screenshot failed for url %s and no safe fallback " + "exists; failing the capture", + "http://example.com", ) @@ -1514,10 +1524,13 @@ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait( @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.take_tiled_screenshot") @patch("superset.utils.webdriver.app") - def test_tiled_fallback_triggered_on_empty_bytes( + def test_tiled_empty_bytes_raises_without_fallback( self, mock_app, mock_take_tiled, mock_browser_manager ): - """Tiled fallback fires when take_tiled_screenshot returns b"" (not None).""" + """Tiled failure raises when take_tiled_screenshot returns b"" (not None), + instead of silently falling through to an unguarded raw capture.""" + from superset.utils.webdriver import PlaywrightTimeout + mock_user = MagicMock() mock_user.username = "test_user" mock_app.config = { @@ -1532,20 +1545,24 @@ def test_tiled_fallback_triggered_on_empty_bytes( mock_page.evaluate.side_effect = [25, 6000] # Empty bytes — falsy but not None; was silently passed through before the fix mock_take_tiled.return_value = b"" - # _get_screenshot("standalone") calls page.screenshot(full_page=True); - # configure that return value so we can assert the fallback was reached + # _get_screenshot("standalone") calls page.screenshot(full_page=True); it + # must never be reached by the failure path under test. mock_page.screenshot.return_value = b"fallback" with patch.object(WebDriverPlaywright, "auth", return_value=mock_context): - result = WebDriverPlaywright("chrome").get_screenshot( - "http://example.com", "standalone", mock_user - ) + # match= keeps this assertion meaningful even when playwright + # is not installed and PlaywrightTimeout aliases bare Exception. + with pytest.raises( + PlaywrightTimeout, match="Tiled screenshot failed for url" + ): + WebDriverPlaywright("chrome").get_screenshot( + "http://example.com", "standalone", mock_user + ) - assert result == b"fallback" # Tiled path was taken (take_tiled_screenshot was called) mock_take_tiled.assert_called_once() - # Standard screenshot was called as fallback (full_page=True for "standalone") - mock_page.screenshot.assert_called_with(full_page=True) + # Standard screenshot must never be called as a fallback + mock_page.screenshot.assert_not_called() @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) @patch("superset.utils.webdriver._browser_manager")