From 2018e49039f0ecf73aa82b9a7780db1cd54bb75b Mon Sep 17 00:00:00 2001 From: Eli Black Date: Tue, 29 Apr 2025 15:20:56 +0800 Subject: [PATCH 1/9] Speed up connection by not asking for context when inspecting stack --- playwright/_impl/_connection.py | 7 +++++-- tests/async/test_asyncio.py | 13 +++++++++++++ tests/sync/test_sync.py | 13 +++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 027daf69d..3dd5df633 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -365,7 +365,6 @@ def _send_message_to_server( if ( self._tracing_count > 0 and frames - and frames and object._guid != "localUtils" ): self.local_utils.add_stack_to_tracing_no_reply(id, frames) @@ -519,7 +518,11 @@ async def wrap_api_call( if self._api_zone.get(): return await cb() task = asyncio.current_task(self._loop) - st: List[inspect.FrameInfo] = getattr(task, "__pw_stack__", inspect.stack()) + st: List[inspect.FrameInfo] = getattr(task, "__pw_stack__", None) + + if st == None: + st = inspect.stack(0) + parsed_st = _extract_stack_trace_information_from_stack(st, is_internal) self._api_zone.set(parsed_st) try: diff --git a/tests/async/test_asyncio.py b/tests/async/test_asyncio.py index 33edc71ce..e7ed72d9e 100644 --- a/tests/async/test_asyncio.py +++ b/tests/async/test_asyncio.py @@ -87,3 +87,16 @@ async def raise_exception() -> None: assert "Something went wrong" in str(exc_info.value.exceptions[0]) assert isinstance(exc_info.value.exceptions[0], ValueError) assert await page.evaluate("() => 11 * 11") == 121 + +async def test_should_return_proper_api_name_on_error(page: Page) -> None: + try: + await page.evaluate("does_not_exist") + + assert False, "Accessing undefined JavaScript variable should have thrown exception" + except Exception as error: + print(':-:' + error.message + ':-:') + assert error.message == """Page.evaluate: ReferenceError: does_not_exist is not defined + at eval (eval at evaluate (:234:30), :1:1) + at eval () + at UtilityScript.evaluate (:234:30) + at UtilityScript. (:1:44)""" \ No newline at end of file diff --git a/tests/sync/test_sync.py b/tests/sync/test_sync.py index 64eace1e9..c673efad0 100644 --- a/tests/sync/test_sync.py +++ b/tests/sync/test_sync.py @@ -346,3 +346,16 @@ def test_call_sync_method_after_playwright_close_with_own_loop( p.start() p.join() assert p.exitcode == 0 + +def test_should_return_proper_api_name_on_error(page: Page) -> None: + try: + page.evaluate("does_not_exist") + + assert False, "Accessing undefined JavaScript variable should have thrown exception" + except Exception as error: + print(':-:' + error.message + ':-:') + assert error.message == """Page.evaluate: ReferenceError: does_not_exist is not defined + at eval (eval at evaluate (:234:30), :1:1) + at eval () + at UtilityScript.evaluate (:234:30) + at UtilityScript. (:1:44)""" \ No newline at end of file From 5a92f4f5713c985daa0f9cd3ea46a9431208bee4 Mon Sep 17 00:00:00 2001 From: Eli Black Date: Tue, 29 Apr 2025 15:30:56 +0800 Subject: [PATCH 2/9] Formatting changes from the pre-commit hook --- playwright/_impl/_connection.py | 13 ++++--------- tests/async/test_asyncio.py | 14 ++++++++++---- tests/sync/test_sync.py | 14 ++++++++++---- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 3dd5df633..647414295 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -362,11 +362,7 @@ def _send_message_to_server( "params": self._replace_channels_with_guids(params), "metadata": metadata, } - if ( - self._tracing_count > 0 - and frames - and object._guid != "localUtils" - ): + if self._tracing_count > 0 and frames and object._guid != "localUtils": self.local_utils.add_stack_to_tracing_no_reply(id, frames) self._transport.send(message) @@ -518,10 +514,9 @@ async def wrap_api_call( if self._api_zone.get(): return await cb() task = asyncio.current_task(self._loop) - st: List[inspect.FrameInfo] = getattr(task, "__pw_stack__", None) - - if st == None: - st = inspect.stack(0) + st: List[inspect.FrameInfo] = getattr( + task, "__pw_stack__", None + ) or inspect.stack(0) parsed_st = _extract_stack_trace_information_from_stack(st, is_internal) self._api_zone.set(parsed_st) diff --git a/tests/async/test_asyncio.py b/tests/async/test_asyncio.py index e7ed72d9e..1e4857ad5 100644 --- a/tests/async/test_asyncio.py +++ b/tests/async/test_asyncio.py @@ -88,15 +88,21 @@ async def raise_exception() -> None: assert isinstance(exc_info.value.exceptions[0], ValueError) assert await page.evaluate("() => 11 * 11") == 121 + async def test_should_return_proper_api_name_on_error(page: Page) -> None: try: await page.evaluate("does_not_exist") - assert False, "Accessing undefined JavaScript variable should have thrown exception" + assert ( + False + ), "Accessing undefined JavaScript variable should have thrown exception" except Exception as error: - print(':-:' + error.message + ':-:') - assert error.message == """Page.evaluate: ReferenceError: does_not_exist is not defined + print(":-:" + error.message + ":-:") + assert ( + error.message + == """Page.evaluate: ReferenceError: does_not_exist is not defined at eval (eval at evaluate (:234:30), :1:1) at eval () at UtilityScript.evaluate (:234:30) - at UtilityScript. (:1:44)""" \ No newline at end of file + at UtilityScript. (:1:44)""" + ) diff --git a/tests/sync/test_sync.py b/tests/sync/test_sync.py index c673efad0..d0f630f67 100644 --- a/tests/sync/test_sync.py +++ b/tests/sync/test_sync.py @@ -347,15 +347,21 @@ def test_call_sync_method_after_playwright_close_with_own_loop( p.join() assert p.exitcode == 0 + def test_should_return_proper_api_name_on_error(page: Page) -> None: try: page.evaluate("does_not_exist") - assert False, "Accessing undefined JavaScript variable should have thrown exception" + assert ( + False + ), "Accessing undefined JavaScript variable should have thrown exception" except Exception as error: - print(':-:' + error.message + ':-:') - assert error.message == """Page.evaluate: ReferenceError: does_not_exist is not defined + print(":-:" + error.message + ":-:") + assert ( + error.message + == """Page.evaluate: ReferenceError: does_not_exist is not defined at eval (eval at evaluate (:234:30), :1:1) at eval () at UtilityScript.evaluate (:234:30) - at UtilityScript. (:1:44)""" \ No newline at end of file + at UtilityScript. (:1:44)""" + ) From e973932c0099f5e65d462d6619edfc17d1e87cd5 Mon Sep 17 00:00:00 2001 From: Eli Black Date: Tue, 29 Apr 2025 16:06:56 +0800 Subject: [PATCH 3/9] Also use inspect.stack(0) for the sync version of _connect --- playwright/_impl/_connection.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 647414295..2d1dad933 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -533,7 +533,9 @@ def wrap_api_call_sync( if self._api_zone.get(): return cb() task = asyncio.current_task(self._loop) - st: List[inspect.FrameInfo] = getattr(task, "__pw_stack__", inspect.stack()) + st: List[inspect.FrameInfo] = getattr( + task, "__pw_stack__", None + ) or inspect.stack(0) parsed_st = _extract_stack_trace_information_from_stack(st, is_internal) self._api_zone.set(parsed_st) try: From ebddce42e287f5142948cef37e02fcb093140a57 Mon Sep 17 00:00:00 2001 From: Eli Black Date: Tue, 29 Apr 2025 17:24:16 +0800 Subject: [PATCH 4/9] Fix pre-commit warnings --- tests/async/test_asyncio.py | 3 +-- tests/sync/test_sync.py | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/async/test_asyncio.py b/tests/async/test_asyncio.py index 1e4857ad5..5d878157d 100644 --- a/tests/async/test_asyncio.py +++ b/tests/async/test_asyncio.py @@ -97,9 +97,8 @@ async def test_should_return_proper_api_name_on_error(page: Page) -> None: False ), "Accessing undefined JavaScript variable should have thrown exception" except Exception as error: - print(":-:" + error.message + ":-:") assert ( - error.message + str(error) == """Page.evaluate: ReferenceError: does_not_exist is not defined at eval (eval at evaluate (:234:30), :1:1) at eval () diff --git a/tests/sync/test_sync.py b/tests/sync/test_sync.py index d0f630f67..de9514634 100644 --- a/tests/sync/test_sync.py +++ b/tests/sync/test_sync.py @@ -356,9 +356,8 @@ def test_should_return_proper_api_name_on_error(page: Page) -> None: False ), "Accessing undefined JavaScript variable should have thrown exception" except Exception as error: - print(":-:" + error.message + ":-:") assert ( - error.message + str(error) == """Page.evaluate: ReferenceError: does_not_exist is not defined at eval (eval at evaluate (:234:30), :1:1) at eval () From 8dd96fe68af20629a71c127c5ac2a9aa1743cc6d Mon Sep 17 00:00:00 2001 From: Eli Black Date: Tue, 29 Apr 2025 23:16:50 +0800 Subject: [PATCH 5/9] Further improve the speed of retrieving stacktraces, by walking the stack manually --- playwright/_impl/_connection.py | 56 ++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 2d1dad933..c9af704f9 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -19,12 +19,14 @@ import inspect import sys import traceback +from inspect import FrameInfo from pathlib import Path from typing import ( TYPE_CHECKING, Any, Callable, Dict, + Generator, List, Mapping, Optional, @@ -362,7 +364,7 @@ def _send_message_to_server( "params": self._replace_channels_with_guids(params), "metadata": metadata, } - if self._tracing_count > 0 and frames and object._guid != "localUtils": + if self.needs_full_stack_trace() and frames and object._guid != "localUtils": self.local_utils.add_stack_to_tracing_no_reply(id, frames) self._transport.send(message) @@ -508,17 +510,42 @@ def _replace_guids_with_channels(self, payload: Any) -> Any: return result return payload + def needs_full_stack_trace(self) -> bool: + return self._tracing_count > 0 + + def get_frame_info(self) -> Generator[FrameInfo]: + current_frame = inspect.currentframe() + + while current_frame: + traceback_info = inspect.getframeinfo(current_frame, 0) + # TODO: Used to be *frameinfo, but I couldn't figure out how to get the type checking to work correctly + # frameinfo = (current_frame,) + traceback_info + # yield FrameInfo(*frameinfo) + # yield FrameInfo(cast(inspect.FrameType, *frameinfo)) + yield FrameInfo( + current_frame, + traceback_info.filename, + traceback_info.lineno, + traceback_info.function, + traceback_info.code_context, + traceback_info.index, + # positions=current_frame.positions, + ) + + current_frame = current_frame.f_back + async def wrap_api_call( self, cb: Callable[[], Any], is_internal: bool = False ) -> Any: if self._api_zone.get(): return await cb() task = asyncio.current_task(self._loop) - st: List[inspect.FrameInfo] = getattr( - task, "__pw_stack__", None - ) or inspect.stack(0) - - parsed_st = _extract_stack_trace_information_from_stack(st, is_internal) + st: Union[List[FrameInfo], Generator[FrameInfo]] = ( + getattr(task, "__pw_stack__", None) or self.get_frame_info() + ) + parsed_st = _extract_stack_trace_information_from_stack( + st, is_internal, self.needs_full_stack_trace() + ) self._api_zone.set(parsed_st) try: return await cb() @@ -533,10 +560,12 @@ def wrap_api_call_sync( if self._api_zone.get(): return cb() task = asyncio.current_task(self._loop) - st: List[inspect.FrameInfo] = getattr( - task, "__pw_stack__", None - ) or inspect.stack(0) - parsed_st = _extract_stack_trace_information_from_stack(st, is_internal) + st: Union[List[FrameInfo], Generator[FrameInfo]] = ( + getattr(task, "__pw_stack__", None) or self.get_frame_info() + ) + parsed_st = _extract_stack_trace_information_from_stack( + st, is_internal, self.needs_full_stack_trace() + ) self._api_zone.set(parsed_st) try: return cb() @@ -567,7 +596,9 @@ class ParsedStackTrace(TypedDict): def _extract_stack_trace_information_from_stack( - st: List[inspect.FrameInfo], is_internal: bool + st: Union[List[FrameInfo], Generator[FrameInfo]], + is_internal: bool, + needs_full_stack: bool, ) -> ParsedStackTrace: playwright_module_path = str(Path(playwright.__file__).parents[0]) last_internal_api_name = "" @@ -596,6 +627,9 @@ def _extract_stack_trace_information_from_stack( "function": method_name, } ) + + if not needs_full_stack: + break if is_playwright_internal: last_internal_api_name = method_name elif last_internal_api_name: From 5ac20ef84f3f6577ce19de5ed734da8dc701a84f Mon Sep 17 00:00:00 2001 From: Eli Black Date: Tue, 29 Apr 2025 23:30:30 +0800 Subject: [PATCH 6/9] Simplify test cases, so that they pass on all browsers --- tests/async/test_asyncio.py | 10 ++-------- tests/sync/test_sync.py | 10 ++-------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/tests/async/test_asyncio.py b/tests/async/test_asyncio.py index 5d878157d..971c65473 100644 --- a/tests/async/test_asyncio.py +++ b/tests/async/test_asyncio.py @@ -97,11 +97,5 @@ async def test_should_return_proper_api_name_on_error(page: Page) -> None: False ), "Accessing undefined JavaScript variable should have thrown exception" except Exception as error: - assert ( - str(error) - == """Page.evaluate: ReferenceError: does_not_exist is not defined - at eval (eval at evaluate (:234:30), :1:1) - at eval () - at UtilityScript.evaluate (:234:30) - at UtilityScript. (:1:44)""" - ) + # Each browser returns slightly different error messages, but they should all start with "Page.evaluate:", because that was the Playwright method where the error originated + assert str(error).startswith("Page.evaluate:") diff --git a/tests/sync/test_sync.py b/tests/sync/test_sync.py index de9514634..92d40c19a 100644 --- a/tests/sync/test_sync.py +++ b/tests/sync/test_sync.py @@ -356,11 +356,5 @@ def test_should_return_proper_api_name_on_error(page: Page) -> None: False ), "Accessing undefined JavaScript variable should have thrown exception" except Exception as error: - assert ( - str(error) - == """Page.evaluate: ReferenceError: does_not_exist is not defined - at eval (eval at evaluate (:234:30), :1:1) - at eval () - at UtilityScript.evaluate (:234:30) - at UtilityScript. (:1:44)""" - ) + # Each browser returns slightly different error messages, but they should all start with "Page.evaluate:", because that was the Playwright method where the error originated + assert str(error).startswith("Page.evaluate:") From 20c0c6bae1a6f5def8fa3b0a3228288bf0413515 Mon Sep 17 00:00:00 2001 From: Eli Black Date: Wed, 30 Apr 2025 14:28:28 +0800 Subject: [PATCH 7/9] Omit get_frame_info() from the stack trace --- playwright/_impl/_connection.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index c9af704f9..b5fac4de5 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -516,12 +516,15 @@ def needs_full_stack_trace(self) -> bool: def get_frame_info(self) -> Generator[FrameInfo]: current_frame = inspect.currentframe() + if current_frame is None: + return + + # Don't include the current method ("get_frame_info()") in the callstack + current_frame = current_frame.f_back + while current_frame: traceback_info = inspect.getframeinfo(current_frame, 0) - # TODO: Used to be *frameinfo, but I couldn't figure out how to get the type checking to work correctly - # frameinfo = (current_frame,) + traceback_info - # yield FrameInfo(*frameinfo) - # yield FrameInfo(cast(inspect.FrameType, *frameinfo)) + yield FrameInfo( current_frame, traceback_info.filename, @@ -529,7 +532,6 @@ def get_frame_info(self) -> Generator[FrameInfo]: traceback_info.function, traceback_info.code_context, traceback_info.index, - # positions=current_frame.positions, ) current_frame = current_frame.f_back From 411a8418707c685964b0942b299cc48f071a65d6 Mon Sep 17 00:00:00 2001 From: Eli Black Date: Fri, 2 May 2025 15:45:35 +0800 Subject: [PATCH 8/9] Fuller type for `Generator` --- playwright/_impl/_connection.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index b5fac4de5..3ec655d76 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -513,7 +513,7 @@ def _replace_guids_with_channels(self, payload: Any) -> Any: def needs_full_stack_trace(self) -> bool: return self._tracing_count > 0 - def get_frame_info(self) -> Generator[FrameInfo]: + def get_frame_info(self) -> Generator[FrameInfo, None]: current_frame = inspect.currentframe() if current_frame is None: @@ -542,7 +542,7 @@ async def wrap_api_call( if self._api_zone.get(): return await cb() task = asyncio.current_task(self._loop) - st: Union[List[FrameInfo], Generator[FrameInfo]] = ( + st: Union[List[FrameInfo], Generator[FrameInfo, None]] = ( getattr(task, "__pw_stack__", None) or self.get_frame_info() ) parsed_st = _extract_stack_trace_information_from_stack( @@ -562,7 +562,7 @@ def wrap_api_call_sync( if self._api_zone.get(): return cb() task = asyncio.current_task(self._loop) - st: Union[List[FrameInfo], Generator[FrameInfo]] = ( + st: Union[List[FrameInfo], Generator[FrameInfo, None]] = ( getattr(task, "__pw_stack__", None) or self.get_frame_info() ) parsed_st = _extract_stack_trace_information_from_stack( @@ -598,7 +598,7 @@ class ParsedStackTrace(TypedDict): def _extract_stack_trace_information_from_stack( - st: Union[List[FrameInfo], Generator[FrameInfo]], + st: Union[List[FrameInfo], Generator[FrameInfo, None]], is_internal: bool, needs_full_stack: bool, ) -> ParsedStackTrace: From 3a97f3f7cdf1b53ada0cdeba025bcccecb17946f Mon Sep 17 00:00:00 2001 From: Eli Black Date: Sat, 3 May 2025 18:46:31 +0800 Subject: [PATCH 9/9] Use simpler type for Generator --- playwright/_impl/_connection.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 3ec655d76..501c18e2a 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -26,7 +26,7 @@ Any, Callable, Dict, - Generator, + Iterator, List, Mapping, Optional, @@ -513,7 +513,7 @@ def _replace_guids_with_channels(self, payload: Any) -> Any: def needs_full_stack_trace(self) -> bool: return self._tracing_count > 0 - def get_frame_info(self) -> Generator[FrameInfo, None]: + def get_frame_info(self) -> Iterator[FrameInfo]: current_frame = inspect.currentframe() if current_frame is None: @@ -542,7 +542,7 @@ async def wrap_api_call( if self._api_zone.get(): return await cb() task = asyncio.current_task(self._loop) - st: Union[List[FrameInfo], Generator[FrameInfo, None]] = ( + st: Union[List[FrameInfo], Iterator[FrameInfo]] = ( getattr(task, "__pw_stack__", None) or self.get_frame_info() ) parsed_st = _extract_stack_trace_information_from_stack( @@ -562,7 +562,7 @@ def wrap_api_call_sync( if self._api_zone.get(): return cb() task = asyncio.current_task(self._loop) - st: Union[List[FrameInfo], Generator[FrameInfo, None]] = ( + st: Union[List[FrameInfo], Iterator[FrameInfo]] = ( getattr(task, "__pw_stack__", None) or self.get_frame_info() ) parsed_st = _extract_stack_trace_information_from_stack( @@ -598,7 +598,7 @@ class ParsedStackTrace(TypedDict): def _extract_stack_trace_information_from_stack( - st: Union[List[FrameInfo], Generator[FrameInfo, None]], + st: Union[List[FrameInfo], Iterator[FrameInfo]], is_internal: bool, needs_full_stack: bool, ) -> ParsedStackTrace: