Skip to content

feat: PER-7348 add waitForReady() call before serialize() #218

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
Shivanshu-07 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: PER-7348 add waitForReady() call before serialize() #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8be478b
feat: PER-7348 add waitForReady() call before serialize()
Shivanshu-07 Apr 17, 2026
e793c46
fix: capture readiness diagnostics and attach to domSnapshot (PER-7348)
Shivanshu-07 Apr 20, 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
45 changes: 45 additions & 0 deletions percy/snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,54 @@ def _get_origin(url):
parsed = urlparse(url)
return f"{parsed.scheme}://{parsed.netloc}"

def _wait_for_ready(driver, kwargs):
"""Run readiness checks before serialize. PER-7348.

Sends PercyDOM.waitForReady via execute_async_script. The script checks
typeof PercyDOM.waitForReady in-browser so older CLI versions without the
method are a graceful no-op. Any failure is caught and logged at debug;
serialize still runs.

Returns readiness diagnostics dict (or None) so callers can attach it
to the domSnapshot for CLI-side logging.

Readiness config precedence: kwargs['readiness'] > cached
percy.config.snapshot.readiness > {} (CLI falls back to balanced default).
If preset is 'disabled', skip the async script call entirely.
"""
readiness_config = kwargs.get('readiness')
if readiness_config is None:
data = is_percy_enabled()
Copy link
Copy Markdown
Contributor

@rishigupta1599 rishigupta1599 Apr 20, 2026

Choose a reason for hiding this comment

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

Redundant call. percy_snapshot (the only production caller path) has already called is_percy_enabled() and has data['config'] in scope. Plumb the config through explicitly rather than relying on the lru_cache re-lookup — it makes the data flow clearer and avoids a surprise dependency on the cache for anyone who calls get_serialized_dom directly (module-level, no leading underscore).

if isinstance(data, dict):
readiness_config = (data.get('config') or {}).get('snapshot', {}).get('readiness', {}) or {}
Copy link
Copy Markdown
Contributor

@rishigupta1599 rishigupta1599 Apr 20, 2026

Choose a reason for hiding this comment

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

If config.snapshot is ever returned from the CLI as a non-dict (e.g. null deserialized to None), .get('readiness', {}) raises AttributeError and blows up the snapshot call. Consider ((data.get('config') or {}).get('snapshot') or {}).get('readiness') or {} for symmetry with the other guards.

else:
readiness_config = {}
if isinstance(readiness_config, dict) and readiness_config.get('preset') == 'disabled':
return None
try:
diagnostics = driver.execute_async_script(
'var config = ' + json.dumps(readiness_config) + ';'
Copy link
Copy Markdown
Contributor

@rishigupta1599 rishigupta1599 Apr 20, 2026

Choose a reason for hiding this comment

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

Two concerns on this execute_async_script call:

  1. No script timeout is set on the driver. execute_async_script uses the session's global script timeout (Selenium default ~30s, but BrowserStack hubs and some remotes run lower). If a user configures readiness.timeoutMs higher than the driver's script timeout, WebDriver fires ScriptTimeoutException before the in-page Promise resolves — so the user-facing timeout config is silently capped. Either driver.set_script_timeout(readiness.timeoutMs / 1000 + buffer) around this call (and restore the previous value) or document the interaction prominently.

  2. Readiness runs once per width for responsive snapshots. capture_responsive_dom calls get_serialized_dom inside its per-width loop, so every responsive snapshot pays the readiness cost N times. With a 10s timeoutMs across 3 widths that's up to 30s of sequential waits per snapshot. Consider running readiness once before the loop and short-circuiting inside get_serialized_dom for responsive calls (e.g. a skip_readiness=True kwarg).

'var done = arguments[arguments.length - 1];'
'try {'
" if (typeof PercyDOM !== 'undefined' && typeof PercyDOM.waitForReady === 'function') {"
' PercyDOM.waitForReady(config).then(function(r){ done(r); }).catch(function(){ done(); });'
' } else { done(); }'
'} catch(e) { done(); }'
)
return diagnostics
except Exception as e:
log(f'waitForReady failed, proceeding to serialize: {e}', 'debug')
return None


def get_serialized_dom(driver, cookies, percy_dom_script=None, **kwargs):
# 0. Readiness gate before serialize (PER-7348). Graceful on old CLI.
readiness_diagnostics = _wait_for_ready(driver, kwargs)
Copy link
Copy Markdown
Contributor

@rishigupta1599 rishigupta1599 Apr 20, 2026

Choose a reason for hiding this comment

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

readiness stays in kwargs after this call and then gets:

  1. JSON-serialized into the PercyDOM.serialize(kwargs) argument on the next line — harmless today because @percy/dom ignores unknown keys, but it's an implicit coupling.
  2. Spread into the requests.post(..., json={**kwargs, ...}) body in percy_snapshot at the end of the function, so the CLI receives a top-level readiness alongside snapshot options. That likely isn't what the CLI expects (readiness config is supposed to flow via healthcheck per the PR description), and it's easy for a future CLI-side validator to reject unknown top-level fields.

Safer: kwargs.pop('readiness', None) inside _wait_for_ready once you've consumed it (or pop at this call site) so neither serialize() nor the snapshot POST sees it.

# 1. Serialize the main page first (this adds the data-percy-element-ids)
dom_snapshot = driver.execute_script(f'return PercyDOM.serialize({json.dumps(kwargs)})')
# Attach readiness diagnostics so the CLI can log timing and pass/fail
if readiness_diagnostics and isinstance(dom_snapshot, dict):
dom_snapshot['readiness_diagnostics'] = readiness_diagnostics
Copy link
Copy Markdown
Contributor

@rishigupta1599 rishigupta1599 Apr 20, 2026

Choose a reason for hiding this comment

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

Minor: the readiness_diagnostics truthiness check drops falsy-but-present results. The PR description specifically calls out { timed_out: true } as a signal the CLI needs for logging — that one's fine because it's truthy — but if waitForReady ever resolves with an empty dict {} (e.g. disabled preset short-circuit inside the JS), the diagnostics are silently dropped. Prefer if readiness_diagnostics is not None and isinstance(dom_snapshot, dict):.

# 2. Process CORS IFrames
try:
page_origin = _get_origin(driver.current_url)
Expand Down
66 changes: 66 additions & 0 deletions tests/test_snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,72 @@ def test_raise_error_poa_token_with_snapshot(self):
" For more information on usage of PercyScreenshot, refer https://www.browserstack.com"\
"/docs/percy/integrate/functional-and-visual", str(context.exception))

# --- Readiness gate (PER-7348) ---------------------------------------

def test_readiness_runs_before_serialize_by_default(self):
mock_healthcheck()
mock_snapshot()

with patch.object(self.driver, 'execute_async_script', wraps=self.driver.execute_async_script) as async_spy, \
patch.object(self.driver, 'execute_script', wraps=self.driver.execute_script) as sync_spy:
percy_snapshot(self.driver, 'readiness-happy-path')

async_scripts = [c.args[0] for c in async_spy.call_args_list if c.args]
sync_scripts = [c.args[0] for c in sync_spy.call_args_list if c.args]
# Readiness call made at least once, and contains the typeof guard + waitForReady
self.assertTrue(any('waitForReady' in s and 'typeof PercyDOM' in s for s in async_scripts),
f'expected readiness script via execute_async_script, got: {async_scripts}')
# Serialize call made at least once via sync execute_script
self.assertTrue(any('PercyDOM.serialize' in s for s in sync_scripts),
f'expected serialize via execute_script, got: {sync_scripts}')

def test_readiness_uses_per_snapshot_config(self):
mock_healthcheck()
mock_snapshot()

readiness = {'preset': 'strict', 'stabilityWindowMs': 500}
with patch.object(self.driver, 'execute_async_script', wraps=self.driver.execute_async_script) as async_spy:
percy_snapshot(self.driver, 'readiness-config', readiness=readiness)

scripts = [c.args[0] for c in async_spy.call_args_list if c.args]
readiness_scripts = [s for s in scripts if 'waitForReady' in s]
self.assertTrue(readiness_scripts, 'readiness script should have been sent')
# JSON-serialized config embedded in the script
self.assertIn('"preset": "strict"', readiness_scripts[0])
self.assertIn('"stabilityWindowMs": 500', readiness_scripts[0])

def test_readiness_skipped_when_preset_disabled(self):
mock_healthcheck()
mock_snapshot()

with patch.object(self.driver, 'execute_async_script', wraps=self.driver.execute_async_script) as async_spy, \
patch.object(self.driver, 'execute_script', wraps=self.driver.execute_script) as sync_spy:
percy_snapshot(self.driver, 'readiness-disabled',
readiness={'preset': 'disabled'})

async_scripts = [c.args[0] for c in async_spy.call_args_list if c.args]
sync_scripts = [c.args[0] for c in sync_spy.call_args_list if c.args]
self.assertFalse(any('waitForReady' in s for s in async_scripts),
f'readiness script should NOT have been sent, got: {async_scripts}')
# Serialize still ran
self.assertTrue(any('PercyDOM.serialize' in s for s in sync_scripts))

def test_snapshot_still_posts_when_readiness_raises(self):
mock_healthcheck()
mock_snapshot()

# Make the readiness call raise; serialize should still run and the
# snapshot POST should still be made.
def explode(*args, **kwargs):
raise RuntimeError('readiness boom')

with patch.object(self.driver, 'execute_async_script', side_effect=explode):
percy_snapshot(self.driver, 'readiness-boom')

# Snapshot endpoint was hit
paths = [req.path for req in httpretty.latest_requests()]
Copy link
Copy Markdown
Contributor

@rishigupta1599 rishigupta1599 Apr 20, 2026

Choose a reason for hiding this comment

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

This test validates the POST happens when execute_async_script raises, but it doesn't assert that serialize() actually ran — which is the whole point of the graceful-degradation path. Consider also spying on execute_script and asserting PercyDOM.serialize was called after the readiness exception. Otherwise this passes trivially even if the serialize call were accidentally short-circuited alongside readiness.

self.assertIn('/percy/snapshot', paths)

class TestPercyScreenshot(unittest.TestCase):
@classmethod
def setUpClass(cls):
Expand Down
Loading