Skip to content

Commit f549909

Browse files
authored
chore: check sent messages against the protocol spec in tests (#3258)
1 parent 3c7c545 commit f549909

29 files changed

Lines changed: 2948 additions & 228 deletions

‎.claude/skills/playwright-roll/SKILL.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,8 @@ The script, for `sync` and then `async`:
241241
1. runs `node scripts/codegen/generate_api.js --flavor <flavor>` (reads the docs from `$PW_SRC_DIR`) straight into `playwright/<flavor>_api/_generated.py`,
242242
2. invokes `pre-commit run --files` on the generated file.
243243

244+
It then runs `python scripts/codegen/generate_protocol.py`, which reads `packages/protocol/spec/*.yml` from `$PW_SRC_DIR` and writes the command parameter tables to `playwright/_impl/_protocol.py`.
245+
244246
Then run `mypy playwright` and work through the errors in `_generated.py` — each one is a call into `_impl/` that the implementation does not support yet.
245247

246248
To see exactly what a regeneration changed in the public API, compare against the committed file:
@@ -272,6 +274,7 @@ Failure modes and fixes:
272274
| A kwarg in `_impl/` that the docs don't mention (e.g. legacy name from a doc rename) | Nothing fails: the generator simply never passes it. | If the param is positional with no default, just rename it in `_impl/`. Check `packages/protocol/spec/*.yml` and the server dispatcher — if the wire field renamed too, also update the channel-send dict key. Only use `extraOptions` for genuinely hidden internal kwargs (`default_browser_type`). |
273275
| `AttributeError: 'X' object has no attribute '_loop'` (or `_dispatcher_fiber`) at first use of a new wrapper class | The non-ChannelOwner wrapper isn't initializing the fields the generated `AsyncBase`/`SyncBase` reads. | In the wrapper's `__init__`, set `self._loop = parent._loop` and `self._dispatcher_fiber = parent._dispatcher_fiber`. |
274276
| `'NoneType' object has no attribute 'get'` after `send_return_as_dict` | Method's protocol response carries no fields and `send_return_as_dict` returned `None`. | `(result or {}).get(...)`. |
277+
| A test fails at teardown with `Protocol violations: X.y: params.z: unknown` (or `missing`, or `expected …`) | The client sends a message that the protocol spec does not allow. The driver would drop an unknown parameter without an error. | Fix the send in `_impl/`: rename the key after a wire rename, or remove a client-side option before the send. |
275278
| Frame/buffer payload arrives as a `str` instead of `bytes` | Protocol `binary` fields cross the wire as base64. | `base64.b64decode(value)` in the impl before exposing. |
276279

277280
After the script settles, run `pre-commit run --all-files` once more to confirm everything is idle.
@@ -323,7 +326,7 @@ All keys use docs names: `Class.member` or `Class.member.arg`, with the upstream
323326
- **A cluster of rules on the same class is a smell.** If you're about to add five entries for `Foo.*`, you're almost certainly looking at a class that needs to be implemented. Implement the whole thing once and the entries disappear.
324327
- **Watch for revert pairs in the same range.** 1.59 added and reverted `Browser.isRemote` (#39613 / #39620) inside the same release. Walking chronologically lets you skip the add when you see the revert later.
325328
- **Watch for rename-revert pairs.** 1.59 had `Locator.normalize` → `Locator.toCode` (#39648) → `Locator.normalize` (#39754). Final state wins; only port the last.
326-
- **Doc renames almost always include a wire-protocol rename.** Whenever you see `### param: X.y.oldName` → `### param: X.y.newName` in a doc commit, also `git -C "$PW_SRC_DIR" show <sha> -- packages/protocol/spec/` and the corresponding `*Dispatcher.ts` file. If the wire field changed too, the channel-send dict key in `_impl/` must change. Suppressing the doc-side mismatch is hiding a real bug — the previous Python code is silently sending an unknown field that the new server ignores.
329+
- **Doc renames almost always include a wire-protocol rename.** Whenever you see `### param: X.y.oldName` → `### param: X.y.newName` in a doc commit, also `git -C "$PW_SRC_DIR" show <sha> -- packages/protocol/spec/` and the corresponding `*Dispatcher.ts` file. If the wire field changed too, the channel-send dict key in `_impl/` must change. Suppressing the doc-side mismatch is hiding a real bug — the previous Python code is silently sending an unknown field that the new server ignores. The test suite reports such a field as a protocol violation, but only in tests that send it.
327330
- **TypedDicts beat `Dict[str, X]` for any structured return.** When the docs describe a return as `[Object]` with named fields, name the struct for the generator, which writes its `TypedDict` into `_api_structures.py`, and re-export it from both public `__init__.py` files. Zero runtime cost (it's still a `dict`), and the docstrings expand its fields from the docs.
328331
- **Positional renames are free.** A param with no default before any `*` separator is positional-or-keyword in Python, but realistic call sites pass it positionally. Renaming such a param doesn't break callers.
329332
- **The "Backport changes" GitHub issue can be misleading.** In the 1.59 roll its checkboxes were all marked `[x]` with annotations like "✅ IMPLEMENTED", but several of those features had not actually been merged into the Python port. Trust the `docs/src/api/` walk over the issue.

‎.pre-commit-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ repos:
2222
rev: v1.17.0
2323
hooks:
2424
- id: mypy
25-
additional_dependencies: [types-pyOpenSSL==24.1.0.20240722]
25+
additional_dependencies: [types-pyOpenSSL==24.1.0.20240722, types-PyYAML==6.0.12.20260906]
2626
- repo: https://github.com/pycqa/flake8
2727
rev: 7.3.0
2828
hooks:

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ Python bindings for [Playwright](https://playwright.dev). The Python client talk
1212
- `playwright/async_api/_generated.py`, `playwright/sync_api/_generated.py`, and the public TypedDicts in `playwright/_impl/_api_structures.py` — **auto-generated** from the upstream API docs. Never edit by hand; rerun `./scripts/update_api.sh` after a roll or after changing `scripts/codegen/`.
1313
- `scripts/codegen/generate_api.js` — the generator. A Node.js script that loads the upstream doc parser (`utils/doclint/api_parser.js`) from `$PW_SRC_DIR`, filters the docs for Python, and writes the whole public API: signatures, types, docstrings, event overloads, the calls into `_impl/`, and the TypedDicts. It does not read `_impl/`.
1414
- `scripts/codegen/python_rules.js` — the Python facts that the docs do not contain (positional options, struct `total=`, hidden members and options). Policy: the shipped Python API wins; each entry is a candidate for an upstream docs fix, and the generator fails on entries that no longer match anything. Every entry needs a one-line rationale comment above it.
15+
- `playwright/_impl/_protocol.py` — **auto-generated** by `scripts/codegen/generate_protocol.py` (run by `./scripts/update_api.sh`) from the upstream protocol spec: the parameters of every command. With `PW_CHECK_PROTOCOL=1`, `playwright/_impl/_protocol_check.py` checks each message that the client sends against it. The test suite sets this, and fails a test that sends an unknown command, an unknown or missing parameter, or a value of the wrong type.
1516
- `scripts/codegen/compare_api.py` — compares two generated files (signatures, annotations, call bodies, docstrings) and lists every difference. Use it to check that a generator change does not change the API.
1617
- `tests/async/`, `tests/sync/` — pytest suites. Most new tests are added to the async file with a sync mirror.
1718
- `DRIVER_VERSION` — the single source of truth for which Playwright release the driver is assembled from (one line, the `playwright-core` npm version, e.g. `1.61.0`, no `v` prefix). Read by `setup.py`, `scripts/build_driver.py`, and CI. The wheel build downloads `playwright-core` at this version from npm plus the matching Node.js binary and assembles the per-platform bundles — no source build. The version is baked into the staged bundle filenames (`driver/playwright-<version>-<suffix>.zip`), so it doubles as the build cache key.

‎local-requirements.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ pytest==9.0.3
1111
pytest-asyncio==1.4.0
1212
pytest-rerunfailures==16.7
1313
pytest-timeout==2.4.0
14+
PyYAML==6.0.3
1415
service_identity==24.2.0
1516
twisted==25.5.0
1617
types-pyOpenSSL==24.1.0.20240722
18+
types-PyYAML==6.0.12.20260906

‎playwright/_impl/_browser.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,11 +275,12 @@ async def start_tracing(
275275
categories: Sequence[str] = None,
276276
) -> None:
277277
params = locals_to_params(locals())
278+
# The client writes the trace to `path` in stop_tracing().
279+
params.pop("path", None)
278280
if page:
279281
params["page"] = page._channel
280282
if path:
281283
self._cr_tracing_path = str(path)
282-
params["path"] = str(path)
283284
await self._channel.send("startTracing", None, params)
284285

285286
async def stop_tracing(self) -> bytes:

‎playwright/_impl/_connection.py‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import asyncio
1616
import collections.abc
1717
import contextvars
18-
import datetime
1918
import inspect
2019
import random
2120
import string
@@ -49,6 +48,7 @@
4948
create_task_and_ignore_exception,
5049
parse_error,
5150
)
51+
from playwright._impl._protocol_check import check_message, is_enabled
5252
from playwright._impl._transport import Transport
5353

5454
if TYPE_CHECKING:
@@ -326,6 +326,7 @@ def __init__(
326326
self._local_utils: Optional["LocalUtils"] = local_utils
327327
self._tracing_count = 0
328328
self._closed_error: Optional[Exception] = None
329+
self._check_protocol = is_enabled()
329330

330331
@property
331332
def local_utils(self) -> "LocalUtils":
@@ -427,14 +428,12 @@ def _send_message_to_server(
427428
if frames
428429
else None
429430
)
430-
metadata = {
431-
"wallTime": int(datetime.datetime.now().timestamp() * 1000),
432-
"apiName": stack_trace_information["apiName"],
431+
metadata: Dict[str, Any] = {
433432
"internal": not stack_trace_information["apiName"],
434433
"timeout": timeout,
435434
}
436435
if location:
437-
metadata["location"] = location # type: ignore
436+
metadata["location"] = location
438437
title = stack_trace_information["title"]
439438
if title:
440439
metadata["title"] = title
@@ -445,6 +444,14 @@ def _send_message_to_server(
445444
"params": self._replace_channels_with_guids(params),
446445
"metadata": metadata,
447446
}
447+
if self._check_protocol:
448+
check_message(
449+
object._type,
450+
method,
451+
message["params"],
452+
metadata,
453+
self._type_of_guid,
454+
)
448455
if self._tracing_count > 0 and frames and object._guid != "localUtils":
449456
self.local_utils.add_stack_to_tracing_no_reply(id, frames)
450457

@@ -453,6 +460,10 @@ def _send_message_to_server(
453460

454461
return callback
455462

463+
def _type_of_guid(self, guid: str) -> Optional[str]:
464+
object = self._objects.get(guid)
465+
return object._type if object else None
466+
456467
async def _abort(
457468
self, object: ChannelOwner, callback: ProtocolCallback, reason: str
458469
) -> None:

‎playwright/_impl/_frame.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,7 +650,9 @@ async def _fill(
650650
force: bool = None,
651651
title: str = None,
652652
) -> None:
653-
await self._channel.send("fill", self._timeout, locals_to_params(locals()))
653+
params = locals_to_params(locals())
654+
params.pop("title", None)
655+
await self._channel.send("fill", self._timeout, params, title=title)
654656

655657
def locator(
656658
self,
@@ -938,6 +940,7 @@ async def wait_for_function(
938940
if isinstance(polling, str) and polling != "raf":
939941
raise Error(f"Unknown polling option: {polling}")
940942
params = locals_to_params(locals())
943+
params.pop("polling", None)
941944
params["arg"] = serialize_argument(arg)
942945
if polling is not None and polling != "raf":
943946
params["pollingInterval"] = polling

‎playwright/_impl/_glob.py‎

Lines changed: 0 additions & 65 deletions
This file was deleted.

‎playwright/_impl/_helper.py‎

Lines changed: 1 addition & 131 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,12 @@
3131
Optional,
3232
Pattern,
3333
Set,
34-
Tuple,
3534
TypedDict,
3635
TypeVar,
3736
Union,
3837
cast,
3938
overload,
4039
)
41-
from urllib.parse import ParseResult, urljoin, urlparse, urlunparse
4240

4341
from playwright._impl._errors import (
4442
Error,
@@ -47,16 +45,15 @@
4745
is_target_closed_error,
4846
rewrite_error,
4947
)
50-
from playwright._impl._glob import glob_to_regex_pattern
5148
from playwright._impl._greenlets import RouteGreenlet
5249
from playwright._impl._internal_structures import NameValue
5350
from playwright._impl._str_utils import escape_regex_flags
51+
from playwright._impl._url_match import URLMatch, url_matches
5452

5553
if TYPE_CHECKING: # pragma: no cover
5654
from playwright._impl._internal_structures import HeadersArray
5755
from playwright._impl._network import Request, Response, Route, WebSocketRoute
5856

59-
URLMatch = Union[str, Pattern[str], Callable[[str], bool]]
6057
URLMatchRequest = Union[
6158
str, Pattern[str], Callable[["Request"], Union[bool, Awaitable[bool]]]
6259
]
@@ -153,133 +150,6 @@ class FrameNavigatedEvent(TypedDict):
153150
Env = Dict[str, Union[str, float, bool]]
154151

155152

156-
def url_matches(
157-
base_url: Optional[str],
158-
url_string: str,
159-
match: Optional[URLMatch],
160-
websocket_url: bool = None,
161-
) -> bool:
162-
if not match:
163-
return True
164-
if isinstance(match, str):
165-
match = re.compile(
166-
resolve_glob_to_regex_pattern(base_url, match, websocket_url)
167-
)
168-
if isinstance(match, Pattern):
169-
return bool(match.search(url_string))
170-
return match(url_string)
171-
172-
173-
def resolve_glob_to_regex_pattern(
174-
base_url: Optional[str], glob: str, websocket_url: bool = None
175-
) -> str:
176-
if websocket_url:
177-
base_url = to_websocket_base_url(base_url)
178-
glob = resolve_glob_base(base_url, glob)
179-
return glob_to_regex_pattern(glob)
180-
181-
182-
def to_websocket_base_url(base_url: Optional[str]) -> Optional[str]:
183-
if base_url is not None and re.match(r"^https?://", base_url):
184-
base_url = re.sub(r"^http", "ws", base_url)
185-
return base_url
186-
187-
188-
def resolve_glob_base(base_url: Optional[str], match: str) -> str:
189-
if match[0] == "*":
190-
return match
191-
192-
token_map: Dict[str, str] = {}
193-
194-
def map_token(original: str, replacement: str) -> str:
195-
if len(original) == 0:
196-
return ""
197-
token_map[replacement] = original
198-
return replacement
199-
200-
# Escaped `\\?` behaves the same as `?` in our glob patterns.
201-
match = match.replace(r"\\?", "?")
202-
# Special case about: URLs as they are not relative to base_url
203-
if (
204-
match.startswith("about:")
205-
or match.startswith("data:")
206-
or match.startswith("chrome:")
207-
or match.startswith("edge:")
208-
or match.startswith("file:")
209-
):
210-
# about: and data: URLs are not relative to base_url, so we return them as is.
211-
return match
212-
# Glob symbols may be escaped in the URL and some of them such as ? affect resolution,
213-
# so we replace them with safe components first.
214-
processed_parts = []
215-
for index, token in enumerate(match.split("/")):
216-
if token in (".", "..", ""):
217-
processed_parts.append(token)
218-
continue
219-
# Handle special case of http*://, note that the new schema has to be
220-
# a web schema so that slashes are properly inserted after domain.
221-
if index == 0 and token.endswith(":"):
222-
# Replace any pattern with http:
223-
if "*" in token or "{" in token:
224-
processed_parts.append(map_token(token, "http:"))
225-
else:
226-
# Preserve explicit schema as is as it may affect trailing slashes after domain.
227-
processed_parts.append(token)
228-
continue
229-
question_index = token.find("?")
230-
if question_index == -1:
231-
processed_parts.append(map_token(token, f"$_{index}_$"))
232-
else:
233-
new_prefix = map_token(token[:question_index], f"$_{index}_$")
234-
new_suffix = map_token(token[question_index:], f"?$_{index}_$")
235-
processed_parts.append(new_prefix + new_suffix)
236-
237-
relative_path = "/".join(processed_parts)
238-
resolved, case_insensitive_part = resolve_base_url(base_url, relative_path)
239-
240-
for token, original in token_map.items():
241-
normalize = case_insensitive_part and token in case_insensitive_part
242-
resolved = resolved.replace(
243-
token, original.lower() if normalize else original, 1
244-
)
245-
246-
return resolved
247-
248-
249-
def resolve_base_url(
250-
base_url: Optional[str], given_url: str
251-
) -> Tuple[str, Optional[str]]:
252-
try:
253-
url = nodelike_urlparse(
254-
urljoin(base_url if base_url is not None else "", given_url)
255-
)
256-
resolved = urlunparse(url)
257-
# Schema and domain are case-insensitive.
258-
hostname_port = (
259-
url.hostname or ""
260-
) # can't use parsed.netloc because it includes userinfo (username:password)
261-
if url.port:
262-
hostname_port += f":{url.port}"
263-
case_insensitive_prefix = f"{url.scheme}://{hostname_port}"
264-
return resolved, case_insensitive_prefix
265-
except Exception:
266-
return given_url, None
267-
268-
269-
def nodelike_urlparse(url: str) -> ParseResult:
270-
parsed = urlparse(url, allow_fragments=True)
271-
272-
# https://url.spec.whatwg.org/#special-scheme
273-
is_special_url = parsed.scheme in ["http", "https", "ws", "wss", "ftp", "file"]
274-
if is_special_url:
275-
# special urls have a list path, list paths are serialized as follows: https://url.spec.whatwg.org/#url-path-serializer
276-
# urllib diverges, so we patch it here
277-
if parsed.path == "":
278-
parsed = parsed._replace(path="/")
279-
280-
return parsed
281-
282-
283153
class HarLookupResult(TypedDict, total=False):
284154
action: Literal["error", "redirect", "fulfill", "noentry"]
285155
message: Optional[str]

‎playwright/_impl/_input.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ async def _click(
7070
clickCount: int = None,
7171
title: str = None,
7272
) -> None:
73-
await self._channel.send(
74-
"mouseClick", None, locals_to_params(locals()), title=title
75-
)
73+
params = locals_to_params(locals())
74+
params.pop("title", None)
75+
await self._channel.send("mouseClick", None, params, title=title)
7676

7777
async def click(
7878
self,

0 commit comments

Comments
 (0)