diff --git a/.python-version b/.python-version index 7d4ef04..c8cfe39 100644 --- a/.python-version +++ b/.python-version @@ -1 +1 @@ -3.10.3 +3.10 diff --git a/package.json b/package.json index cb9e567..90b0476 100644 --- a/package.json +++ b/package.json @@ -4,6 +4,6 @@ "test": "make test" }, "devDependencies": { - "@percy/cli": "1.30.9" + "@percy/cli": "^1.31.14" } } diff --git a/percy/snapshot.py b/percy/snapshot.py index 746bd3b..5c75387 100644 --- a/percy/snapshot.py +++ b/percy/snapshot.py @@ -191,9 +191,127 @@ def _get_origin(url): parsed = urlparse(url) return f"{parsed.scheme}://{parsed.netloc}" -def get_serialized_dom(driver, cookies, percy_dom_script=None, **kwargs): +def _resolve_readiness_config(percy_config, kwargs): + """Shallow-merge global (.percy.yml) readiness config with per-snapshot + override. Per-snapshot keys win; unspecified keys are inherited. + + Defensive against `config.snapshot` being None or non-dict — the CLI is + free to evolve its healthcheck payload and `None` should degrade to `{}`, + not raise AttributeError mid-snapshot.""" + config = percy_config or {} + global_readiness = ((config.get('snapshot') or {}).get('readiness')) or {} + per_snapshot = kwargs.get('readiness') or {} + if not isinstance(global_readiness, dict): + global_readiness = {} + if not isinstance(per_snapshot, dict): + per_snapshot = {} + return {**global_readiness, **per_snapshot} + + +def _wait_for_ready(driver, percy_config, kwargs): + """Run readiness checks before serialize. + + 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 (or None) so callers can attach it + to the domSnapshot for CLI-side logging. + + Config precedence: per-snapshot `kwargs['readiness']` shallow-merged + over global `percy_config.snapshot.readiness`; per-snapshot keys win, + unspecified keys (e.g. a global `preset: disabled` kill switch) are + inherited. Skips entirely when the merged preset is 'disabled'. + + The caller must pass `percy_config` explicitly (from the `is_percy_enabled()` + payload they already have in scope) — we don't re-call the cached lookup + here, both for clarity and to avoid surprise dependencies on the cache. + """ + has_explicit_kwarg = 'readiness' in kwargs + has_global_config = bool( + (percy_config or {}).get('snapshot', {}).get('readiness') + if isinstance(percy_config, dict) else False) + if not has_explicit_kwarg and not has_global_config: + return None + readiness_config = _resolve_readiness_config(percy_config, kwargs) + if readiness_config.get('preset') == 'disabled': + return None + # Match readiness.timeoutMs to the driver's async-script timeout so a + # higher user-configured readiness timeout isn't silently capped by + # WebDriver's default (~30s on Selenium 4, lower on some remotes). + timeout_ms = readiness_config.get('timeoutMs') + previous_timeout = None + if isinstance(timeout_ms, (int, float)) and timeout_ms > 0: + try: + # Selenium 4 exposes driver.timeouts.script (float seconds). + previous_timeout = getattr(driver.timeouts, 'script', None) + driver.set_script_timeout(timeout_ms / 1000 + 2) # +2s buffer + except Exception: + previous_timeout = None # older Selenium / unsupported — best effort + # JS-side hard timeout: geckodriver does not reliably honor selenium's + # script_timeout for async scripts whose pending work lives in microtasks + # (Promise.then chains), so tests can hang indefinitely. Wrap done() in + # a once-only guard and arm a setTimeout that calls it after the + # readiness deadline + 2s buffer, regardless of what waitForReady does. + deadline_ms = int((timeout_ms if isinstance(timeout_ms, (int, float)) and timeout_ms > 0 + else 10000) + 2000) + try: + # done() must be called ASYNCHRONOUSLY for execute_async_script to + # unblock — calling it synchronously within the script's body has + # historically hung geckodriver in CI for hours. fireDone() wraps + # done() in setTimeout(_, 0) so every code path defers the callback + # to the next event-loop tick. + diagnostics = driver.execute_async_script( + 'var config = ' + json.dumps(readiness_config) + ';' + 'var done = arguments[arguments.length - 1];' + 'var doneFired = false;' + 'function fireDone(v) {' + ' if (doneFired) return;' + ' doneFired = true;' + ' setTimeout(function() { done(v); }, 0);' + '}' + 'setTimeout(function() { fireDone(); }, ' + str(deadline_ms) + ');' + 'try {' + " if (typeof PercyDOM !== 'undefined'" + " && typeof PercyDOM.waitForReady === 'function') {" + ' PercyDOM.waitForReady(config)' + ' .then(function(r){ fireDone(r); })' + ' .catch(function(){ fireDone(); });' + ' } else { fireDone(); }' + '} catch(e) { fireDone(); }' + ) + return diagnostics + except Exception as e: + log(f'waitForReady failed, proceeding to serialize: {e}', 'debug') + return None + finally: + if previous_timeout is not None: + try: + driver.set_script_timeout(previous_timeout) + except Exception: + pass + + +def get_serialized_dom(driver, cookies, percy_config=None, percy_dom_script=None, + skip_readiness=False, readiness_diagnostics=None, **kwargs): + # 0. Readiness gate before serialize. Graceful on old CLI. + # `skip_readiness` lets responsive capture run readiness once before the + # width loop and pass diagnostics through, instead of paying the cost + # per width. + if not skip_readiness: + readiness_diagnostics = _wait_for_ready(driver, percy_config, kwargs) + # Strip `readiness` from kwargs before forwarding — it's an SDK-local + # concern; the CLI already has it from healthcheck and a top-level + # `readiness` in the POST body is brittle against future validators. + kwargs.pop('readiness', None) # 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. + # `is not None` preserves legitimate falsy returns (e.g. `{}` meaning + # "gate ran, no notable diagnostics"). + if readiness_diagnostics is not None and isinstance(dom_snapshot, dict): + dom_snapshot['readiness_diagnostics'] = readiness_diagnostics # 2. Process CORS IFrames try: page_origin = _get_origin(driver.current_url) @@ -307,6 +425,12 @@ def capture_responsive_dom(driver, cookies, config, percy_dom_script=None, **kwa resize_count = 0 # Initialize resize listener once before the loop driver.execute_script("PercyDOM.waitForResize()") + # Run readiness ONCE before the per-width loop. With N widths and a + # `timeoutMs` of e.g. 10s, running readiness per width can cost up to + # N*timeout seconds of sequential waits — almost always undesirable. + # Per-width DOM mutations after viewport changes are handled by the + # `waitForResize` instrumentation above, not by re-running readiness. + responsive_readiness_diagnostics = _wait_for_ready(driver, config, kwargs) target_height = current_height if PERCY_RESPONSIVE_CAPTURE_MIN_HEIGHT: @@ -339,7 +463,11 @@ def capture_responsive_dom(driver, cookies, config, percy_dom_script=None, **kwa print(f'{width}x{height} ready, taking snapshot...') _responsive_sleep() dom_snapshot = get_serialized_dom( - driver, cookies, percy_dom_script=percy_dom_script, **kwargs) + driver, cookies, percy_config=config, + percy_dom_script=percy_dom_script, + skip_readiness=True, + readiness_diagnostics=responsive_readiness_diagnostics, + **kwargs) dom_snapshot['width'] = width print(f'Taken snapshot for width: {width}, height: {height}') dom_snapshots.append(dom_snapshot) @@ -383,10 +511,15 @@ def percy_snapshot(driver, name, **kwargs): ) else: dom_snapshot = get_serialized_dom( - driver, cookies, percy_dom_script=percy_dom_script, **kwargs) + driver, cookies, percy_config=data.get('config'), + percy_dom_script=percy_dom_script, **kwargs) + # Strip SDK-local `readiness` from the snapshot POST body. The CLI + # already has it via healthcheck; sending it again here risks future + # CLI-side validators rejecting unknown top-level fields. + post_kwargs = {k: v for k, v in kwargs.items() if k != 'readiness'} # Post the DOM to the snapshot endpoint with snapshot options and other info - response = requests.post(f'{PERCY_CLI_API}/percy/snapshot', json={**kwargs, **{ + response = requests.post(f'{PERCY_CLI_API}/percy/snapshot', json={**post_kwargs, **{ 'client_info': CLIENT_INFO, 'environment_info': ENV_INFO, 'dom_snapshot': dom_snapshot, diff --git a/tests/test_snapshot.py b/tests/test_snapshot.py index 9419138..f5800e7 100644 --- a/tests/test_snapshot.py +++ b/tests/test_snapshot.py @@ -13,7 +13,12 @@ from selenium.webdriver.remote.webelement import WebElement from selenium.webdriver.remote.remote_connection import RemoteConnection from selenium.webdriver.safari.remote_connection import SafariRemoteConnection -from percy.snapshot import create_region +from percy.snapshot import ( + create_region, + _resolve_readiness_config, + _wait_for_ready, + get_serialized_dom, +) from percy import percy_snapshot, percySnapshot, percy_screenshot import percy.snapshot as local @@ -119,6 +124,7 @@ def mock_screenshot(fail=False, data=False): }), status=(500 if fail else 200)) +# pylint: disable=too-many-public-methods class TestPercySnapshot(unittest.TestCase): @classmethod def setUpClass(cls): @@ -472,6 +478,143 @@ 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)) + +class TestReadinessGate(unittest.TestCase): + """Unit tests for _wait_for_ready / _resolve_readiness_config using a + fully-mocked WebDriver. Bypasses real geckodriver/Firefox traffic, so + cannot hang on real in-page observers like the integration-style tests + did.""" + + def test_resolve_readiness_config_shallow_merges(self): + merged = _resolve_readiness_config( + {'snapshot': {'readiness': {'preset': 'balanced', 'timeoutMs': 8000}}}, + {'readiness': {'stabilityWindowMs': 500}} + ) + self.assertEqual(merged, { + 'preset': 'balanced', 'timeoutMs': 8000, 'stabilityWindowMs': 500 + }) + + def test_resolve_readiness_config_per_snapshot_wins(self): + merged = _resolve_readiness_config( + {'snapshot': {'readiness': {'preset': 'balanced'}}}, + {'readiness': {'preset': 'strict'}} + ) + self.assertEqual(merged['preset'], 'strict') + + def test_resolve_readiness_config_handles_none_snapshot(self): + merged = _resolve_readiness_config({'snapshot': None}, {}) + self.assertEqual(merged, {}) + + def test_resolve_readiness_config_handles_non_dict_inputs(self): + merged = _resolve_readiness_config( + {'snapshot': {'readiness': 'not-a-dict'}}, + {'readiness': 12345} + ) + self.assertEqual(merged, {}) + + def test_wait_for_ready_opt_in_skips_when_no_config(self): + driver = Mock() + result = _wait_for_ready(driver, percy_config={}, kwargs={}) + self.assertIsNone(result) + driver.execute_async_script.assert_not_called() + + def test_wait_for_ready_runs_when_kwargs_opt_in(self): + diagnostics = {'passed': True, 'preset': 'balanced'} + driver = Mock() + driver.execute_async_script.return_value = diagnostics + driver.timeouts.script = 30 + + result = _wait_for_ready(driver, percy_config={}, kwargs={'readiness': {}}) + + self.assertEqual(result, diagnostics) + self.assertEqual(driver.execute_async_script.call_count, 1) + script = driver.execute_async_script.call_args.args[0] + self.assertIn('PercyDOM.waitForReady', script) + self.assertIn('typeof PercyDOM', script) + + def test_wait_for_ready_runs_when_global_config_opts_in(self): + driver = Mock() + driver.execute_async_script.return_value = None + driver.timeouts.script = 30 + percy_config = {'snapshot': {'readiness': {'preset': 'balanced'}}} + + _wait_for_ready(driver, percy_config=percy_config, kwargs={}) + + self.assertEqual(driver.execute_async_script.call_count, 1) + + def test_wait_for_ready_skips_disabled_preset(self): + driver = Mock() + result = _wait_for_ready( + driver, percy_config={}, kwargs={'readiness': {'preset': 'disabled'}}) + self.assertIsNone(result) + driver.execute_async_script.assert_not_called() + + def test_wait_for_ready_inlines_per_snapshot_config_into_script(self): + driver = Mock() + driver.execute_async_script.return_value = None + driver.timeouts.script = 30 + cfg = {'preset': 'strict', 'stabilityWindowMs': 500} + + _wait_for_ready(driver, percy_config={}, kwargs={'readiness': cfg}) + + script = driver.execute_async_script.call_args.args[0] + self.assertIn('"preset": "strict"', script) + self.assertIn('"stabilityWindowMs": 500', script) + + def test_wait_for_ready_sets_and_restores_script_timeout(self): + driver = Mock() + driver.execute_async_script.return_value = None + driver.timeouts.script = 30 # selenium 4 default (seconds) + + _wait_for_ready( + driver, percy_config={}, + kwargs={'readiness': {'timeoutMs': 5000}}) + + # set to readiness.timeoutMs/1000 + 2s buffer, then restored + driver.set_script_timeout.assert_any_call(7) # 5000/1000 + 2 + driver.set_script_timeout.assert_any_call(30) # restored + + def test_wait_for_ready_swallows_exception_and_returns_none(self): + driver = Mock() + driver.execute_async_script.side_effect = RuntimeError('boom') + + with patch('percy.snapshot.log') as mock_log: + result = _wait_for_ready(driver, percy_config={}, + kwargs={'readiness': {}}) + + self.assertIsNone(result) + mock_log.assert_called_once() + self.assertIn('waitForReady failed', mock_log.call_args.args[0]) + + def test_get_serialized_dom_pops_readiness_from_serialize_call(self): + """`readiness` is SDK-local; PercyDOM.serialize must not see it.""" + driver = Mock() + driver.execute_script.return_value = {'html': ''} + driver.execute_async_script.return_value = None + driver.current_url = 'http://localhost:8000/' + driver.get_cookies.return_value = [] + + get_serialized_dom(driver, cookies=[], percy_config={}, + readiness={'preset': 'balanced'}) + + serialize_call = next( + c for c in driver.execute_script.call_args_list + if 'PercyDOM.serialize' in c.args[0] + ) + self.assertNotIn('readiness', serialize_call.args[0]) + + def test_get_serialized_dom_attaches_diagnostics(self): + driver = Mock() + driver.execute_script.return_value = {'html': ''} + driver.execute_async_script.return_value = {'passed': True} + driver.current_url = 'http://localhost:8000/' + driver.get_cookies.return_value = [] + + dom_snapshot = get_serialized_dom( + driver, cookies=[], percy_config={}, readiness={'preset': 'balanced'}) + + self.assertEqual(dom_snapshot['readiness_diagnostics'], {'passed': True}) + class TestPercyScreenshot(unittest.TestCase): @classmethod def setUpClass(cls):