eschutho commented on PR #42273: URL: https://github.com/apache/superset/pull/42273#issuecomment-5146067349
Ran an adversarial self-review on the rebase before requesting human review. Full result: no blockers — verified the raise sits only in the tiled branch (the other two `_get_screenshot` call sites are now readiness-gated by #42253/#42427, so they're no longer "unguarded" and are correctly left alone); `PlaywrightTimeout` propagates via the `except PlaywrightTimeout: raise` handler in both real and playwright-less alias modes; all production callers handle the new exception safely (reports → `ReportScheduleScreenshotFailedError` + notify, thumbnails → cache payload ERROR — no blank cached, Excel export → chart skipped-and-noted); `len(img)` after the raise is safe since `img` is guaranteed truthy; no stale tests elsewhere assert the old fallback. Two review nits applied in the amended commit: - Reworded "failing the report" → "failing the capture" in the comment/log/test — this path also serves thumbnails and Excel exports, where nothing is a report. - Added `match="Tiled screenshot failed for url"` to both `pytest.raises(PlaywrightTimeout)` asserts so they stay meaningful in playwright-less environments where `PlaywrightTimeout` aliases bare `Exception`. One pre-existing adjacent issue deliberately left out of scope: `combine_screenshot_tiles` still silently returns the first tile when stitching fails (screenshot_utils.py) — same silent-degradation shape, candidate for a follow-up. 713/713 unit tests in `tests/unit_tests/utils/` pass post-amend; ruff check/format clean. -- 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]
