Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
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
22 changes: 22 additions & 0 deletions packages/core/src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,23 @@ export const configSchema = {
sync: {
type: 'boolean'
},
readiness: {
type: 'object',
additionalProperties: false,
properties: {
preset: { type: 'string', enum: ['balanced', 'strict', 'fast', 'disabled'] },
stabilityWindowMs: { type: 'integer', minimum: 50, maximum: 30000 },
Comment thread
Shivanshu-07 marked this conversation as resolved.
jsIdleWindowMs: { type: 'integer', minimum: 50, maximum: 30000 },
networkIdleWindowMs: { type: 'integer', minimum: 50, maximum: 10000 },
timeoutMs: { type: 'integer', minimum: 1000, maximum: 60000 },
imageReady: { type: 'boolean' },
fontReady: { type: 'boolean' },
jsIdle: { type: 'boolean' },
readySelectors: { type: 'array', items: { type: 'string' } },
notPresentSelectors: { type: 'array', items: { type: 'string' } },
maxTimeoutMs: { type: 'integer', minimum: 1000, maximum: 60000 }
}
},
responsiveSnapshotCapture: {
type: 'boolean',
default: false
Expand Down Expand Up @@ -489,6 +506,7 @@ export const snapshotSchema = {
domTransformation: { $ref: '/config/snapshot#/properties/domTransformation' },
enableLayout: { $ref: '/config/snapshot#/properties/enableLayout' },
sync: { $ref: '/config/snapshot#/properties/sync' },
readiness: { $ref: '/config/snapshot#/properties/readiness' },
responsiveSnapshotCapture: { $ref: '/config/snapshot#/properties/responsiveSnapshotCapture' },
testCase: { $ref: '/config/snapshot#/properties/testCase' },
labels: { $ref: '/config/snapshot#/properties/labels' },
Expand Down Expand Up @@ -682,6 +700,10 @@ export const snapshotSchema = {
type: 'array',
items: { type: 'string' }
},
readiness_diagnostics: {
type: 'object',
description: 'Diagnostics from readiness checks run before serialization'
},
corsIframes: {
type: 'array',
items: {
Expand Down
28 changes: 22 additions & 6 deletions packages/core/src/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,15 +213,31 @@ export class Page {

await this.insertPercyDom();

// serialize and capture a DOM snapshot
this.log.debug('Serialize DOM', this.meta);
// Run readiness checks before serializing — wait for page stability
let readiness = snapshot.readiness || this.browser?.percy?.config?.snapshot?.readiness;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/* istanbul ignore next */ on the outer await this.eval(...) also swallows the .catch(e => this.log.debug(...)) handler, which is plain Node-side JS that is testable (reject the eval spy and assert the debug log). Move the ignore to the inner arrow only, or drop it and cover the catch — it's the kind of silent failure path (Readiness check failed: ...) we rely on in production triage.

if (readiness && readiness.preset !== 'disabled') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

diagnostics is fetched here but never attached to capture/domSnapshot. Combined with the new reader in snapshot.js that looks for migrated.domSnapshot?.readiness_diagnostics, that reader is effectively dead code on the CLI URL path — we capture diagnostics only to log timed_out locally and then drop the rest.

If the intent is to surface readiness metrics on the backend/UI, merge diagnostics into capture (e.g. return { ...snapshot, ...capture, readiness_diagnostics: diagnostics }) or attach onto domSnapshot before returning. Otherwise please remove the reader in snapshot.js until an SDK path actually embeds them — right now it's a no-op branch with a misleading implication.

this.log.debug('Waiting for readiness', this.meta);
/* istanbul ignore next: no instrumenting injected code */
let diagnostics = await this.eval(async (_, config) => {
// eslint-disable-next-line no-undef
if (typeof PercyDOM?.waitForReady === 'function') return PercyDOM.waitForReady(config);
}, readiness).catch(e => {
this.log.debug(`Readiness check failed: ${e}`, this.meta);
});

if (diagnostics?.timed_out) {
this.log.debug('Readiness timed out, capturing anyway', this.meta);
}
}

// Serialize the DOM — always sync
this.log.debug('Serialize DOM', this.meta);
/* istanbul ignore next: no instrumenting injected code */
let capture = await this.eval((_, options) => ({
let capture = await this.eval((_, options) => {
/* eslint-disable-next-line no-undef */
domSnapshot: PercyDOM.serialize(options),
url: document.URL
}), { enableJavaScript, disableShadowDOM, forceShadowAsLightDOM, domTransformation, reshuffleInvalidTags, ignoreCanvasSerializationErrors, ignoreStyleSheetSerializationErrors, pseudoClassEnabledElements });
return { domSnapshot: PercyDOM.serialize(options), url: document.URL };
}, { enableJavaScript, disableShadowDOM, forceShadowAsLightDOM, domTransformation, reshuffleInvalidTags, ignoreCanvasSerializationErrors, ignoreStyleSheetSerializationErrors, pseudoClassEnabledElements });

return { ...snapshot, ...capture };
}
Expand Down
11 changes: 11 additions & 0 deletions packages/core/src/snapshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,17 @@ export function validateSnapshotOptions(options) {
log.warn('Encountered snapshot serialization warnings:');
for (let w of domWarnings) log.warn(`- ${w}`);
}

// log readiness diagnostics when present (SDK-submitted snapshots with readiness enabled)
let readinessDiag = migrated.domSnapshot?.readiness_diagnostics;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two concerns:

  1. migrated.domSnapshot is a union of string (legacy SDK payload — just HTML) and object ({ html, warnings, ... }). Optional chaining on a string returns undefined so this is safe today, but an explicit typeof migrated.domSnapshot === 'object' gate reads more clearly and avoids future footguns.

  2. Nothing in this PR actually populates domSnapshot.readiness_diagnostics — the CLI path in page.js discards diagnostics, and no SDK yet embeds them. This reader will stay dormant until an SDK change lands. Either land the producer in the same PR (attach diagnostics in page.js, update sdk-utils helpers to embed them in the returned payload) or drop this block until the producer ships — to avoid confusion about what's actually wired up.

if (readinessDiag) {
if (readinessDiag.timed_out) {
log.warn(`Readiness timed out after ${readinessDiag.total_duration_ms}ms (preset: ${readinessDiag.preset || 'custom'})`);
} else {
log.debug(`Readiness passed in ${readinessDiag.total_duration_ms}ms (preset: ${readinessDiag.preset || 'custom'})`);
}
}

// warn on validation errors
let errors = PercyConfig.validate(migrated, schema);
if (errors?.length > 0) {
Expand Down
90 changes: 90 additions & 0 deletions packages/core/test/percy.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ describe('Percy', () => {
});

// expect required arguments are passed to PercyDOM.serialize
// readiness is now a separate waitForReady call, not part of serialize options
expect(evalSpy.calls.allArgs()[3]).toEqual(jasmine.arrayContaining([jasmine.anything(), { enableJavaScript: undefined, disableShadowDOM: true, domTransformation: undefined, reshuffleInvalidTags: undefined, ignoreCanvasSerializationErrors: undefined, ignoreStyleSheetSerializationErrors: undefined, forceShadowAsLightDOM: undefined, pseudoClassEnabledElements: undefined }]));

expect(snapshot.url).toEqual('http://localhost:8000/');
Expand All @@ -120,6 +121,95 @@ describe('Percy', () => {
}));
});

it('runs readiness check before serializing when readiness option is set', async () => {
server.reply('/', () => [200, 'text/html', '<p>Hello Percy!</p>']);

await percy.browser.launch();
let page = await percy.browser.page();
await page.goto('http://localhost:8000');

percy.loglevel('debug');

let snapshot = await page.snapshot({
readiness: { preset: 'fast', timeoutMs: 1000 }
});

expect(logger.stderr).toEqual(jasmine.arrayContaining([
jasmine.stringMatching(/Waiting for readiness/)
]));
expect(snapshot.url).toEqual('http://localhost:8000/');
});

it('skips readiness check when readiness preset is disabled', async () => {
server.reply('/', () => [200, 'text/html', '<p>Hello Percy!</p>']);

await percy.browser.launch();
let page = await percy.browser.page();
await page.goto('http://localhost:8000');

percy.loglevel('debug');

let snapshot = await page.snapshot({
readiness: { preset: 'disabled' }
});

expect(logger.stderr).not.toEqual(jasmine.arrayContaining([
jasmine.stringMatching(/Waiting for readiness/)
]));
expect(snapshot.url).toEqual('http://localhost:8000/');
});

it('logs when readiness times out and continues to capture', async () => {
server.reply('/', () => [200, 'text/html', '<p>Hello Percy!</p>']);

await percy.browser.launch();
let page = await percy.browser.page();
await page.goto('http://localhost:8000');

percy.loglevel('debug');

// Force the readiness eval to resolve with a timed_out diagnostics
// payload while letting all other eval calls (PercyDOM injection,
// serialize, etc.) pass through untouched. The readiness eval is the
// only call whose first arg is a config object containing `preset`.
let originalEval = page.eval.bind(page);
spyOn(page, 'eval').and.callFake((fn, ...args) => {
if (args[0] && typeof args[0] === 'object' && 'preset' in args[0]) {
return Promise.resolve({ timed_out: true, total_duration_ms: 1234 });
}
return originalEval(fn, ...args);
});

let snapshot = await page.snapshot({
readiness: { preset: 'balanced' }
});

expect(logger.stderr).toEqual(jasmine.arrayContaining([
jasmine.stringMatching(/Waiting for readiness/),
jasmine.stringMatching(/Readiness timed out, capturing anyway/)
]));
expect(snapshot.url).toEqual('http://localhost:8000/');
});

it('falls back to the percy config readiness when none is set per snapshot', async () => {
server.reply('/', () => [200, 'text/html', '<p>Hello Percy!</p>']);

percy.config.snapshot.readiness = { preset: 'fast', timeoutMs: 1000 };

await percy.browser.launch();
let page = await percy.browser.page();
await page.goto('http://localhost:8000');

percy.loglevel('debug');

let snapshot = await page.snapshot({});

expect(logger.stderr).toEqual(jasmine.arrayContaining([
jasmine.stringMatching(/Waiting for readiness/)
]));
expect(snapshot.url).toEqual('http://localhost:8000/');
});

describe('.start()', () => {
// rather than stub prototypes, extend and mock
class TestPercy extends Percy {
Expand Down
62 changes: 62 additions & 0 deletions packages/core/test/snapshot.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,68 @@ describe('Snapshot', () => {
expect(uploads[2]).toEqual(Buffer.from(textResource.content).toString('base64'));
});

it('warns when readiness diagnostics indicate a timeout', async () => {
// domSnapshot is sent as a JSON string by SDKs, which preserves snake_case
// keys like readiness_diagnostics through option normalization.
await percy.snapshot({
name: 'Readiness Timed Out',
url: 'http://localhost:8000/',
domSnapshot: JSON.stringify({
html: testDOM,
readiness_diagnostics: {
timed_out: true,
total_duration_ms: 12345,
preset: 'balanced'
}
})
});

expect(logger.stderr).toEqual(jasmine.arrayContaining([
'[percy] Readiness timed out after 12345ms (preset: balanced)'
]));
expect(logger.stdout).toEqual(jasmine.arrayContaining([
'[percy] Snapshot taken: Readiness Timed Out'
]));
});

it('falls back to "custom" preset label when readiness timeout omits the preset', async () => {
await percy.snapshot({
name: 'Readiness Timed Out Custom',
url: 'http://localhost:8000/',
domSnapshot: JSON.stringify({
html: testDOM,
readiness_diagnostics: {
timed_out: true,
total_duration_ms: 9999
}
})
});

expect(logger.stderr).toEqual(jasmine.arrayContaining([
'[percy] Readiness timed out after 9999ms (preset: custom)'
]));
});

it('debug logs readiness diagnostics when readiness passes', async () => {
percy.loglevel('debug');

await percy.snapshot({
name: 'Readiness Passed',
url: 'http://localhost:8000/',
domSnapshot: JSON.stringify({
html: testDOM,
readiness_diagnostics: {
timed_out: false,
total_duration_ms: 250
}
})
});

expect(logger.stderr).toEqual(jasmine.arrayContaining([
'[percy:core:snapshot] Readiness passed in 250ms (preset: custom)'
]));
});

it('handles duplicate snapshots when testCase is not passed', async () => {
await percy.snapshot([{
url: 'http://localhost:8000/foobar',
Expand Down
2 changes: 2 additions & 0 deletions packages/dom/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ export {
} from './serialize-dom';

export { loadAllSrcsetLinks } from './serialize-image-srcset';

export { waitForReady } from './readiness';
Loading
Loading