Repository navigation
test: port the upstream tests by registry id - #3269
Pavel Feldman (pavelfeldman) wants to merge 1 commit into
Conversation
7453f2c to
0805ec6
Compare
Port every test in upstream's tests/registry.md to tests/async and
tests/sync, named after its id and title, or list it in
tests/upstream_not_ported.txt with the reason.
scripts/check_test_registry.py checks this in CI. The older tests move
to tests/{async,sync}/legacy.
Fix the client bugs that the ports found, so that the same tests pass
as with Node.js, and roll the driver to 1.65.0-alpha-2026-10-17.
CI runs the tests in parallel with pytest-xdist, on fewer bots.
test.sh runs the library tests under xvfb.
0805ec6 to
786d689
Compare
| artifacts_dir: Optional[Union[str, Path]], | ||
| ) -> Browser: | ||
| # Port of BrowserType.connectOverCDP() in client/browserType.ts. | ||
| if self.name != "chromium" and self.name != "webkit": |
There was a problem hiding this comment.
Overall, I'd prefer checks like this on the server side.
| headers: Optional[Dict[str, str]], | ||
| expose_network: Optional[str], | ||
| ) -> Browser: | ||
| # Like @apiCall() in client/browserType.ts: the errors that the client |
There was a problem hiding this comment.
I wonder why not use the actual @apiCall?
| return_when=asyncio.FIRST_COMPLETED, | ||
| ) | ||
| def close_connection() -> None: | ||
| # Port of connection.close() in client/connection.ts, which also closes |
There was a problem hiding this comment.
There are quite a few useless comments 😄
| _PLAYWRIGHT_MODULE_PATH = os.path.join(str(Path(playwright.__file__).parents[0]), "") | ||
|
|
||
|
|
||
| def _non_user_code_dirs() -> Tuple[str, ...]: |
There was a problem hiding this comment.
I think this should be a separate PR, it is a behavior change. Also, stack parsing in python historically was a performance-sensitive place.
| raise Error(error) | ||
| failed: asyncio.Future = self._loop.create_future() | ||
| failed.set_exception(Error(error)) | ||
| await waiter.wait_for_future(failed) |
There was a problem hiding this comment.
Why is "raise Error" not enough here?
| _MAX_IDLE_YIELDS = 100 | ||
|
|
||
|
|
||
| async def _yield_until_idle() -> None: |
There was a problem hiding this comment.
This one is worth a separate PR for sure!
|
|
||
| def _json_stringify(value: str) -> str: | ||
| # JSON.stringify() of a string. | ||
| return json.dumps(value, ensure_ascii=False) |
There was a problem hiding this comment.
Why is "no ascii" important here? Why not use a shared one from 'helper'?
| return resolved, case_insensitive_prefix | ||
| except Exception: | ||
| # new URL(givenURL, baseURL); None where it throws. | ||
| url = parse_url(given_url, base_url) |
There was a problem hiding this comment.
This place has a loaded history, most likely will lead to regressions.
| @@ -26,32 +28,48 @@ | |||
|
|
|||
|
|
|||
| class Waiter: | |||
There was a problem hiding this comment.
It is unclear what's going on in this file. Perhaps it fixes some tests, but there is no way to know which ones.
| _DEVIATION_CHARACTERS = re.compile("(" + "|".join(map(chr, (0xDF, 0x3C2))) + ")") | ||
|
|
||
|
|
||
| class URL: |
There was a problem hiding this comment.
Not sure what we are trying to achieve here. Again, seeing some tests will at least hint at what's going on.
2 participants
Summary
tests/registry.mdtotests/asyncandtests/sync, namedtest_<ID>_<title>, or list it intests/upstream_not_ported.txtwith the reason (3699 ported, 1234 not ported).scripts/check_test_registry.pychecks the coverage; CI runs it in the lint job.tests/assetswith upstream; extend the test server, fixtures and helpers that the ported tests need.reason="python: ...").tests/{async,sync}/legacy.