feat(cli-exec): add --fail-on-error so CI fails when Percy can't start - #2371
Open
aryanku-dev wants to merge 1 commit into
Open
feat(cli-exec): add --fail-on-error so CI fails when Percy can't start#2371aryanku-dev wants to merge 1 commit into
aryanku-dev wants to merge 1 commit into
Conversation
`percy exec` derives its exit code solely from the wrapped command, so a Percy startup failure (invalid/missing token, browser launch failure) is logged as "Skipping visual tests" and the pipeline still reports success. The visual tests silently never run. Adds an opt-in `--fail-on-error` flag (also `PERCY_FAIL_ON_ERROR=true`) that exits 1 when Percy failed to start. Default behavior is unchanged. The check keys off the exception thrown by `percy.yield.start()` rather than scraping error-level logs, because some error-level entries are non-fatal diagnostics (e.g. "Unable to analyze error logs" from the error-analysis side channel) that would otherwise fail healthy runs. The wrapped command's own non-zero status still takes priority, as it is the more specific signal. Refs #1181, PER-10368 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #1181. Related to PER-10368 but NOT the fix for it — see "Provenance" at the bottom.
Problem
percy execderives its exit code solely from the wrapped command. When Percy itself fails to start — invalid/missing token, browser launch failure — the error is logged and then swallowed, and the pipeline reports success:The visual tests silently never ran. This is the long-standing complaint in #1181 (open since 2023).
Root cause
packages/cli-exec/src/exec.js:catcharoundpercy.yield.start()logsSkipping visual testsand continues — the error is discarded.if (status) exit(status, error, false).So Percy's own error state can never influence the exit code.
PERCY_EXIT_WITH_ZERO_ON_ERRORis the opposite knob (it forces 0); there was no way to opt into failing.Fix
Adds an opt-in
--fail-on-errorflag (orPERCY_FAIL_ON_ERROR=true) that exits1when Percy failed to start.Default behavior is unchanged — without the flag, the wrapped command's status is still the only input to the exit code, so this is not a breaking change for existing pipelines.
Two deliberate design points:
logger.query(l => l.level === 'error')looked tempting but would false-positive:percy.js:869logsUnable to analyze error logsat error level from the error-analysis side channel, and that fires on healthy runs. Verified: a real successful build with--fail-on-errorstill exits 0.Scope
This covers startup failures (Percy never started → zero visual coverage). It deliberately does not cover:
percy build:wait.Could not take DOM snapshot). These still produce afinishedbuild with fewer snapshots and exit 0. That is a real remaining gap, called out here so it is not mistaken for covered.Testing
yarn workspace @percy/cli-exec test— 78/78 pass, including 5 new specs:PERCY_FAIL_ON_ERRORis set without the flagAlso verified end-to-end against the built CLI:
--fail-on-errorPERCY_FAIL_ON_ERROR=true--fail-on-error--fail-on-errorProvenance / honest scoping
This started as an investigation into PER-10368 ("Percy errors not failing pipeline"). The ticket's attached screenshot later revealed that customer's actual failure was a
@percy/dombug dropping individual snapshots — fixed separately in #2372 — not a startup failure. So this PR does not resolve PER-10368.It stands on its own as a fix for #1181, which is a distinct and independently-reported problem. Reviewers should judge it on that basis alone.
Note for reviewers
#1181 was previously answered with "it's by design; we'll evaluate the need for this in the future." This PR keeps that default intact and only adds an opt-in, but the flag name / whether this should eventually become the default is a product call worth confirming.
🤖 Generated with Claude Code