You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/07/18 00:42:28 UTC
(superset) 01/01: fix(reports): downgrade chart-container timeout log level and fix tiling veto on unknown height
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch fix-chart-container-locate-wait-timeout
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 0f752c9c8c44b2504ef7c902d2b465b216c4e144
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Fri Jul 17 20:11:40 2026 +0000
fix(reports): downgrade chart-container timeout log level and fix tiling veto on unknown height
The chart-container locate-wait timeout was logged via logger.exception
(ERROR + traceback), inconsistent with the WARNING level already used for
the sibling spinner-wait timeouts in the same method. Bring it in line and
add rendered/total chart-container progress to the message; the timeout
still re-raises, so report generation already failed loudly here.
Also fix a real bug in the tiled-screenshot heuristic: when the DOM query
for a dashboard's height comes back empty (returns 0), the code treated
that as "small enough for a single tile", vetoing tiling even when
chart_count alone showed the dashboard was large. That could let a large
dashboard skip the scroll-driven tiling pass and ship with below-the-fold
charts that never rendered. An unknown height no longer overrides a
chart_count-based large-dashboard signal, and the warning is now only
logged when it's actually consequential (downgraded to debug otherwise).
Co-Authored-By: Claude <no...@anthropic.com>
---
superset/utils/webdriver.py | 50 +++++++--
tests/unit_tests/utils/webdriver_test.py | 176 ++++++++++++++++++++++++++++++-
2 files changed, 214 insertions(+), 12 deletions(-)
diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py
index 505223fd2ae..01c585d6728 100644
--- a/superset/utils/webdriver.py
+++ b/superset/utils/webdriver.py
@@ -327,16 +327,27 @@ class WebDriverPlaywright(WebDriverProxy):
logger.exception("Timed out requesting url %s", url)
raise
+ slice_container_elems: list[Locator] = []
+ rendered_chart_count = 0
try:
# chart containers didn't render
logger.debug("Wait for chart containers to draw at url: %s", url)
slice_container_locator = page.locator(".chart-container")
- for slice_container_elem in slice_container_locator.all():
+ slice_container_elems = slice_container_locator.all()
+ for slice_container_elem in slice_container_elems:
slice_container_elem.wait_for()
+ rendered_chart_count += 1
except PlaywrightTimeout:
- logger.exception(
- "Timed out waiting for chart containers to draw at url %s",
+ # Customer-side chart loading is often just slow, not a
+ # Superset bug, so this is a WARNING (matching the other
+ # locate-wait timeouts below) rather than an ERROR -- but
+ # it still fails the screenshot; see the `raise` below.
+ logger.warning(
+ "Timed out waiting for chart containers to draw at url %s "
+ "(%s of %s chart containers rendered before the timeout)",
url,
+ rendered_chart_count,
+ len(slice_container_elems),
)
raise
selenium_animation_wait = app.config[
@@ -374,19 +385,38 @@ class WebDriverPlaywright(WebDriverProxy):
"SCREENSHOT_TILED_VIEWPORT_HEIGHT", viewport_height
)
- if dashboard_height == 0:
- logger.warning(
+ # A height of 0 means the DOM query above found no matching
+ # element (or it hadn't laid out yet), not that the
+ # dashboard is actually empty. Treat it as "unknown" rather
+ # than "fits in a single tile": chart_count alone already
+ # tells us whether this looks like a large dashboard, and
+ # that signal must not be silently vetoed just because we
+ # couldn't measure height, or a large dashboard could skip
+ # tiling and ship with unrendered below-the-fold charts.
+ height_unknown = dashboard_height == 0
+ likely_large_dashboard = (
+ chart_count >= chart_threshold
+ or dashboard_height > height_threshold
+ )
+ if height_unknown:
+ log_fn = (
+ logger.warning if likely_large_dashboard else logger.debug
+ )
+ log_fn(
"Could not determine dashboard height for element %s "
- "at url %s; falling back to standard screenshot behavior",
+ "at url %s (%s chart containers found); %s",
element_name,
url,
+ chart_count,
+ "attempting tiled screenshot anyway"
+ if likely_large_dashboard
+ else "falling back to standard screenshot behavior",
)
# Use tiled screenshots for large dashboards
- use_tiled = (
- chart_count >= chart_threshold
- or dashboard_height > height_threshold
- ) and dashboard_height > tile_height
+ use_tiled = likely_large_dashboard and (
+ height_unknown or dashboard_height > tile_height
+ )
if use_tiled:
logger.info(
diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py
index 02b9ffe5d4a..ddb505c7ba6 100644
--- a/tests/unit_tests/utils/webdriver_test.py
+++ b/tests/unit_tests/utils/webdriver_test.py
@@ -899,12 +899,184 @@ class TestWebDriverPlaywrightErrorHandling:
)
assert result == b"fake_screenshot"
- mock_logger.warning.assert_any_call(
- "Could not determine dashboard height for element %s at url %s; "
+ # chart_count (1) is well below the tiling threshold (20), so this is
+ # the benign/expected case and must not be logged as a WARNING.
+ mock_logger.debug.assert_any_call(
+ "Could not determine dashboard height for element %s "
+ "at url %s (%s chart containers found); %s",
+ "dashboard",
+ "http://example.com",
+ 1,
"falling back to standard screenshot behavior",
+ )
+ assert not any(
+ call.args and "Could not determine dashboard height" in call.args[0]
+ for call in mock_logger.warning.call_args_list
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.logger")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ def test_unknown_height_does_not_veto_tiling_for_large_dashboard(
+ self, mock_take_tiled, mock_logger, mock_browser_manager
+ ):
+ """
+ A large dashboard (by chart_count) whose height can't be measured
+ must still attempt tiling instead of being silently downgraded to a
+ standard screenshot, since below-the-fold charts may not have
+ rendered without the scroll-driven tiling pass.
+ """
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+ mock_chart_container = MagicMock()
+
+ mock_browser_manager.get_browser.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+
+ def locator_side_effect(selector):
+ if selector == ".chart-container":
+ locator = MagicMock()
+ locator.all.return_value = [mock_chart_container]
+ return locator
+ return mock_element
+
+ mock_page.locator.side_effect = locator_side_effect
+ mock_element.wait_for.return_value = None
+ mock_chart_container.wait_for.return_value = None
+ mock_page.wait_for_timeout.return_value = None
+ mock_take_tiled.return_value = b"tiled_screenshot"
+
+ def evaluate_side_effect(script):
+ if script == 'document.querySelectorAll(".chart-container").length':
+ return 25 # chart_count >= threshold
+ if "const target = document.querySelector" in script:
+ return 0 # height could not be determined
+ return None
+
+ mock_page.evaluate.side_effect = evaluate_side_effect
+
+ with patch("superset.utils.webdriver.app") as mock_app:
+ mock_app.config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 5,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 1,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ with patch.object(WebDriverPlaywright, "auth") as mock_auth:
+ mock_auth.return_value = mock_context
+
+ driver = WebDriverPlaywright("chrome")
+ result = driver.get_screenshot(
+ "http://example.com", "dashboard", mock_user
+ )
+
+ assert result == b"tiled_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ "Could not determine dashboard height for element %s "
+ "at url %s (%s chart containers found); %s",
"dashboard",
"http://example.com",
+ 25,
+ "attempting tiled screenshot anyway",
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.logger")
+ def test_chart_container_timeout_logs_warning_with_progress_and_raises(
+ self, mock_logger, mock_browser_manager
+ ):
+ """
+ Timing out while waiting for `.chart-container` elements to draw must
+ be logged as a WARNING (matching the other locate-wait timeouts in
+ this method, and the customer-side-slowness convention established
+ for these Playwright timeouts) with rendered/total progress, and must
+ still fail the screenshot by re-raising.
+ """
+ from superset.utils.webdriver import PlaywrightTimeout
+
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_browser_manager.get_browser.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+
+ timeout = PlaywrightTimeout()
+ rendered_ok = MagicMock()
+ rendered_ok.wait_for.return_value = None
+ never_renders = MagicMock()
+ never_renders.wait_for.side_effect = timeout
+
+ def locator_side_effect(selector):
+ if selector == ".chart-container":
+ locator = MagicMock()
+ locator.all.return_value = [rendered_ok, never_renders]
+ return locator
+ return mock_element
+
+ mock_page.locator.side_effect = locator_side_effect
+ mock_element.wait_for.return_value = None
+
+ with patch("superset.utils.webdriver.app") as mock_app:
+ mock_app.config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 5,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 1,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_TILED_ENABLED": False,
+ }
+
+ with patch.object(WebDriverPlaywright, "auth") as mock_auth:
+ mock_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 chart containers to draw at url %s "
+ "(%s of %s chart containers rendered before the timeout)",
+ "http://example.com",
+ 1,
+ 2,
)
+ mock_logger.exception.assert_not_called()
@patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
@patch("superset.utils.webdriver._browser_manager")