mirror of
https://github.com/apache/superset.git
synced 2026-05-29 20:29:34 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
committed by
Amin Ghadersohi
parent
cb53745d43
commit
4e6ab1ceec
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user