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/16 22:20:41 UTC
(superset) branch fix-tile-readiness-check created (now d32c2b891f2)
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a change to branch fix-tile-readiness-check
in repository https://gitbox.apache.org/repos/asf/superset.git
at d32c2b891f2 fix(reports): positive per-tile chart readiness check for tiled screenshots
This branch includes the following new commits:
new d32c2b891f2 fix(reports): positive per-tile chart readiness check for tiled screenshots
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(reports): positive per-tile chart readiness check for tiled screenshots
Posted by el...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch fix-tile-readiness-check
in repository https://gitbox.apache.org/repos/asf/superset.git
commit d32c2b891f290e359dc80e184361b89b5cf36d74
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Thu Jul 16 22:20:19 2026 +0000
fix(reports): positive per-tile chart readiness check for tiled screenshots
`take_tiled_screenshot()` waited for the *absence* of `.loading` elements
visible in the viewport before capturing each tile. With
DashboardVirtualization on (default), a chart holder that has just scrolled
into view but hasn't fired its IntersectionObserver callback yet mounts
neither a spinner nor a chart, so the predicate passed vacuously and the
tile was captured blank. On top of that, a per-tile timeout was caught,
logged as a warning, and the tile was captured anyway -- delivering a
spinner screenshot to report recipients instead of failing the report.
Replace the absence-of-`.loading` predicate with a positive readiness
check: every chart holder (`data-test="dashboard-component-chart-holder"`)
intersecting the viewport must show a terminal state (a rendered chart via
`.slice_container`, or an error/empty state via `[role="alert"]` /
`.ant-empty` / `.missing-chart-container`) before a tile is captured. A
holder with nothing mounted no longer satisfies the wait.
A per-tile timeout is now logged at ERROR with the tile index, the
load_wait, and the identities of the still-unready chart holders, and
re-raises instead of being swallowed -- the report now fails
(ReportScheduleScreenshotFailedError) instead of silently shipping a
degraded screenshot.
Co-Authored-By: Claude <no...@anthropic.com>
---
superset/utils/screenshot_utils.py | 84 +++++++++++++++++----
tests/unit_tests/utils/test_screenshot_utils.py | 98 ++++++++++++++++++++-----
2 files changed, 149 insertions(+), 33 deletions(-)
diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py
index ccd2c80aa37..36e8b7db2c2 100644
--- a/superset/utils/screenshot_utils.py
+++ b/superset/utils/screenshot_utils.py
@@ -39,6 +39,54 @@ if TYPE_CHECKING:
except ImportError:
Page = None
+# Selectors used to build a positive per-tile readiness check. A chart holder
+# is only "ready" once it shows a terminal state (a rendered chart or an
+# error/empty state) -- the mere absence of a `.loading` element is not
+# sufficient, since a chart holder that intersects the viewport but hasn't
+# mounted anything yet (e.g. its IntersectionObserver callback hasn't fired)
+# would otherwise pass vacuously.
+# See superset-frontend/src/dashboard/components/gridComponents/ChartHolder/
+# ChartHolder.tsx for `data-test="dashboard-component-chart-holder"`,
+# superset-frontend/src/components/Chart/Chart.tsx for `.slice_container`
+# (rendered chart container) and `.loading` (spinner, via the shared Loading
+# component), and superset-frontend/packages/superset-ui-core/src/components/
+# EmptyState for `.ant-empty` (e.g. "no results"/"add required control
+# values" states).
+_UNREADY_CHART_HOLDERS_JS_BODY = """
+ const holders = document.querySelectorAll(
+ '[data-test="dashboard-component-chart-holder"]'
+ );
+ const unready = [];
+ for (const holder of holders) {
+ const r = holder.getBoundingClientRect();
+ if (!(r.top < window.innerHeight && r.bottom > 0)) {
+ continue;
+ }
+ const stillLoading = holder.querySelector('.loading') !== null;
+ const isReady = holder.querySelector(
+ '.slice_container, [role="alert"], .ant-empty, .missing-chart-container'
+ ) !== null;
+ if (stillLoading || !isReady) {
+ const chartIdEl = holder.querySelector('[data-test-chart-id]');
+ unready.push(
+ chartIdEl ? chartIdEl.getAttribute('data-test-chart-id') : 'unknown'
+ );
+ }
+ }
+"""
+
+# Predicate for page.wait_for_function: true once every viewport-visible chart
+# holder has reached a terminal state.
+_TILE_READY_CHECK_JS = (
+ f"() => {{ {_UNREADY_CHART_HOLDERS_JS_BODY} return unready.length === 0; }}"
+)
+
+# Diagnostic query for page.evaluate: identities of chart holders (by
+# data-test-chart-id) still not ready, used to build the timeout log message.
+_FIND_UNREADY_CHART_HOLDERS_JS = (
+ f"() => {{ {_UNREADY_CHART_HOLDERS_JS_BODY} return unready; }}"
+)
+
def combine_screenshot_tiles(screenshot_tiles: list[bytes]) -> bytes:
"""
@@ -150,31 +198,31 @@ 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.
+ # Wait for every chart holder visible in the current viewport to reach
+ # a terminal state (rendered chart or error/empty state). Only check
+ # viewport-visible chart holders to avoid blocking on virtualization
+ # placeholders rendered for off-screen charts. A holder that hasn't
+ # mounted anything yet does not satisfy this check -- unlike checking
+ # for the absence of `.loading`, which passes vacuously in that case.
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;
- }""",
+ _TILE_READY_CHECK_JS,
timeout=load_wait * 1000,
)
except PlaywrightTimeout:
- logger.warning(
- "Timed out waiting for visible spinners to clear on tile %s/%s "
- "(load_wait=%ss)",
+ unready_chart_ids = page.evaluate(_FIND_UNREADY_CHART_HOLDERS_JS)
+ logger.error(
+ "Timed out waiting for %s chart container(s) to become ready "
+ "on tile %s/%s (load_wait=%ss); unready chart ids: %s. "
+ "Aborting tiled screenshot rather than capturing a blank or "
+ "partially-loaded tile.",
+ len(unready_chart_ids),
i + 1,
num_tiles,
load_wait,
+ unready_chart_ids,
)
+ raise
# Wait for chart animations (e.g. ECharts) to finish after spinner clears.
# The global animation wait before tiling only covers the first tile;
@@ -229,6 +277,10 @@ def take_tiled_screenshot(
return combined_screenshot
+ except PlaywrightTimeout:
+ # Let per-tile readiness timeouts propagate so the caller fails the
+ # report instead of silently falling back to a degraded screenshot.
+ raise
except Exception as e:
logger.exception("Tiled screenshot failed: %s", e)
return None
diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py
index bf264a54b8d..dd4bec6bede 100644
--- a/tests/unit_tests/utils/test_screenshot_utils.py
+++ b/tests/unit_tests/utils/test_screenshot_utils.py
@@ -321,8 +321,8 @@ class TestTakeTiledScreenshot:
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."""
+ def test_per_tile_readiness_wait_uses_viewport_check(self, mock_page):
+ """wait_for_function polls viewport-visible chart holders after each scroll."""
with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
take_tiled_screenshot(
mock_page, "dashboard", tile_height=2000, load_wait=30
@@ -336,32 +336,96 @@ class TestTakeTiledScreenshot:
js = call[0][0]
assert "getBoundingClientRect" in js
assert "window.innerHeight" in js
+ assert "dashboard-component-chart-holder" 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."""
+ def test_per_tile_readiness_timeout_raises_and_skips_capture(self, mock_page):
+ """A per-tile readiness timeout raises and does not capture that tile.
+
+ This is a product decision (fail loudly, never snapshot spinners or
+ blank charts): a per-tile timeout must abort the tiled screenshot
+ instead of warning and continuing.
+ """
from superset.utils.screenshot_utils import PlaywrightTimeout
- timeout = PlaywrightTimeout()
+ timeout = PlaywrightTimeout("Timeout waiting for chart holders")
mock_page.wait_for_function.side_effect = timeout
+ mock_page.evaluate.side_effect = [
+ {"height": 5000, "top": 100, "left": 50, "width": 800}, # dimensions
+ None, # window.scrollTo(...) for tile 1
+ ["42"], # unready chart ids, gathered after the 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
+ with pytest.raises(PlaywrightTimeout):
+ take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=30
+ )
+
+ # No tile should have been captured -- fail loudly, don't snapshot
+ # a blank or partially-loaded tile.
+ mock_page.screenshot.assert_not_called()
+
+ # Only the first tile's wait_for_function is attempted (the timeout
+ # aborts before any subsequent tile is processed).
+ assert mock_page.wait_for_function.call_count == 1
+
+ mock_logger.error.assert_called_once()
+ error_args = mock_logger.error.call_args[0]
+ assert "unready" in error_args[0].lower()
+ assert error_args[1] == 1 # count of unready chart containers
+ assert error_args[2] == 1 # tile index
+ assert error_args[3] == 3 # total tiles
+ assert error_args[4] == 30 # load_wait
+ assert error_args[5] == ["42"]
+
+ def test_chart_holder_with_nothing_mounted_blocks_wait(self, mock_page):
+ """Regression test for the vacuous-pass race (PR #39895).
+
+ A chart holder that intersects the viewport but has not yet mounted
+ a spinner or a chart (e.g. its IntersectionObserver callback hasn't
+ fired) must not satisfy the readiness predicate.
+ """
+ from superset.utils.screenshot_utils import PlaywrightTimeout
+
+ js_call_count = {"n": 0}
+
+ def fake_wait_for_function(js, timeout=None):
+ js_call_count["n"] += 1
+ # Simulate evaluating the predicate against a DOM with a chart
+ # holder in viewport that has mounted nothing at all.
+ assert "dashboard-component-chart-holder" in js
+ raise PlaywrightTimeout("Timeout waiting for chart holders")
+
+ mock_page.wait_for_function.side_effect = fake_wait_for_function
+ mock_page.evaluate.side_effect = [
+ {"height": 2000, "top": 0, "left": 0, "width": 800}, # dimensions
+ None, # window.scrollTo(...) for tile 1
+ ["7"], # the un-mounted holder's chart id
+ ]
+
+ with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
+ with pytest.raises(PlaywrightTimeout):
+ take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=5
)
- # Screenshot should still proceed (non-fatal)
+ assert js_call_count["n"] == 1
+ mock_page.screenshot.assert_not_called()
+
+ def test_all_chart_holders_ready_passes(self, mock_page):
+ """All chart holders rendered or errored -> wait passes, tile captured."""
+ with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
+ result = take_tiled_screenshot(
+ mock_page, "dashboard", tile_height=2000, load_wait=5
+ )
+
+ # mock_page.wait_for_function is a MagicMock by default and does not
+ # raise, i.e. the readiness check passes immediately for every tile.
+ assert mock_page.wait_for_function.call_count == 3
+ assert mock_page.screenshot.call_count == 3
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 "
- "(load_wait=%ss)",
- 1,
- 3,
- 30,
- )
def test_load_wait_default_is_sixty_seconds(self):
"""load_wait defaults to 60 to match SCREENSHOT_LOAD_WAIT config default."""