eschutho opened a new pull request, #42273:
URL: https://github.com/apache/superset/pull/42273
### Decisions made that were not in the instructions
- **Exception type: `PlaywrightTimeout`.** No dedicated exception class
exists for "capture failed, no more waiting to do" in this method.
`PlaywrightTimeout` is already used three times in
`WebDriverPlaywright.get_screenshot()` (element-wait, chart-container-wait, and
both spinner-wait sites) as the established "log a WARNING, then raise so the
report fails" pattern, and it's the one exception type in the enclosing `try`
block that isn't swallowed — the sibling `except PlaywrightError:` handler at
the bottom of the method logs via `logger.exception` and does *not* re-raise,
so raising a `PlaywrightError` here would silently swallow the failure instead
of propagating it. Reusing `PlaywrightTimeout` keeps this fix consistent with
the existing local convention rather than introducing a new exception type.
- **Test-level propagation check.** Verified (not part of the fix) that
`PlaywrightTimeout` raised here already propagates cleanly through
`BaseScreenshot.get_screenshot()` (`superset/utils/screenshots.py`, no
swallowing `except`) into `BaseReportState._get_screenshots()`
(`superset/commands/report/execute.py`), which wraps any `Exception` as
`ReportScheduleScreenshotFailedError`, driving the report to
`ReportState.ERROR`. The sibling per-tile-readiness-timeout fix
(`take_tiled_screenshot()` in `screenshot_utils.py`) only tests up to the point
the exception leaves the function under test, not the full chain to
`ReportScheduleScreenshotFailedError` — this PR's tests follow that same
convention and assert the raise happens at the
`WebDriverPlaywright.get_screenshot()` boundary.
### SUMMARY
`WebDriverPlaywright.get_screenshot()` fell back to `_get_screenshot()` — a
raw `page.screenshot()`/`element.screenshot()` call with **zero**
wait/readiness logic — whenever the tiled capture (`take_tiled_screenshot()`)
returned a falsy result (`None` or `b""`). Unlike the tiled path itself or the
non-tiled/standard branch in this same method, this fallback never waited for
spinners to clear or charts to render, so it could silently deliver a
screenshot of loading spinners or a blank dashboard in a report/thumbnail.
This PR removes that fallback. When tiled capture fails for any reason, the
method now logs a `WARNING` (a capture/rendering issue, not a system fault —
matching the level already used for the other capture-failure paths in this
method) and raises `PlaywrightTimeout`, which propagates to a clean report
failure the same way the other timeout paths in this method already do.
### BEFORE/AFTER
**Before:**
```python
if not img:
logger.warning("Tiled screenshot failed, falling back to standard
screenshot")
img = WebDriverPlaywright._get_screenshot(page, element, element_name)
```
**After:**
```python
if not img:
logger.warning(
"Tiled screenshot failed for url %s and no safe fallback "
"exists; failing the report",
url,
)
raise PlaywrightTimeout(f"Tiled screenshot failed for url {url}")
```
### TESTING INSTRUCTIONS
-
`tests/unit_tests/utils/webdriver_test.py::TestWebDriverPlaywrightFallback::test_tiled_screenshot_failure_raises_without_fallback`
— `take_tiled_screenshot()` returns `None` → asserts `PlaywrightTimeout` is
raised, `page.screenshot`/`element.screenshot` are never called, and the
`WARNING` log fires with the expected message.
-
`tests/unit_tests/utils/webdriver_test.py::TestWebDriverPlaywrightAnimationWaitOrder::test_tiled_empty_bytes_raises_without_fallback`
— same assertions for the `b""` (falsy-but-not-`None`) case.
- Ran `pytest tests/unit_tests/utils/webdriver_test.py
tests/unit_tests/utils/test_screenshot_utils.py` — 59 passed, 1 pre-existing
failure unrelated to this change
(`test_get_screenshot_handles_playwright_timeout` fails on `master` too, due to
a `playwright` package version mismatch with the test's no-arg
`PlaywrightTimeout()` construction).
- Ran `pre-commit run --files superset/utils/webdriver.py
tests/unit_tests/utils/webdriver_test.py` — all hooks pass (ruff, ruff-format,
mypy, pylint).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
**Not changed (out of scope for this PR):**
- `take_tiled_screenshot()` itself in `screenshot_utils.py`, and the
tiling-routing decision (chart-count/height threshold) in `webdriver.py`.
- The non-tiled/standard screenshot branch in
`WebDriverPlaywright.get_screenshot()`.
- The Selenium webdriver path (`WebDriverSelenium`).
- Pixel-level/image-content blank-screenshot validation — not built as a
backstop here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
