Skip to content

fix: merge .percy.yml config options with snapshot options for serializeDOM #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rishigupta1599 wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: merge .percy.yml config options with snapshot options for serializeDOM #223

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9758c6
fix: merge .percy.yml config options with snapshot options for serial…
rishigupta1599 May 5, 2026
4e94aef
fix: guard against null snapshot config in merge (avoid TypeError)
rishigupta1599 Jun 16, 2026
178d599
test: cover .percy.yml config <-> per-snapshot merge precedence
rishigupta1599 Jun 16, 2026
d2339f9
feat: deep-merge .percy.yml config with per-snapshot options
rishigupta1599 Jun 16, 2026
95b75bf
Merge remote-tracking branch 'origin/master' into fix/merge-percy-yml…
rishigupta1599 Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions percy/snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,19 +500,23 @@ def percy_snapshot(driver, name, **kwargs):
driver.execute_script(percy_dom_script)
cookies = driver.get_cookies()

# Merge .percy.yml config options with snapshot options (snapshot options take priority)
config_options = data['config'].get('snapshot') or {}

@rishigupta1599 rishigupta1599 Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Guard config itself against null, not just `config['snapshot']

This fixes snapshot being null, but data['config'] itself can be None: is_percy_enabled() does data.get('config', {}), which only defaults when the key is absent — a healthcheck returning {"config": null} makes data['config'] None, and .get('snapshot') then raises AttributeError. The codebase's own _resolve_readiness_config defends against exactly this (config = percy_config or {}, see snapshot.py:198-200).

Suggestion:

Suggested change
config_options = data['config'].get('snapshot') or {}
config_options = (data['config'] or {}).get('snapshot') or {}

Reviewer: stack:code-review

merged_kwargs = {**config_options, **kwargs}

@rishigupta1599 rishigupta1599 Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Missing tests for the new config-merge behavior

No test covers that config-level snapshot options flow into serializeDOM, that per-call kwargs override config options, or that a null snapshot config degrades to {} on the percy_snapshot path. The existing null-config test (test_snapshot.py:505) only exercises _resolve_readiness_config, a different function.

Suggestion: Add percy_snapshot tests with mocked healthcheck config {'snapshot': {...}} asserting the option reaches the serialize path, a kwargs-override case, and a {'snapshot': None} (and config: None) case.

Reviewer: stack:code-review


# Serialize and capture the DOM
if is_responsive_snapshot_capture(data['config'], **kwargs):
if is_responsive_snapshot_capture(data['config'], **merged_kwargs):
dom_snapshot = capture_responsive_dom(
driver=driver,
cookies=cookies,
config=data['config'],
percy_dom_script=percy_dom_script,
**kwargs,
**merged_kwargs,
)
else:
dom_snapshot = get_serialized_dom(
driver, cookies, percy_config=data.get('config'),
percy_dom_script=percy_dom_script, **kwargs)
percy_dom_script=percy_dom_script, **merged_kwargs)

# Strip SDK-local `readiness` from the snapshot POST body. The CLI
# already has it via healthcheck; sending it again here risks future
Expand Down
Loading