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/05/05 16:27:16 UTC
(superset) 01/01: fix(reports): narrow spinner checks to viewport and tighten exception handling
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch playwright-report-spinners
in repository https://gitbox.apache.org/repos/asf/superset.git
commit a0b2b1811b38901b627f90826755b7c96c108e90
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Tue May 5 16:26:51 2026 +0000
fix(reports): narrow spinner checks to viewport and tighten exception handling
- Replace global '.loading' count check in webdriver.py with a
getBoundingClientRect viewport-visibility check to avoid deadlock
when DashboardVirtualization renders off-screen placeholder spinners
- Narrow except clause in screenshot_utils.py from bare Exception to
PlaywrightTimeout so non-timeout errors (e.g. browser crash) propagate
- Fix load_wait default from 30 s to 60 s to match SCREENSHOT_LOAD_WAIT
config default
- Add tests covering per-tile spinner wait, timeout warning, non-timeout
propagation, and load_wait default value
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/utils/screenshot_utils.py | 34 ++++++++++++++-
superset/utils/webdriver.py | 18 +++++++-
tests/unit_tests/utils/test_screenshot_utils.py | 58 +++++++++++++++++++++++++
tests/unit_tests/utils/webdriver_test.py | 11 ++++-
4 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py
index dfd2e49f54f..8deada447e7 100644
--- a/superset/utils/screenshot_utils.py
+++ b/superset/utils/screenshot_utils.py
@@ -28,6 +28,11 @@ logger = logging.getLogger(__name__)
# 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 3d61411b737..29a4afc10f9 100644
--- a/superset/utils/webdriver.py
+++ b/superset/utils/webdriver.py
@@ -301,7 +301,16 @@ class WebDriverPlaywright(WebDriverProxy):
"Wait for loading element of charts to be gone at url: %s", url
)
page.wait_for_function(
- "() => document.querySelectorAll('.loading').length === 0",
+ """() => {
+ 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:
@@ -368,7 +377,12 @@ class WebDriverPlaywright(WebDriverProxy):
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 f1b970bf83c..8cdd7106697 100644
--- a/tests/unit_tests/utils/test_screenshot_utils.py
+++ b/tests/unit_tests/utils/test_screenshot_utils.py
@@ -320,3 +320,61 @@ class TestTakeTiledScreenshot:
# 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 cf7447eef1d..a4564e9aabf 100644
--- a/tests/unit_tests/utils/webdriver_test.py
+++ b/tests/unit_tests/utils/webdriver_test.py
@@ -684,7 +684,16 @@ class TestWebDriverPlaywrightErrorHandling:
driver.get_screenshot("http://example.com", "test-element", mock_user)
mock_page.wait_for_function.assert_called_once_with(
- "() => document.querySelectorAll('.loading').length === 0",
+ """() => {
+ 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