Repository navigation
PPLT-6241: honor PERCY_SERVER_ADDRESS and harden CDP detection for Selenium 4.28+ - #277
RaghavsBrowserStack wants to merge 2 commits into
Conversation
…lenium 4.28+
- Fall back to PERCY_SERVER_ADDRESS (exported by `percy exec --port`) when
PERCY_CLI_API is unset.
- Compare browserName case-insensitively for CDP decisions.
- Remember drivers whose CDP command is rejected ("Unrecognised command") and
skip CDP for them afterwards instead of failing every resize.
- Drop the <4.28 Selenium cap in the dev/CI requirements.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title includes the valid Jira ID PPLT-6241 and clearly describes the address fallback and CDP detection changes. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Lowercasing browserName made drivers reporting "Chrome" switch from a real window resize to CDP emulation, which earlier releases never did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
RaghavsBrowserStack
left a comment
RaghavsBrowserStack
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.
| # across 3.8-3.10; revisit once that wedge is root-caused so CI can track | ||
| # current Selenium. | ||
| selenium>=4,<4.28 | ||
| # CI/dev test env only — setup.py keeps `selenium>=3` for end users. Selenium |
There was a problem hiding this comment.
[Medium] Comment misstates Selenium's Python floor: the 3.9 job now gets 4.36.0
Selenium 4.28.0–4.36.0 declare requires_python >=3.9; only 4.37.0+ needs 3.10. So the 3.9 CI job resolves 4.36.0, the version the removed comment said wedged geckodriver on the responsive test.
Suggestion: Correct the comment, and confirm Test (3.9) and Test (3.10) pass without hanging. If 3.9 wedges, use selenium>=4,<4.28; python_version<"3.10" plus selenium>=4; python_version>="3.10".
Reviewer: stack:percy-web-sdks-code-review
| # raise "Unrecognised command" on every call. That never recovers mid-session, | ||
| # so stop sending CDP to the driver instead of failing on each resize/snapshot. | ||
| _CDP_REJECTED_DRIVERS = WeakSet() | ||
| _CDP_REJECTION_MARKERS = ('unrecognised command', 'unknown command', 'unknown method') |
There was a problem hiding this comment.
[Low] Rejection markers are broader than a driver-level rejection
'unknown method' is generic. A single unsupported CDP method would disable CDP for the whole session, for both resize and shadow-DOM capture, because the memo is shared. Separately, under python -O Selenium's assert is stripped, so the rejection surfaces as a marker-less TypeError and the driver is never remembered (degraded, not broken).
Suggestion: Require executecdpcommand in the message, or drop 'unknown method'.
Reviewer: stack-code-reviewer, stack:percy-web-sdks-code-review
Claude Code PR ReviewPR: #277 • Head: 5617a95 • Reviewers: stack:percy-web-sdks-code-review, stack-code-reviewer, stack-percy-docs-reviewer SummaryThe CLI address now falls back to Review Table
Findings
Checked and not a defect: one reviewer asked whether the inner Cross-SDK follow-up (non-gating): check the other non-JS ports (selenium-java/ruby/.NET, playwright-python/java/.NET, capybara). Any that read only Verdict: FAIL — the only High is the unremediated knowledge-base gap. The code change is sound. Fix the KB entries, then confirm the Python 3.9 and 3.10 CI jobs pass. |
Jira: https://browserstack.atlassian.net/browse/PPLT-6241
Problem
PERCY_CLI_API, butpercy exec --port NexportsPERCY_SERVER_ADDRESS. On a non-default port the SDK disables itself, and the script exits 0 with an empty build.execute_cdp_cmdnow exists on everyRemotedriver, but only aChromiumRemoteConnection(picked when the requestedbrowserNameis exactlychrome) understands it. On Automate withbrowserName: "Chrome", every CDP call raisesUnrecognised command executeCdpCommandand falls back. It falls back cleanly but noisily, once per resize.selenium<4.28, so 4.50 was untested.Fix
PERCY_CLI_API→PERCY_SERVER_ADDRESS→http://localhost:5338._is_chrome()comparesbrowserNamecase-insensitively inexpose_closed_shadow_rootsandchange_window_dimension_and_wait.WeakSetand CDP is skipped for it afterwards. Other CDP errors behave as before.requirements.txt: the<4.28cap is dropped (selenium>=4); pip still resolves 4.27.x on Python 3.8/3.9.Testing
percy/is 100% and pylint scores 10/10. There are 11 new unit tests intests/test_snapshot_units.py.percy exec --port 5506,PERCY_CLI_APIunset, example-percy-selenium-python):browserName: "Chrome",percy_snapshotwithresponsive_snapshot_capture, widths 375/1280):PERCY_CLI_APIworkaround): 54714705: 4Unrecognised commanderrors (shadow DOM plus a failed CDP attempt for each resize)set_window_size; snapshot taken at both widthsSummary by CodeRabbit
Backwards compatibility
PERCY_CLI_APIstill takes precedence;PERCY_SERVER_ADDRESSis only a fallback.browserName == 'chrome'check from earlier releases, so drivers reporting e.g.Chromestill get a plain window resize.setup.pykeepsselenium>=3; only the CI/dev requirements change.🤖 Generated with Claude Code