Skip to content

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

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
+210 −3
Open

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9ed826
fix: merge .percy.yml config options with snapshot options for serial…
rishigupta1599 May 5, 2026
65470c8
Merge remote-tracking branch 'origin/master' into fix/merge-percy-yml…
rishigupta1599 Jun 16, 2026
fef3e61
fix: do not let null per-call options clobber .percy.yml config in merge
rishigupta1599 Jun 16, 2026
ede01f6
test: cover .percy.yml config <-> per-snapshot merge precedence
rishigupta1599 Jun 16, 2026
449acb4
feat: deep-merge .percy.yml config with per-snapshot options
rishigupta1599 Jun 16, 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
25 changes: 22 additions & 3 deletions src/main/java/io/percy/selenium/Percy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,25 @@ public JSONObject snapshot(String name, Map<String, Object> options) {

Object domSnapshot = null;

// Merge .percy.yml config options with snapshot options (snapshot options take priority)
Map<String, Object> mergedOptions = new HashMap<>();
if (cliConfig != null && cliConfig.has("snapshot") && !cliConfig.isNull("snapshot")) {
JSONObject snapshotConfig = cliConfig.getJSONObject("snapshot");
for (String key : snapshotConfig.keySet()) {
mergedOptions.put(key, snapshotConfig.get(key));

@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.

[Low] Config-supplied widths becomes a JSONArray, not a List

snapshotConfig.get(key) returns org.json native types. A widths array from .percy.yml is copied into mergedOptions as a JSONArray; Java-side consumers that branch on instanceof List would silently ignore it (the serialize/POST new JSONObject(options) path is unaffected).

Suggestion: convert JSONArray values to List when copying, or add a test confirming config-supplied widths flow through.

Reviewer: stack:code-review

@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] Config values land in mergedOptions as raw org.json types

snapshotConfig.get(key) puts JSONObject/JSONArray instances into mergedOptions. buildSnapshotJS re-wraps via new JSONObject(options) so serialization is fine, but extractResponsiveWidths checks instanceof List<?> and resolveReadinessConfig checks instanceof Map — a widths/readiness value sourced from .percy.yml would be a JSONArray/JSONObject and silently miss those branches.

Suggestion: Seed from fully-unwrapped Java types, e.g. mergedOptions.putAll(snapshotConfig.toMap());

Reviewer: stack:code-reviewer

}
}
if (options != null) {
// Only overlay non-null per-call options so a null value (set by the
// positional snapshot() overloads for unset params) does not clobber a
// real value coming from .percy.yml config.
for (Map.Entry<String, Object> entry : options.entrySet()) {
if (entry.getValue() != null) {
mergedOptions.put(entry.getKey(), entry.getValue());
}
}
}

try {
JavascriptExecutor jse = (JavascriptExecutor) driver;
jse.executeScript(fetchPercyDOM());
Expand All @@ -383,10 +402,10 @@ public JSONObject snapshot(String name, Map<String, Object> options) {
} catch(Exception e) {
log("Cookie collection failed " + e.getMessage(), "debug");
}
if (isCaptureResponsiveDOM(options)) {
domSnapshot = captureResponsiveDom(driver, cookies, options);
if (isCaptureResponsiveDOM(mergedOptions)) {
domSnapshot = captureResponsiveDom(driver, cookies, mergedOptions);
} else {
domSnapshot = getSerializedDOM(jse, cookies, options);
domSnapshot = getSerializedDOM(jse, cookies, mergedOptions);
}
} catch (WebDriverException e) {
// For some reason, the execution in the browser failed.
Expand Down
Loading