Repository navigation
fix(reports): replace stale Selenium references in screenshot messages - #44244
Conversation
Selenium support was removed in apache#43028. Reword the Playwright install hint, which referred to a migration from Cypress, and update config comments, a debug log, the MCP README sample configs and a docker-compose comment that still described Selenium. The SCREENSHOT_SELENIUM_* config keys keep their names so existing configs continue to work.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44244 +/- ##
=======================================
Coverage 80.56% 80.56%
=======================================
Files 2938 2940 +2
Lines 175246 175303 +57
Branches 40680 40688 +8
=======================================
+ Hits 141184 141238 +54
- Misses 31396 31400 +4
+ Partials 2666 2665 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@villebro my last docs candidate for 7.0 cherrying |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
||
| # Time before selenium times out after trying to locate an element on the page and wait | ||
| # for that element to load for a screenshot. | ||
| # Time before the headless browser times out after trying to locate an element on the |
There was a problem hiding this comment.
SCREENSHOT_LOCATE_WAIT is only copied into _screenshot_locate_wait, which the Playwright path never reads, so this new comment makes an inert setting look like the browser's element-timeout control. Could we mark it as compatibility-only and point operators to the active Playwright wait settings, or remove it with the other dead screenshot config?
There was a problem hiding this comment.
Good catch, confirmed: nothing has read _screenshot_locate_wait since #43028. Element waits come from the report execution deadline, or SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT outside reports. Since it's dead config, I reverted my comment change here and removed the key in #44245, along with the other dead screenshot config. That PR also drops the unused attribute, its test entries and the docs example that set it, and notes the removal in UPDATING.md.
🤖 Drafted by Claude Code, co-signed by @sfirke.
|
The observation is correct. superset/config.py |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| PLAYWRIGHT_INSTALL_MESSAGE = ( | ||
| "To complete the migration from Cypress " | ||
| "and enable WebGL/DeckGL screenshot support, install Playwright with: " | ||
| "Install Playwright and Chromium with: " | ||
| "pip install playwright && playwright install chromium" | ||
| ) |
There was a problem hiding this comment.
Suggestion: Browser launch failures from invalid arguments, permissions, or crashes are reported as missing dependencies, sending operators toward an incorrect fix.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Error handling
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/webdriver.py
**Line:** 61:64
**Comment:**
*Error Handling: Browser launch failures from invalid arguments, permissions, or crashes are reported as missing dependencies, sending operators toward an incorrect fix.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Good catch, and it applied more broadly than the line it was flagged on: every failure to start the browser was being reported as a missing dependency, not just some of them. A worker running as root without --no-sandbox, a bad entry in WEBDRIVER_OPTION_ARGS, a Chromium killed for running out of memory, an unwritable temp directory — all of them told the operator to install Playwright, which was already installed.
Fixed in 069ade1 by deleting the message rather than correcting it. Playwright's own error is now passed through untouched.
That turned out to be better even for the case the message was written for. When the browser binary really is missing, Playwright says so, names the exact path where it expected to find it, and gives the command to install it. The path is the useful part — it's how you spot a wrong PLAYWRIGHT_BROWSERS_PATH or a Docker layer that dropped the browser cache. The old message had none of that, and its "pip install playwright" advice couldn't ever be right here, because this code only runs after Playwright has already imported successfully.
The install hint is still used in one place: when the Playwright import itself fails. There's no underlying error to show there, so a written message is all that's available.
One consequence worth flagging. When a report fails, this text can be emailed to the report's recipients, not just written to the worker log. So a failed screenshot can now put Playwright's wording — and sometimes Chromium's own output — in front of people who just subscribe to the report. That was already true of every other error on this path, so it isn't a new kind of exposure, but it does widen what those emails can contain. The real fix is to stop using one string for both operator diagnostics and recipient-facing mail, which is a bigger change than belongs in this PR.
🤖 Drafted by Claude Code, reviewed and approved by @sfirke.
There was a problem hiding this comment.
(I will note: this error was not introduced in this PR, this is just a chance to fix another prior outstanding issue)
Every exception from the Playwright browser launch was rewritten into the "install Playwright" message, so a container running as root without --no-sandbox, a bad WEBDRIVER_OPTION_ARGS flag, an OOM-killed Chromium or an unwritable temp dir all told the operator to reinstall a package that was already present. Playwright's own error is raised instead. It is more useful than the message it replaces, including for the missing-browser case it was written for: Playwright names the path where the binary was expected, which is how a wrong PLAYWRIGHT_BROWSERS_PATH or a dropped Docker layer gets spotted, and gives the install command. The old hint's "pip install playwright" half was wrong here by construction, since this code only runs once the import has succeeded. The install hint is kept for the failed-import branch, which has no error of its own to report. Launch args and the traceback are logged.
81d4286 to
069ade1
Compare
left a comment
There was a problem hiding this comment.
Nice cleanup, and appreciate you chasing the browser-launch error handling down to the real bug instead of just the line that got flagged. Both threads look genuinely closed, the dead _screenshot_locate_wait config split into #44245 and the launch failure now surfaces Playwright's own error with a regression test. LGTM, approving, will merge soon.
3 participants
SUMMARY
Follow-up to #43028, which removed Selenium support and made Playwright with Chromium the only screenshot backend. A few messages and comments still referred to Selenium (or Cypress) and can mislead operators:
superset/utils/webdriver.py: the hint appended to thePlaywright is required for screenshots.error said "To complete the migration from Cypress and enable WebGL/DeckGL screenshot support". It now just says how to install Playwright and Chromium.superset/config.py:SCREENSHOT_SELENIUM_HEADSTARTandSCREENSHOT_SELENIUM_ANIMATION_WAITapply to Playwright despite their names. The keys are not renamed, so existing configs keep working.superset/utils/screenshots.py: a comment and theSelenium image sizedebug log.superset/mcp_service/README.md: removeWEBDRIVER_TYPE = 'chrome'from both sample configs. That key was removed in feat(Reports&Alerts): remove Selenium support, require Playwright #43028.docker-compose.yml: the worker memory-limit comment.Apart from the wording of that one error message and one debug log line, there are no behavior changes.
Related: #44243 fixes the same references in the docs. #44245 (master only) removes the unused
INCLUDE_FIREFOXbuild arg and other dead config.Cherry-pick: this is text-only and a candidate for 7.0, so the install error shown on 7.0.x deployments without Playwright is accurate.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Error raised when Playwright or Chromium is unavailable:
Before:
After:
TESTING INSTRUCTIONS
pytest tests/unit_tests/utils/webdriver_test.py tests/unit_tests/utils/test_screenshot_cache_fix.py(the only test touchingPLAYWRIGHT_INSTALL_MESSAGEchecks that it is a string).ADDITIONAL INFORMATION
🤖 Drafted by Claude Code, co-signed by @sfirke.