Repository navigation
PPLT-6233: Export PERCY_CLI_API and CYPRESS_PERCY_SERVER_ADDRESS from percy exec - #2466
RaghavsBrowserStack wants to merge 1 commit into
Conversation
… percy exec The Python and .NET SDKs read PERCY_CLI_API and the Cypress browser only sees CYPRESS_-prefixed vars, so with a non-default --port they fell back to 5338 and silently disabled snapshots. Export both with the server address, keeping any value the user already set. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Note Some coding-guideline sources were skipped; the reason is listed next to each one. Other guidelines were applied as usual. On a public repository, sources from your configuration are listed without their names. Skipped guideline sources: 1
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-6233 and clearly describes the exported environment variables added by the change. |
| 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. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![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) — 3 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| if (address) { | ||
| // the Python and .NET SDKs read PERCY_CLI_API, and Cypress only forwards CYPRESS_-prefixed | ||
| // vars to the browser; without these a non-default --port leaves those SDKs on 5338 | ||
| env.PERCY_CLI_API ||= address; |
There was a problem hiding this comment.
[Medium] Inherited or stale value silently wins over the live --port address
||= keeps any PERCY_CLI_API or CYPRESS_PERCY_SERVER_ADDRESS already in the environment, for example from an outer percy exec or a CI config hard-coding :5338. PERCY_SERVER_ADDRESS is always overwritten, so with --port 4567 the vars disagree and the Python, .NET and Cypress SDKs silently hit the wrong server. That is the bug this PR fixes.
Suggestion: Overwrite unconditionally like PERCY_SERVER_ADDRESS, or keep ||= and log.debug/log.warn when the kept value differs from address. Either way, document the precedence.
Reviewer: stack:percy-cli-code-review, stack-percy-cli-code-reviewer
| // provide SDKs with useful env vars | ||
| env.PERCY_SERVER_ADDRESS = percy?.address(); | ||
| let address = percy?.address(); | ||
| env.PERCY_SERVER_ADDRESS = address; |
There was a problem hiding this comment.
[Low] PERCY_SERVER_ADDRESS becomes the string "undefined" when Percy is disabled (pre-existing)
env is process.env (cli-command/src/command.js:219), so assigning undefined stores "undefined". The new if (address) guard protects only the two new vars.
Suggestion: Move this assignment inside the guard. A follow-up PR is fine.
Reviewer: stack:percy-cli-code-review, stack-percy-cli-code-reviewer
| ].join('')])).toBeResolved(); | ||
| }); | ||
|
|
||
| it('does not export the alternate server address env vars when percy is disabled', async () => { |
There was a problem hiding this comment.
[Low] Disabled-path spec doesn't assert that Percy was actually disabled
This spec only checks that the vars are absent. There is also no spec for an inherited or stale value.
Suggestion: Assert [percy] Percy is disabled in logger.stderr, and add a nested or stale spec once the ||= precedence is decided.
Reviewer: stack:percy-cli-code-review, stack-percy-cli-code-reviewer
Claude Code PR ReviewPR: #2466 • Head: 6539aa2 • Reviewers: stack:percy-cli-code-review, stack-percy-cli-code-reviewer, stack-percy-docs-reviewer Summary
Review Table
Findings
Docs coverage
Verdict: PASS — the change is correct and tested, with no High or Critical findings. Settle the |
1 participant
Jira: https://browserstack.atlassian.net/browse/PPLT-6233 (epic PPLT-6210)
Problem
percy exec --port <N>exports onlyPERCY_SERVER_ADDRESSto the child command. The Python and .NET SDKs (Selenium and Playwright) readPERCY_CLI_APIinstead, and the Cypress browser only seesCYPRESS_*variables. With a non-default port these SDKs fall back tolocalhost:5338, logPercy is not running, disabling snapshots, and the command exits 0 with an empty build.Pre-existing, not caused by the framework upgrades. Cypress 16 makes it more visible because it removed
Cypress.env().Fix
When Percy is running,
percy execnow also exports:PERCY_CLI_API(Python and .NET SDKs)CYPRESS_PERCY_SERVER_ADDRESS(Cypress ≤15 viaCypress.env(); on 16CYPRESS_*vars still populatecy.env())Both get the same address as
PERCY_SERVER_ADDRESS. A value the user already set is kept. Neither is exported when Percy is disabled, so the SDKs keep their own defaults.Cypress 16
expose: not exported. The Cypress migration guide,Cypress.exposeAPI and environment variables guide document only two ways to setexposevalues: the config file and the--expose/-xCLI flag. They sayCYPRESS_*OS variables "populateenvonly" and "never reachCypress.expose()". There is noCYPRESS_EXPOSE_*or similar env-var route, so on Cypress 16@percy/cypressmust read the address viacy.env()or a task, notCypress.expose().Testing
yarn testinpackages/cli-exechas 2 failures on this branch:handles terminating the child process when interruptedandexec:start starts a long-running percy process. These are timing/signal specs. Master also fails locally (3 failures, including the interrupt spec), so they are not caused by this change. Leaving it to CI.percy exec --port 5501,PERCY_CLI_APInot set:Percy is not running, disabling snapshots, empty build: https://percy.io/9560f98d/web/testing-framework-upgrade-c905f5b0/builds/54713182🤖 Generated with Claude Code
Summary by CodeRabbit