Skip to content

fix: merge .percy.yml config options into serializeDOM #1151

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 8 commits into
base: master
Choose a base branch
from
+5,246 −4,965
Open

fix: merge .percy.yml config options into serializeDOM #1151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3fb8c00
fix: merge .percy.yml config options with snapshot options for serial…
rishigupta1599 May 5, 2026
b698b95
refactor: use utils.mergeSnapshotOptions() from @percy/sdk-utils
rishigupta1599 May 5, 2026
53d450d
Merge remote-tracking branch 'origin/master' into fix/merge-percy-yml…
rishigupta1599 Jun 16, 2026
f08fb0c
chore: fix lint errors (lint green)
rishigupta1599 Jun 16, 2026
ac11973
fix: send merged config+snapshot options to postSnapshot (restore POS…
rishigupta1599 Jun 16, 2026
da7a6bf
chore: bump @percy deps to ^1.32.0 (published mergeSnapshotOptions); …
rishigupta1599 Jun 17, 2026
6d87a28
ci: run on Node 20 (Node 16 can no longer install the ember toolchain)
rishigupta1599 Jun 19, 2026
e8d92bc
test: update canvas serialize specs for @percy/dom 1.32.0
rishigupta1599 Jun 19, 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
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: 16
node-version: 20
- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "::set-output name=dir::$(yarn cache dir)"
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: 16
node-version: 20
registry-url: 'https://registry.npmjs.org'
- name: Get yarn cache directory path
id: yarn-cache-dir-path
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
node: [16]
node: [20]
runs-on: ${{ matrix.os }}
steps:
- uses: actions-ecosystem/action-regex-match@v2
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/typecheck.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: 16
node-version: 20
- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "::set-output name=dir::$(yarn cache dir)"
Expand Down
18 changes: 7 additions & 11 deletions addon-test-support/@percy/ember/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,6 @@ function generateName(assertOrTestOrName) {
}
}

// Helper to add pseudoClassEnabledElements that are in .percy.yml file to snapshot options.
// when options.pseudoClassEnabledElements is not set in percySnapshot options
function addPseudoClassEnabledELements(options) {
if (!options.pseudoClassEnabledElements && utils.percy?.config?.snapshot?.pseudoClassEnabledElements) {
options.pseudoClassEnabledElements = utils.percy.config.snapshot.pseudoClassEnabledElements;
}
}

// Helper to scope a DOM snapshot to the ember-testing container to capture the
// ember application without the testing UI
function scopeDOM(scope, dom) {
Expand Down Expand Up @@ -78,7 +70,8 @@ export default async function percySnapshot(name, {
// window.PercyDOM mid-flight if the caller forgot to await — capture once.
const PercyDOM = window.PercyDOM;

addPseudoClassEnabledELements(options);
// Merge .percy.yml config options with snapshot options (snapshot options take priority)
const mergedOptions = utils.mergeSnapshotOptions(options);

@rishigupta1599 rishigupta1599 Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

[Critical] utils.mergeSnapshotOptions is not exported by @percy/sdk-utils

utils here is @percy/sdk-utils, whose index exports do not include mergeSnapshotOptions (it lives in @percy/core/src/snapshot.js). At runtime this is undefined(...) and throws TypeError: utils.mergeSnapshotOptions is not a function. The error is swallowed by the surrounding try/catch and logged as a generic "Could not take DOM snapshot", so every snapshot silently fails.

Suggestion: Either export mergeSnapshotOptions from @percy/sdk-utils (and bump the dependency), or merge inline: const mergedOptions = { ...(utils.percy?.config?.snapshot || {}), ...options };

Reviewer: stack:code-review

@rishigupta1599 rishigupta1599 Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

[High] Config-merged options never reach postSnapshot

mergedOptions is spread only into PercyDOM.serialize (L129). The posted payload is built from forwardOpts, which is destructured from the raw options (const { readiness: _readiness, ...forwardOpts } = options; at L149) — so .percy.yml config.snapshot keys not passed per-snapshot are dropped from the post body. On base master the old helper mutated options in place, so the merged value reached both serialize and post. The existing test uses pseudoClassEnabledElements from percy config when option is not passed (tests/acceptance/index-test.js:129-140) asserts the /percy/snapshot post body and will now fail. This is a genuine regression, separate from the sdk-utils publish ordering.

Suggestion: Derive the posted options from the merged set:

const { readiness: _readiness, ...forwardOpts } = mergedOptions;

And base the readiness checks (L90, L96-98) on mergedOptions too for consistency.

Reviewer: stack:code-review


// Readiness gate. Backward-compat:
// - Older CLI bundles lack PercyDOM.waitForReady — typeof guard handles that.
Expand Down Expand Up @@ -133,7 +126,7 @@ export default async function percySnapshot(name, {
domTransformation: dom => scopeDOM(emberTestingScope, (
domTransformation ? domTransformation(dom) : dom
)),
...options
...mergedOptions

@rishigupta1599 rishigupta1599 Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

[High] Merge applied only to serialize; readiness gate and post payload still use raw options

...mergedOptions is spread here, but the readiness block and const { readiness, ...forwardOpts } = options still read the un-merged options. The old addPseudoClassEnabledELements mutated options in place so config values reached postSnapshot; now config-level snapshot settings (including pseudoClassEnabledElements) are dropped from the post payload.

Suggestion: Derive forwardOpts and the readiness inputs from mergedOptions so merged config flows consistently to readiness, serialize, and post.

Reviewer: stack:code-review

});

// Attach readiness diagnostics so the CLI can log timing and pass/fail.
Expand All @@ -152,8 +145,11 @@ export default async function percySnapshot(name, {

// Strip `readiness` before posting — it's SDK-local and the CLI already
// has it from .percy.yml healthcheck. Avoids round-tripping config.
// Forward the SAME merged options used for serialize so config-level keys
// (from .percy.yml) reach the POST body too, not just per-call options.
// Per-call precedence is preserved by mergeSnapshotOptions.
// eslint-disable-next-line no-unused-vars
const { readiness: _readiness, ...forwardOpts } = options;
const { readiness: _readiness, ...forwardOpts } = mergedOptions;

// Post the DOM to the snapshot endpoint with snapshot options and other info
await utils.postSnapshot({
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
},
"packageManager": "yarn@1.22.22",
"dependencies": {
"@percy/sdk-utils": "^1.31.15-beta.0",
"@percy/sdk-utils": "^1.32.0",
"ember-auto-import": "^2.10.0",
"ember-cli-babel": "^8.2.0"
},
Expand All @@ -49,7 +49,7 @@
"@ember/test-helpers": "^2.2.5",
"@glimmer/component": "^1.0.4",
"@glimmer/tracking": "^1.0.4",
"@percy/cli": "^1.31.1",
"@percy/cli": "^1.32.0",
"@types/mocha": "^10.0.0",
"@types/qunit": "^2.11.1",
"babel-eslint": "^10.1.0",
Expand Down
4 changes: 2 additions & 2 deletions tests/acceptance/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ module('percySnapshot', hooks => {
test("serialize canvas when enableJavascript is not present", async assert => {
await percySnapshot('Snapshot 1');
assert.matches((await helpers.get('requests'))[1].body.domSnapshot.html, (
/<body class="ember-application"><img src=".*" data-percy-element-id=".*" data-percy-canvas-serialized="" style="max-width: 100%;"><\/body>/));
/<body class="ember-application"><img data-percy-element-id=".*" data-percy-canvas-serialized="" src=".*" style="max-width: 100%;"><\/body>/));
});

test("doesn't serialize canvas when enableJavascript is true", async assert => {
Expand All @@ -120,7 +120,7 @@ module('percySnapshot', hooks => {
test("removes canvas element when dom transformation is passed", async assert => {
await percySnapshot('Snapshot 1', {
domTransformation: (html) => { html.querySelector('canvas')?.remove(); return html; },
enable_javascript: true
enableJavaScript: true
});
assert.matches((await helpers.get('requests'))[1].body.domSnapshot.html, (
/<body class="ember-application"><\/body>/));
Expand Down
Loading
Loading