Repository: microsoft/playwright-python
Pull request #2651 (2024-11-13 20:04:57)

fix(tracing): apiName determination with event listeners
The PR fixes an issue with incorrect `apiName` determination in tracing when using event listeners. Here's what was happening:

When using sync event listeners, the stack trace would include internal implementation details from the `_impl_to_api_mapping` module, which acts as glue between user code and internal code. We expect no other function between the implementation and the user call-side. This caused the API name to be incorrectly determined since these internal frames were being included in the stack analysis.

The key changes:
- Added filtering to skip stack frames from `_impl_to_api_mapping` when extracting stack trace information
- Added test cases for both sync and async event listeners to verify correct API name determination

The async case worked correctly because async event listeners only schedule `asyncio.Task`s without maintaining the full stack trace.

Test Plan:
- Added new test cases `test_should_correctly_determine_sync_apiname` for both sync and async implementations
- Tests verify correct API action sequence in traces when using event listeners
- Tested with response handlers making API calls like `all_headers()` and `text()`

Fixes https://github.com/microsoft/playwright-python/issues/2650


Patch:
diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py
index 910693f..8433058 100644
--- a/playwright/_impl/_connection.py
+++ b/playwright/_impl/_connection.py
@@ -37,6 +37,7 @@ from pyee import EventEmitter
 from pyee.asyncio import AsyncIOEventEmitter
 
 import playwright
+import playwright._impl._impl_to_api_mapping
 from playwright._impl._errors import TargetClosedError, rewrite_error
 from playwright._impl._greenlets import EventGreenlet
 from playwright._impl._helper import Error, ParsedMessagePayload, parse_error
@@ -573,6 +574,12 @@ def _extract_stack_trace_information_from_stack(
     api_name = ""
     parsed_frames: List[StackFrame] = []
     for frame in st:
+        # Sync and Async implementations can have event handlers. When these are sync, they
+        # get evaluated in the context of the event loop, so they contain the stack trace of when
+        # the message was received. _impl_to_api_mapping is glue between the user-code and internal
+        # code to translate impl classes to api classes. We want to ignore these frames.
+        if playwright._impl._impl_to_api_mapping.__file__ == frame.filename:
+            continue
         is_playwright_internal = frame.filename.startswith(playwright_module_path)
 
         method_name = ""


Test patch:
diff --git a/tests/async/test_tracing.py b/tests/async/test_tracing.py
index 0274575..dae1be6 100644
--- a/tests/async/test_tracing.py
+++ b/tests/async/test_tracing.py
@@ -12,11 +12,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import asyncio
 import re
 from pathlib import Path
 from typing import Dict, List
 
-from playwright.async_api import Browser, BrowserContext, BrowserType, Page
+from playwright.async_api import Browser, BrowserContext, BrowserType, Page, Response
 from tests.server import Server
 from tests.utils import get_trace_actions, parse_trace
 
@@ -145,6 +146,35 @@ async def test_should_collect_trace_with_resources_but_no_js(
     assert script["snapshot"]["response"]["content"].get("_sha1") is None
 
 
+async def test_should_correctly_determine_sync_apiname(
+    context: BrowserContext, page: Page, server: Server, tmpdir: Path
+) -> None:
+    await context.tracing.start(screenshots=True, snapshots=True)
+
+    received_response: "asyncio.Future[None]" = asyncio.Future()
+
+    async def _handle_response(response: Response) -> None:
+        await response.request.all_headers()
+        await response.text()
+        received_response.set_result(None)
+
+    page.once("response", _handle_response)
+    await page.goto(server.PREFIX + "/grid.html")
+    await received_response
+    await page.close()
+    trace_file_path = tmpdir / "trace.zip"
+    await context.tracing.stop(path=trace_file_path)
+
+    (_, events) = parse_trace(trace_file_path)
+    assert events[0]["type"] == "context-options"
+    assert get_trace_actions(events) == [
+        "Page.goto",
+        "Request.all_headers",
+        "Response.text",
+        "Page.close",
+    ]
+
+
 async def test_should_collect_two_traces(
     context: BrowserContext, page: Page, server: Server, tmpdir: Path
 ) -> None:
diff --git a/tests/sync/test_tracing.py b/tests/sync/test_tracing.py
index cdf669f..98a6f61 100644
--- a/tests/sync/test_tracing.py
+++ b/tests/sync/test_tracing.py
@@ -13,10 +13,11 @@
 # limitations under the License.
 
 import re
+import threading
 from pathlib import Path
 from typing import Any, Dict, List
 
-from playwright.sync_api import Browser, BrowserContext, BrowserType, Page
+from playwright.sync_api import Browser, BrowserContext, BrowserType, Page, Response
 from tests.server import Server
 from tests.utils import get_trace_actions, parse_trace
 
@@ -138,6 +139,35 @@ def test_should_collect_trace_with_resources_but_no_js(
     assert script["snapshot"]["response"]["content"].get("_sha1") is None
 
 
+def test_should_correctly_determine_sync_apiname(
+    context: BrowserContext, page: Page, server: Server, tmpdir: Path
+) -> None:
+    context.tracing.start(screenshots=True, snapshots=True)
+    received_response = threading.Event()
+
+    def _handle_response(response: Response) -> None:
+        response.request.all_headers()
+        response.text()
+        received_response.set()
+
+    page.once("response", _handle_response)
+    page.goto(server.PREFIX + "/grid.html")
+    received_response.wait()
+
+    page.close()
+    trace_file_path = tmpdir / "trace.zip"
+    context.tracing.stop(path=trace_file_path)
+
+    (_, events) = parse_trace(trace_file_path)
+    assert events[0]["type"] == "context-options"
+    assert get_trace_actions(events) == [
+        "Page.goto",
+        "Request.all_headers",
+        "Response.text",
+        "Page.close",
+    ]
+
+
 def test_should_collect_two_traces(
     context: BrowserContext, page: Page, server: Server, tmpdir: Path
 ) -> None: