You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR adds a test for using BiDi request handlers when using classic navigation. This is currently broken in Chrome and Edge, but works in Firefox.
🔧 Implementation Notes
I wanted to mark is as xfail_chrome/xfail_edge, but the test hangs for several minutes when you attempt to run it with those browsers, so I just explicitly skip it inside the test.
💡 Additional Considerations
Once Chrome/Edge fixes this, we should stop skipping this test.
🔄 Types of changes
Test
PR Type
Tests
Description
Add test verifying BiDi request handlers work with classic navigation
Skip test for Chrome and Edge due to known browser limitations
Test passes in Firefox, demonstrating expected behavior
Diagram Walkthrough
flowchart LR
A["Test Setup"] --> B["Add Request Handler"]
B --> C["Classic Navigation"]
C --> D["Verify No Exceptions"]
D --> E["Skip for Chrome/Edge"]
Loading
File Walkthrough
Relevant files
Tests
bidi_network_tests.py
Add BiDi request handler test for classic navigation
-callback_id = driver.network.add_request_handler("before_request", callback)-assert callback_id is not None, "Request handler not added"-pages.load("formPage.html")-assert len(exceptions) == 0, "Exception raised when continuing request in callback"+callback_id = None+try:+ callback_id = driver.network.add_request_handler("before_request", callback)+ assert callback_id is not None, "Request handler not added"+ pages.load("formPage.html")+ assert len(exceptions) == 0, f"Exception raised when continuing request in callback: {exceptions}"+finally:+ if callback_id:+ driver.network.remove_request_handler(callback_id)
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a resource leak in a test where a network handler is not removed, which can lead to flaky tests, and proposes a robust try...finally block to ensure cleanup.
Medium
General
✅ Consolidate browser skip logicSuggestion Impact:The commit consolidated the separate skip conditions into a single conditional, though it used browser_name.lower() with ("chrome", "microsoftedge") and updated the skip message format.
code diff:
+ browser_name = driver.caps["browserName"]+ if browser_name.lower() in ("chrome", "microsoftedge"):+ pytest.skip(reason="Request handlers don't yet work in {browser_name} when using classic navigation")
Consolidate the separate if conditions for skipping the test on Chrome and Edge into a single condition using in for better readability and maintainability.
-if driver.caps["browserName"] == "chrome":- pytest.skip(reason="Request handlers don't yet work in Chrome when using classic navigation")-if driver.caps["browserName"] == "edge":- pytest.skip(reason="Request handlers don't yet work in Edge when using classic navigation")+if driver.caps["browserName"] in ("chrome", "edge"):+ pytest.skip(reason="Request handlers don't yet work in Chrome/Edge when using classic navigation")
[Suggestion processed]
Suggestion importance[1-10]: 4
__
Why: The suggestion improves code conciseness and maintainability by consolidating two if statements into a single check, which is a good practice but has a low impact on functionality.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
navin772
left a comment
User description
💥 What does this PR do?
This PR adds a test for using BiDi request handlers when using classic navigation. This is currently broken in Chrome and Edge, but works in Firefox.
🔧 Implementation Notes
I wanted to mark is as
xfail_chrome/xfail_edge, but the test hangs for several minutes when you attempt to run it with those browsers, so I just explicitly skip it inside the test.💡 Additional Considerations
Once Chrome/Edge fixes this, we should stop skipping this test.
🔄 Types of changes
PR Type
Tests
Description
Add test verifying BiDi request handlers work with classic navigation
Skip test for Chrome and Edge due to known browser limitations
Test passes in Firefox, demonstrating expected behavior
Diagram Walkthrough
File Walkthrough
bidi_network_tests.py
Add BiDi request handler test for classic navigationpy/test/selenium/webdriver/common/bidi_network_tests.py
test_handler_with_classic_navigationto verify BiDirequest handlers work with classic navigation
feature is not yet supported