test(regression): 100% E2E coverage for CLI configs (PER-8250)

[AkashBrowserStack]

Summary

Adds 100% E2E coverage for the percy snapshot CLI config surface
(PER-8250). The existing
test/regression/ harness covered only a subset of asset-discovery options;
this extends it to every non-excluded option across the percy, snapshot,
discovery, and project namespaces plus the cli-snapshot static/sitemap
and per-snapshot capture options.

Three complementary tracks (full per-option matrix in
test/regression/COVERAGE.md):

• Config validation (config-validation.test.js) — token-free
  percy snapshot --dry-run that loads fixtures setting every option and
  asserts the CLI accepts them all (no Invalid config:). This is the
  literal-100% backbone and gates every PR (including forks) without a token.
  Includes a negative self-test proving the validator actually fires.
• Visual (regression.test.js, new pages/config-*.html) — render-affecting
  options reviewed as visual diffs: scope/scopeOptions, per-snapshot percyCSS
  override, execute + additionalSnapshots, domTransformation,
  discovery.scrollToBottom, disableShadowDOM, forceShadowAsLightDOM,
  responsiveSnapshotCapture.
• Functional (functional.test.js, gated server.js routes) — discovery
  options asserted on what the test servers observed (robust to log-format
  changes): requestHeaders, authorization, cookies, userAgent, captureSrcset,
  disallowedHostnames.

CI: the regression job in test.yml now runs config validation token-free
always, plus visual + functional with PERCY_REGRESSION_TOKEN.

Scope — what "100% coverage" means here

"100%" = every CLI config option reachable via percy snapshot is exercised
end-to-end through the real CLI and validated — ~54 options across the
percy / snapshot / discovery / project namespaces plus the cli-snapshot
static/sitemap and per-snapshot capture options. Coverage is layered:

• All ~54 options → loaded & schema-validated end-to-end (Track C, token-free;
  asserts the CLI accepts each with no Invalid config:).
• Render-affecting options → additionally verified by visual diff (Track V).
• Discovery-behavior options → additionally verified by server-observed
  assertions (Track F).

Non-visual options (e.g. concurrency, retry, sync, deferUploads,
project.*, readiness.* sub-options) are covered at the config-validity
level — the CLI accepts and loads them — not with bespoke runtime-behavior
tests, by design.

Explicitly excluded: the 6 onlyAutomate snapshot options — fullPage,
freezeAnimation, freezeAnimatedImage, freezeAnimatedImageOptions,
ignoreRegions, considerRegions — are unreachable via the percy snapshot
web flow and belong to the SDK/Automate path (PER-8249), so they are not
covered here. CLI flags (--allowed-hostname, --disable-cache, …) are 1:1
wrappers over these config values and are covered at the value level.

Test-only changes; no production code touched.

Test plan

• yarn test:regression:config — passes locally, token-free (12 assertions
  incl. negative self-test; 25 + 8 snapshots enumerated, no Invalid config:)
• Gated server routes verified token-free (headers/auth/cookies/UA/srcset
  recorded; auth route 401s without creds)
• eslint test/regression/ clean
• Visual track — verified by CI regression job (Percy dashboard review)
• Functional track — verified by CI regression job (PERCY_REGRESSION_TOKEN)

Draft until CI confirms the two token-gated tracks and the visual build is
reviewed in the Percy dashboard.

---

🤖 Generated with Claude Code

[AkashBrowserStack]

Claude Code PR Review

PR: #2312 • Head: fb9ffce • Reviewers: stack:code-reviewer

Summary

Test-only PR extending the test/regression/ E2E harness with three tracks — a token-free config-validation track (percy snapshot --dry-run over fixtures exercising every non-onlyAutomate CLI config option, with an intentional-invalid negative guard), a visual track (new render-affecting snapshots + pages), and a functional discovery track (a real run whose headers/auth/cookies/UA/srcset/disallowed-host behavior is asserted against what gated local test servers observed). Adds COVERAGE.md, two yarn scripts, and wires all three into the regression job in test.yml.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	percy:secret / abc123 are local-server fixture stubs, not real creds
High	Security	Authentication/authorization checks present	N/A	Test harness; no production auth code
High	Security	Input validation and sanitization	Pass	New /gated/* + /disallowed-probe.css routes are exact-match static responders; existing path-traversal guard retained
High	Security	No IDOR — resource ownership validated	N/A	No resource ownership in test fixtures
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	Assertions map to verified core behavior (schema, dry-run skipDiscovery, Fetch.failRequest abort); minor vacuous-pass risks noted below
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Servers stopped in finally; reviewer/runner errors captured
High	Correctness	No race conditions or concurrency issues	Pass	Observation state reset per run; servers bound/closed deterministically
Medium	Testing	New code has corresponding tests	Pass	This is test code; strong coverage incl. an explicit negative-guard for the validator
Medium	Testing	Error paths and edge cases tested	Pass	Negative-guard fixture; a couple of soft assertions (see Low findings)
Medium	Testing	Existing tests still pass (no regressions)	Pass	Test-only change; CI 46/46 green
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Config track is fast dry-runs; functional is a single capture
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to a test harness
Medium	Quality	Follows existing codebase patterns	Pass	Matches harness style; one duplicate-spawn consistency gap (see Findings)
Medium	Quality	Changes are focused (single concern)	Pass	Focused regression-coverage PR
Low	Quality	Meaningful names, no dead code	Pass	Clean, well-commented
Low	Quality	Comments explain why, not what	Pass	Good rationale comments throughout
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

• File: test/regression/regression.test.js:36

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: The pre-existing visual-track runner still uses its own inline spawn('npx', …, { shell: true }) and was not refactored onto the new shared runPercy helper. Result: two spawn implementations in the same dir with opposite shell settings; the "no command-injection surface" rationale for dropping shell:true doesn't hold for the most prominent file. Not exploitable (args are static arrays), hence Medium.

• Suggestion: Make regression.test.js import and use runPercy from lib/percy-cli.js, deleting its inline spawn and shell:true.

• File: test/regression/config-validation.test.js (the expectCount: 25 cases)

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: Cases 1/2/4 hard-code expectCount: 25 against snapshots.yml, which is also the visual track's file. Adding any future visual snapshot there (the documented way to add visual coverage) silently breaks the config track with a confusing cross-track count mismatch.

• Suggestion: Point the config cases at a dedicated fixture not shared with the visual track, or assert count >= expected for the shared file while keeping the exact-count assertion only on the focused per-snapshot-options.yml. The real signal (absence of Invalid config:) should remain.

• File: test/regression/lib/percy-cli.js:33 (snapshotCount regex)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Core logs Found 1 snapshot (singular) for a single snapshot, but the regex /Found (\d+) snapshots/ only matches the plural → returns null for a 1-snapshot fixture. Not triggered today (counts are 25/8) but a latent bug for any future single-snapshot fixture.

• Suggestion: Relax to /Found (\d+) snapshot/.

• File: test/regression/functional.test.js:54 (captureSrcset assertion)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The <img src="/gated/srcset-1x.png"> means the 1x route is hit by src regardless of captureSrcset, so obs.srcset.includes('1x') isn't evidence of the feature. Only '2x' is the real signal.

• Suggestion: Assert specifically on '2x' (or point src at a non-candidate URL) so the assertion can't pass on the src alone.

• File: test/regression/functional.test.js:61 (disallowed-probe assertion)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: disallowedProbeRequested === false would also hold if the cross-origin @import were never attempted for an unrelated reason; there's no positive control proving the probe is hit when the host is allowed, so a future regression that stops discovering @import resources could pass vacuously.

• Suggestion: Acceptable tradeoff for E2E; optionally add a one-off allowed-host control or a comment noting the limitation.

• File: .github/workflows/test.yml:138-187 (regression job)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The job is timeout-minutes: 15 and now runs config + visual + functional (the latter two do real captures/uploads sequentially). If visual already approaches the budget, adding functional risks intermittent timeouts.

• Suggestion: Monitor wall-clock and bump the timeout if needed.

Informational (no action required): per-snapshot scope/waitForSelector/readySelectors in per-snapshot-options.yml are schema-validated only (Track C is --dry-run, which skips discovery/capture) — correct by design, just not proof the options work. New harness code uses rmSync/??/?., all available on the pinned Node 14.14+.

Schema & behavioral claims were cross-checked against packages/core/src/config.js, packages/cli-snapshot/src/config.js, and core network.js/percy.js: all fixture options are valid and in-range; the onlyAutomate exclusion list matches the schema exactly; snapshot counts (25, 8) are correct; the disallowed-host request is genuinely aborted before reaching the server; dry-run is token-free.

---

Verdict: PASS — No High/Critical issues. Two Medium findings (adopt the shared runPercy helper in regression.test.js; decouple the config-track count from the shared snapshots.yml) are worth addressing but do not block merge.

[pranavz28]

Coverage audit: ~3% of reachable options not yet exercised

Verified the fixtures against the schema source of truth (packages/core/src/config.js, packages/cli-snapshot/src/config.js) — ~145 of ~150 reachable options are covered, and the onlyAutomate exclusions are correctly justified. Three small gaps remain before "100% of every option" is literally true. All are test-only, non-blocking follow-ups:

1. regions[].elementSelector.elementXpath is never set — config.js:290
The schema accepts three selector forms (boundingBox, elementCSS, elementXpath), but the fixtures only use elementCSS (all-config.yml:80, per-snapshot-options.yml:86) and boundingBox (all-config.yml:90). The elementXpath form is unexercised.
Fix: add one region using elementSelector.elementXpath to all-config.yml (no snapshot-count change → Track C still asserts 25).

2. algorithm: layout enum value is never set — config.js:303 (snapshot) / config.js:321 (region)
The enum is ['standard','layout','ignore','intelliignore'], but fixtures only use standard, intelliignore, and ignore. The layout value is never validated.
Fix: set one algorithm: layout (e.g. a region or the top-level snapshot.algorithm) in all-config.yml.

3. Server mode (serve / port) is not exercised — config.js:831-832
The suite runs list-mode and --base-url mode; the core /snapshot/server schema reached via percy snapshot <dir> isn't covered by any fixture. (Borderline — arguably out of scope for "percy snapshot config," but it is reachable.)
Fix (optional): a minimal serve/port fixture, or document it as an explicit non-goal in COVERAGE.md alongside the onlyAutomate exclusions.

Items 1–2 are two-line additions that make the literal-100% claim accurate; item 3 is optional. Great PR overall — the Track C additionalProperties: false validation genuinely proves every fixture option is schema-recognized.

[AkashBrowserStack]

@pranavz28 thanks for the careful audit — all three gaps are now closed in b04f42f (test-only):

1. regions[].elementSelector.elementXpath — added a region using the elementXpath selector form in all-config.yml (alongside the existing elementCSS and boundingBox regions). No snapshot-count change → Track C still asserts 25.
2. algorithm: layout — that same new region uses algorithm: layout, so the enum value is now validated (the suite already covered standard, intelliignore, and ignore).
3. Server mode (serve) — added a minimal static-site/ fixture plus a Track C case percy snapshot static-site --dry-run --clean-urls (2 snapshots), which exercises the /snapshot/server path. port is not reachable via percy snapshot — there's no --port flag on the command and it isn't in the static config namespace (the command only sets serve/cleanUrls/baseUrl for a directory; see packages/cli-snapshot/src/snapshot.js), so it's a programmatic/SDK-only option. I've documented it as an explicit non-goal in COVERAGE.md alongside the onlyAutomate exclusions rather than fake-exercise it.

COVERAGE.md updated to reflect all three region selector forms, the full algorithm enum (incl. layout), and the serve vs port distinction. That makes the literal-100% claim accurate for every CLI-reachable option. Appreciate the thorough review! 🙏

[AkashBrowserStack]

Claude Code PR Review

PR: #2312 • Head: b04f42f • Reviewers: stack:code-reviewer

Summary

Re-review of the test-only PR adding three regression tracks (config-validation, visual, functional discovery) to test/regression/. The harnesses are well-constructed, assertions are non-vacuous and verify genuine CLI behavior, fixtures are schema-faithful, and all prior review findings are genuinely resolved (verified against the actual core source, not just claimed). No High/Critical issues.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	password: secret / abc123 are local-server fixture stubs
High	Security	Authentication/authorization checks present	N/A	Test harness; no production auth code
High	Security	Input validation and sanitization	Pass	Gated routes are exact-match static responders; serveStaticFile traversal guard intact
High	Security	No IDOR — resource ownership validated	N/A	No resource ownership in fixtures
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	Assertions match verified core behavior (skipUploads≠skipDiscovery, disallowed-host abort path, CDP-applied headers)
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Servers stopped in try/finally; runner errors captured
High	Correctness	No race conditions or concurrency issues	Pass	Observation state reset per run; deterministic server bind/close
Medium	Testing	New code has corresponding tests	Pass	This is test code; 3 tracks + negative-guard self-test
Medium	Testing	Error paths and edge cases tested	Pass	invalid-example.yml proves the validator fires; auth route 401s without creds
Medium	Testing	Existing tests still pass (no regressions)	Pass	Test-only; CI green
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	dry-runs + --debug discovery; servers preload assets
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to a test harness
Medium	Quality	Follows existing codebase patterns	Pass	Shared runPercy helper; matches harness style
Medium	Quality	Changes are focused (single concern)	Pass	Focused regression-coverage PR
Low	Quality	Meaningful names, no dead code	Pass	Clear naming, well-commented
Low	Quality	Comments explain why, not what	Pass	Good rationale comments + COVERAGE.md/README
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies

Findings

Prior findings — all verified resolved:

• lib/percy-cli.js — spawn-shell-true gone (array-arg spawn, no shell:true).
• pages/functional-discovery.html — missing-integrity gone (cross-origin probe via @import, not <link>).
• Functional track --debug/skipUploads — verified correct in core/percy.js (skipUploads does NOT imply skipDiscovery; uploads gated on !skipUploads): discovery runs, servers observe, no build created, token-free.
• Coverage gaps — elementXpath + algorithm: layout region added; static-site/ + Track C case exercise serve; port documented as not-CLI-reachable.

Remaining (all Low / nits, none blocking):

• File: test/regression/regression.test.js:36 · Low · Pre-existing inline spawn(..., { shell: true }) not refactored onto the shared runPercy helper. Out of scope — this file is not in the PR's diff — but a follow-up could move it onto lib/percy-cli.js to clear the latent semgrep pattern.
• File: test/regression/lib/percy-cli.js (snapshotCount) · Low · Regex /Found (\d+) snapshots/ misses the singular Found 1 snapshot form. No impact today (all counts > 1); tighten to /Found (\d+) snapshots?/ for future single-snapshot fixtures.
• File: test/regression/config-validation.test.js (expectCount) · Low · Counts are coupled to the shared snapshots.yml/per-snapshot-options.yml; verified accurate (25 / 8). Intentional (catches dropped snapshots) and documented in COVERAGE.md.
• File: test/regression/functional.test.js:36 · Low (nit) · --verbose is redundant alongside --debug (the debug flag already raises log level). Harmless.

---

Verdict: PASS — Clean, well-engineered test-only PR; all prior findings genuinely resolved (verified against core source), only Low/nit items remain, none blocking merge.

[AkashBrowserStack]

Verification — the suite was mutation-tested (it catches regressions, not just passes)

To prove the specs actually fail when an option changes (and only the affected spec moves), each track was mutation-tested. The real percy-cli-regression project was not touched — the visual checks ran in a throwaway project, and the branch was restored after each run.

Track C (config-validation, token-free): set widths: [99999] (out of range) → the "no Invalid config:" assertion fails. ✅

Track F (functional, token-free): each mutation fails its own server-observation assertion → ✅

• remove discovery.disallowedHostnames → "blocked the 9101 probe" fails (probe now reaches the server)
• captureSrcset: false → "2x candidate" fails (only 1x fetched)
• wrong authorization.password → auth route 401s, "authorization sent" fails

Track V (visual): baseline build on main, then mutated builds on feature branches targeting main. Percy flagged a diff on exactly the mutated snapshot(s), everything else unchanged → ✅

• scope removed → Config - Scope 91.27% diff (1 changed / 24 unchanged)
• percyCSS recolor → Config - percyCSS Override 77.56% diff
• disableShadowDOM: true → false → Config - Disable Shadow DOM 40.70% diff
• domTransformation banner text changed → Config - DOM Transformation 0.15% diff (3 changed / 22 unchanged)

The methodology is documented in test/regression/COVERAGE.md ("Verifying the suite actually catches regressions") so it can be re-run before trusting future changes.