Playwright Async Actions with Selenium Actions Parity and Browser Session Interoperability#657
Conversation
nasif-z
changed the title
async/await
|
@claude Review the changes and suggest additional improvements - specially in regards to if something was missed with async/await. |
Partial match was not picking up action named "validate partial text"
nasif-z
force-pushed
the
playwright-async
branch
from
05970f0 to
1f0e02d
Compare
Removes threading and uses async instead for `Thread_ScreenShot`
nasif-z
changed the title
async/await- Await coroutine functions inside the logger decorator before logging exit. - Preserve existing sync logger behavior and custom fail message logging. - Add tests covering async log order, sync behavior, and fail messages.
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The async refactor introduces a high-severity regression in loop execution, and there is another async callsite still invoking the sequential engine synchronously. → Security ReviewStatus: ✅ Completed No critical security regressions stand out in the PR diff. The changes are primarily async refactors and Playwright/Selenium interoperability enhancements, with no obvious new injection, authz, secret-handling, or path-traversal issues introduced. → Performance ReviewStatus: ✅ Completed No significant performance regressions or scalability issues stood out in the PR diff. → Testing ReviewStatus: ✅ Completed The PR changes the core async action dispatcher and Playwright/Selenium interoperability, but there are no added tests covering those new execution paths. That leaves the highest-risk behavior unverified, especially the async
|
- add lazy Get_Element resolution with timeout helper - pass wait values into high-frequency Playwright actions - avoid double wait in Wait_For_Element - cover lazy and resolved locator/action parity paths
- Await sequential loop and bypass handler execution - Run async sequential callbacks inside performance worker threads - Await Playwright page/session activation helpers - Execute async driver functions correctly in threaded runs - Await coroutine results returned by installer wrapper callables - Share sync session cleanup for CLI prompt path
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed I found one concrete regression in the new Playwright session routing and one broader API break from making the sequential runner async without updating all direct callers. → Security ReviewStatus: ✅ Completed No high-signal security regressions found in the PR diff. The changes mainly refactor Playwright/Selenium orchestration to async and add new browser actions without introducing an obvious injection, authz, or secret-handling flaw. → Performance ReviewStatus: ✅ Completed One performance risk: the new Playwright network logger buffers every matching request/response in memory until stop, which can grow without bound on busy pages. → Testing ReviewStatus: ✅ Completed The PR adds substantial async dispatch and Playwright routing behavior, but I don’t see tests covering the new execution paths or the browser-driver precedence rules.
|
- Normalize legacy wait aliases after browser-driver routing selects Playwright - Parse legacy wait and wait disable rows in Playwright wait handling - Cover Playwright versus Selenium routing and legacy timeout/state parsing
- It did not match the Selenium counterpart behaviour - Selenium's double click action actually does a mouse movement for execution
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The async migration introduced two blocking regressions in the performance-action path: newly async handlers are still called synchronously, and the handlers themselves still use → Security ReviewStatus: ✅ Completed No critical security regressions stood out in the async Playwright/Selenium parity changes. I found one data-exposure concern in the new network logging path. → Performance ReviewStatus: ✅ Completed I found one performance issue in the new Playwright network logging path: it retains request objects unnecessarily during capture, which can grow memory usage without affecting the final output. → Testing ReviewStatus: ✅ Completed The PR’s async browser-session routing changes are largely untested; the highest-risk gap is a missing regression test that proves
|
…up with `resolve=False` Actions: keystrokes, validation text reads, save/get element info, scroll, scroll-to-element, change attribute, select/deselect, check/uncheck, and slider
- Uses Playwright's native CSS locator where possible - Falls back to Selenium-style element locator when needed
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The async conversion is broadly in the right direction, but there is a runtime blocker in the performance action path: it now calls → Security ReviewStatus: ✅ Completed I found one moderate data-exposure regression: the new Playwright network-log action collects and persists raw response bodies and request metadata without any redaction or sensitivity filtering. → Performance ReviewStatus: ✅ Completed Found one performance concern in the new async execution path: it repeatedly creates fresh event loops in hot paths, which adds overhead and can become costly under load. → Testing ReviewStatus: ✅ Completed The PR adds several high-risk async execution and Playwright-routing paths, but the new behavior is only partially covered; the biggest gap is regression coverage for async Python execution and browser-driver routing precedence.
|
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The async migration is broadly consistent, but the new performance-action bridge will fail at runtime because it nests → Security ReviewStatus: ✅ Completed No clear security regressions in the PR diff. The changes are mostly async plumbing and Playwright feature additions, with no obvious new injection, authz, or secret-handling issues in the reviewed paths. → Performance ReviewStatus: ✅ Completed The async refactor is functionally broad, but I only see a couple of performance concerns: one benchmark-path overhead from per-task event-loop creation, and one new always-on compatibility hydration step in Python execution. → Testing ReviewStatus: ✅ Completed The PR updates core action dispatching and Playwright routing, but the test suite only covers a subset of the new async and browser-session behavior. The most important gaps are regression coverage for async Python execution and the new Playwright conditional-action path.
|
- Route Playwright Page, Locator, and FrameLocator lookups through shared LocateElement parsing and query construction. - Fold Playwright execution into existing locator, text filter, shadow DOM, index, and result-selection paths. - Remove the separate Web/Playwright/locator.py helper API and update Playwright actions to use shared LocateElement.Get_Element. - Preserve lazy locator returns for normal Playwright action rows while resolving conditionals, indexes, text filters, and return_all_elements lookups. - Update sequential Playwright conditional lookup to use shared LocateElement. - Refresh Playwright locator and action parity tests for the shared API. - Verified with PYTHONPATH=. pytest tests/test_playwright_locator.py tests/test_playwright_actions_parity.py and compileall checks.
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The async conversion is directionally correct, but one new execution path will fail at runtime because it nests → Security ReviewStatus: ✅ Completed No security findings in the PR diff. The changes primarily convert sequential execution to async and add Playwright/Selenium interoperability without introducing a clear injection, authz, secrets, or data-exposure issue. → Performance ReviewStatus: ✅ Completed Two hotspots stand out in the async conversion: per-task event-loop creation in the load-testing path, and unconditional browser-session hydration before every custom Python execution. Both are likely to add avoidable overhead at scale. → Testing ReviewStatus: ✅ Completed The async conversion and new Playwright routing paths are mostly untested. I found a critical gap in core runner coverage plus missing smoke coverage for the newly declared Playwright actions.
|
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The async Playwright/Selenium conversion is mostly coherent, but I found two correctness regressions: a legacy Playwright alias now changes behavior, and the new async → Security ReviewStatus: ✅ Completed No security-impacting regressions were introduced in this PR diff. The changes are primarily async plumbing and Playwright action routing, with no clear new injection, authz, secrets, or exposure issues in the reviewed patch. → Performance ReviewStatus: ✅ Completed One performance regression stood out: the new async execution path still uses blocking sleeps inside → Testing ReviewStatus: ✅ Completed The PR expands async sequential execution and adds several new Playwright action routes, but the existing test suite does not cover the new dispatch/routing behavior or the newly registered actions.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5db2c85858
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6 participants
Summary
MainDriverintegration.LocateElement.Get_Elementparsing and query construction instead of a separate Playwright locator module.Playwright Async Migration
MainDriverintegration to async execution.go to linkaction to async and reset iframe context during navigation.Browser Session Interoperability
go to linkand other commonly used actions.driver_idinstead of assuming the default session.Shared Locator Support
LocateElement.Get_Elementwhile keeping Selenium, Appium, XML, and desktop behavior synchronous.LocateElementparsing forindex,allow hidden,save parameter,get parameter, Shadow DOM rows, raw XPath/CSS, and relationship rows._construct_query,_construct_xpath_list,_locate_index_number,build_css_selector_query,text_filter, andshadow_root_elements.locator,filter(visible=True),wait_for,all,count,text_content, and Shadow DOM locator chaining.return_all_elementslookups.Framework/Built_In_Automation/Web/Playwright/locator.pyand updated Playwright actions to import sharedLocateElementasPlaywrightLocator.LocateElement.Get_Element.Playwright Feature Coverage And Parity
actioncolumn parsing behavior between frameworks.browser driveras an optional parameter andBROWSER_DRIVERas a runtime parameter.Reliability Fixes
validate partial textwas not being resolved correctly.Browser Provisioning And Installation
Performance And Maintenance
PILpost-processing for Playwright screenshots.LocateElement.py.uv.lock.Tests
tests/test_browser_sessions.py.tests/test_playwright_actions_parity.py.tests/test_playwright_locator.py.PYTHONPATH=. pytest tests/test_playwright_locator.py tests/test_playwright_actions_parity.py.python -m compileall Framework/Built_In_Automation/Shared_Resources/LocateElement.py Framework/Built_In_Automation/Web/Playwright/BuiltInFunctions.py Framework/Built_In_Automation/Sequential_Actions/sequential_actions.py.Files With Significant Changes
Framework/Built_In_Automation/Shared_Resources/LocateElement.pyFramework/Built_In_Automation/Web/Playwright/BuiltInFunctions.pyFramework/Built_In_Automation/Sequential_Actions/sequential_actions.pyFramework/Built_In_Automation/Web/Selenium/BuiltInFunctions.pyFramework/Built_In_Automation/Web/utils.pyFramework/Built_In_Automation/Web/Playwright/utils.pyFramework/Built_In_Automation/Sequential_Actions/common_functions.pyFramework/MainDriverApi.pytests/test_playwright_locator.pytests/test_playwright_actions_parity.py