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

Show file tree
Hide file tree
Changes from 4 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
62 changes: 62 additions & 0 deletions src/test/java/io/percy/selenium/SdkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,68 @@ public void snapshotSurvivesReadinessThrow() throws Exception {
assertEquals("<html></html>", result.get("html"));
}

@Test
public void snapshotMergesCliConfigWithPerCallOptionsPrecedence() throws Exception {
// .percy.yml config carries a config-only key (enableJavaScript) and a
// percyCSS value that the per-call option should override.
RemoteWebDriver mockedDriver = mock(RemoteWebDriver.class);
Percy mockedPercy = spy(new Percy(mockedDriver));

setField(mockedPercy, "isPercyEnabled", true);
setField(mockedPercy, "domJs",
"window.PercyDOM = window.PercyDOM || {}; window.PercyDOM.serialize = function(){ return {}; };");
setField(mockedPercy, "cliConfig", new JSONObject().put("snapshot",
new JSONObject()
.put("enableJavaScript", true)
.put("percyCSS", "FROM_CONFIG")));
mockedPercy.sessionType = "web";

when(mockedDriver.getCurrentUrl()).thenReturn("https://example.com");
WebDriver.Options mockedOptions = mock(WebDriver.Options.class);
when(mockedDriver.manage()).thenReturn(mockedOptions);
when(mockedOptions.getCookies()).thenReturn(Collections.emptySet());
when(mockedDriver.findElements(By.tagName("iframe"))).thenReturn(Collections.emptyList());

// Capture every script passed to the JavascriptExecutor so we can inspect
// the PercyDOM.serialize(...) payload that getSerializedDOM builds.
ArgumentCaptor<String> scriptCaptor = ArgumentCaptor.forClass(String.class);
when(((JavascriptExecutor) mockedDriver).executeScript(any(String.class)))
.thenReturn(new HashMap<String, Object>());

// Avoid an actual POST back to the CLI.
doReturn(new JSONObject()).when(mockedPercy)

@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] POST body not asserted

The request(...) stub uses any(JSONObject.class) and never asserts on the posted payload. This SDK intentionally posts the raw per-call options to the CLI (the documented cross-SDK pattern, with config applied server-side), so this is a coverage observation rather than a defect.

Suggestion: Optional — add an ArgumentCaptor<JSONObject> on request(...) if future behavior ever depends on the posted body. Non-blocking.

Reviewer: stack:code-review

.request(eq("/percy/snapshot"), any(JSONObject.class), eq("merge precedence"));

Map<String, Object> options = new HashMap<String, Object>();
options.put("percyCSS", "FROM_CALL");

mockedPercy.snapshot("merge precedence", options);

verify((JavascriptExecutor) mockedDriver, atLeastOnce()).executeScript(scriptCaptor.capture());

String serializeScript = null;
for (String script : scriptCaptor.getAllValues()) {
if (script != null && script.startsWith("return PercyDOM.serialize(")) {
serializeScript = script;
}
}
assertNotNull(serializeScript, "PercyDOM.serialize script should have been executed");

// Extract the JSON argument passed to PercyDOM.serialize(...) and assert
// the merged options reflect config<->per-call precedence.
String jsonArg = serializeScript
.substring(serializeScript.indexOf('(') + 1, serializeScript.lastIndexOf(')'))
.trim();
JSONObject serialized = new JSONObject(jsonArg);

@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] Fragile serialize-arg extraction

The serialized options are recovered via substring/indexOf string slicing of the JS script. This silently breaks if buildSnapshotJS formatting ever changes (e.g. added comment or multiline output).

Suggestion: Optional — capture/verify the serialized map more directly (e.g. spy on the builder) instead of parsing the script string. Non-blocking.

Reviewer: stack:code-review


// Config-only key survives the merge.
assertTrue(serialized.getBoolean("enableJavaScript"),
"enableJavaScript from .percy.yml config should be present in serialized options");
// Per-call option wins over the config value.
assertEquals("FROM_CALL", serialized.getString("percyCSS"),
"per-call percyCSS should override the .percy.yml config value");
}

private static Object invokePrivate(Object target, String methodName, Class<?>[] paramTypes, Object... args)
throws Exception {
Method method = Percy.class.getDeclaredMethod(methodName, paramTypes);
Expand Down
Loading