sfirke commented on code in PR #44244:
URL: https://github.com/apache/superset/pull/44244#discussion_r4047333315
##########
superset/utils/webdriver.py:
##########
@@ -57,10 +57,9 @@
WindowSize = tuple[int, int]
logger = logging.getLogger(__name__)
-# Installation message for missing Playwright (Cypress doesn't work with
DeckGL)
+# Installation hint appended to errors raised when Playwright or Chromium is
missing
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"
)
Review Comment:
Fixed in 453a1d2567.
Every exception from the Playwright browser launch was being 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 there.
Now only a genuinely missing browser binary gets the install hint.
Playwright reports that through the message text (`Executable doesn't exist at
…` / `Please run the following command to download new browsers`), since it
raises the same error type for a missing binary as for a crashed or
misconfigured launch. Every other launch failure raises a distinct error that
carries Playwright's own message and points at `WEBDRIVER_OPTION_ARGS`, sandbox
permissions and worker memory, and logs the launch args plus the traceback.
One consequence worth stating: `_get_screenshots` wraps whatever this raises
in `ReportScheduleScreenshotFailedError(f"Failed taking a screenshot
{str(ex)}")`, so on the non-install path the underlying Playwright text — which
can include Chromium's stderr — can now reach a report error notification where
a fixed string used to. That already happens for every other exception on this
path, so it isn't a new exposure, but it does change what those emails can
contain.
Unit tests cover both branches.
---
🤖 _Drafted by Claude Code, reviewed and approved by @sfirke._
--
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]
