[py][bidi]: use bidi `navigate` command in network tests by navin772 · Pull Request #16251 · SeleniumHQ/selenium

navin772 · 2025-08-25T06:12:31Z

User description

🔗 Related Issues

💥 What does this PR do?

Fixes and enables some tests in bidi_network_tests.py for chrome and edge.

🔧 Implementation Notes

We can say that it is a bug in chromium, if we navigate to a page using a classic command like driver.get() (which we were doing in our tests), continue request won't work, it is working when we navigate using BiDi commands - driver.browsing_context.navigate. At the time of network PR, we didn't have the browsing context module so it was skipped.
For the auth test, seems like httpbin is not working (TLS issue), swapping it with https://postman-echo.com/basic-auth works well. We can also run a docker container in the CI to avoid dependency on this site - https://hub.docker.com/r/kennethreitz/httpbin/ but I think it will be complex to add it to all workflows.

💡 Additional Considerations

🔄 Types of changes

Cleanup (formatting, renaming)

PR Type

Tests

Description

Fix BiDi network tests by using BiDi navigate command
Switch from httpbin to postman-echo for auth testing
Remove xfail markers for Chrome and Edge tests

Diagram Walkthrough

flowchart LR
  A["Classic driver.get()"] -- "Replace with" --> B["BiDi navigate command"]
  C["httpbin.org"] -- "Switch to" --> D["postman-echo.com"]
  E["Failing tests"] -- "Enable" --> F["Working tests"]

File Walkthrough

Relevant files

Tests

bidi_network_tests.py `Fix BiDi network tests navigation` py/test/selenium/webdriver/common/bidi_network_tests.py Replace classic navigation with BiDi `browsing_context.navigate()` Switch from httpbin to postman-echo for auth testing Remove xfail markers for Chrome and Edge Add ReadinessState import for navigation waiting	+7/-10

qodo-code-review · 2025-08-25T06:12:59Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 5678 - Partially compliant Compliant requirements: Non-compliant requirements: Investigate and resolve "ConnectFailure" on ChromeDriver instantiation Ensure stable ChromeDriver sessions without repeated connection failures Provide reproducible steps or fixes for the connection refusal Requires further human verification: 1234 - Partially compliant Compliant requirements: Non-compliant requirements: Ensure click() triggers JavaScript in link href in Firefox 2.48 Provide validation/tests for JS href execution on click Requires further human verification:
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Possible Issue In test_continue_with_auth, the variable `callback_id` is asserted but never defined before assignment; this will raise a NameError or fail the assertion. Ensure `callback_id` is set from add_auth_handler as intended. def test_continue_with_auth(driver): callback_id = driver.network.add_auth_handler("postman", "password") assert callback_id is not None, "Request handler not added" driver.browsing_context.navigate( context=driver.current_window_handle, url="https://postman-echo.com/basic-auth", wait=ReadinessState.COMPLETE ) assert "authenticated" in driver.page_source, "Authorization failed" API Usage The BiDi navigate call uses `context=driver.current_window_handle`. Confirm that BiDi `browsing_context.navigate` expects a browsing context id compatible with `current_window_handle`; some drivers require `driver.bidi_session` context ids. url = pages.url("formPage.html") driver.browsing_context.navigate(context=driver.current_window_handle, url=url, wait=ReadinessState.COMPLETE) assert driver.find_element(By.NAME, "login").is_displayed(), "Request not continued" Test Fragility External dependency on postman-echo.com for basic-auth may introduce flakiness. Consider local mock/server or dockerized httpbin as noted in PR description. driver.browsing_context.navigate( context=driver.current_window_handle, url="https://postman-echo.com/basic-auth", wait=ReadinessState.COMPLETE ) assert "authenticated" in driver.page_source, "Authorization failed"

qodo-code-review · 2025-08-25T06:14:00Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use correct BiDi context id Ensure the correct browsing context id is passed. `driver.current_window_handle` typically returns a WebDriver window handle, which may not match the BiDi browsing context id. Use the BiDi-derived context id to avoid navigation failing silently. py/test/selenium/webdriver/common/bidi_network_tests.py [78] -driver.browsing_context.navigate(context=driver.current_window_handle, url=url, wait=ReadinessState.COMPLETE) +context_id = driver.browsing_context.current_context # obtain the BiDi browsing context id from the driver +driver.browsing_context.navigate(context=context_id, url=url, wait=ReadinessState.COMPLETE) Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that `driver.current_window_handle` might not be the appropriate BiDi context ID, and using a dedicated BiDi property improves the robustness and correctness of the test.	Medium
Possible issue	Assert against correct response Verify the expected response body for Postman Echo's basic auth endpoint. It returns a JSON payload with `"authenticated": true`, not the literal string `"authenticated"` in raw HTML. Parse the body or check for the specific JSON key to prevent false negatives. py/test/selenium/webdriver/common/bidi_network_tests.py [83-88] callback_id = driver.network.add_auth_handler("postman", "password") assert callback_id is not None, "Request handler not added" +context_id = driver.browsing_context.current_context driver.browsing_context.navigate( - context=driver.current_window_handle, url="https://postman-echo.com/basic-auth", wait=ReadinessState.COMPLETE + context=context_id, url="https://postman-echo.com/basic-auth", wait=ReadinessState.COMPLETE ) -assert "authenticated" in driver.page_source, "Authorization failed" +assert '"authenticated": true' in driver.page_source or "authenticated" in driver.find_element(By.TAG_NAME, "body").text, "Authorization failed" Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out the response format of the new endpoint and proposes a more specific assertion, which makes the test more robust and less prone to false positives.	Medium
Learned best practice	Align assertion with test intent The final assertion checks `driver.network.intercepts`, which appears unrelated to auth handlers and likely a copy-paste error. Update the assertion to verify that the auth handler was removed, e.g., by checking the auth handlers collection or by asserting that an authenticated request now fails. This keeps the test name, intent, and assertions consistent. py/test/selenium/webdriver/common/bidi_network_tests.py [91-95] def test_remove_auth_handler(driver): callback_id = driver.network.add_auth_handler("user", "passwd") assert callback_id is not None, "Request handler not added" driver.network.remove_auth_handler(callback_id) - assert driver.network.intercepts == [], "Intercept not removed" + # Ensure the auth handler is removed; authenticated request should now fail + driver.browsing_context.navigate( + context=driver.current_window_handle, + url="https://postman-echo.com/basic-auth", + wait=ReadinessState.COMPLETE, + ) + assert "authenticated" not in driver.page_source, "Auth handler not removed" Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Fix syntax errors, typos, and naming inconsistencies in code and tests to maintain clarity and prevent confusion.	Low
General	Reduce auth flow flakiness Add an explicit wait for `ReadinessState.COMPLETE` is good, but network-level auth challenges can complete with `interactive` in some drivers. To avoid flakiness, consider waiting for either `COMPLETE` or verifying the page content post-navigation with a short retry loop. py/test/selenium/webdriver/common/bidi_network_tests.py [85-87] +context_id = driver.browsing_context.current_context driver.browsing_context.navigate( - context=driver.current_window_handle, url="https://postman-echo.com/basic-auth", wait=ReadinessState.COMPLETE + context=context_id, url="https://postman-echo.com/basic-auth", wait=ReadinessState.COMPLETE ) +# Fallback verification to reduce flakiness +for _ in range(5): + if '"authenticated": true' in driver.page_source: + break + driver.sleep(0.2) +assert '"authenticated": true' in driver.page_source, "Authorization failed" Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion proposes adding a retry loop to handle potential flakiness, which could improve test stability, although the existing wait on `ReadinessState.COMPLETE` should already be sufficient.	Low
Update

cgoldberg

LGTM. Thanks!

use bidi method to navigate and switch to postman-echo

ba572a4

selenium-ci added the C-py Python Bindings label Aug 25, 2025

qodo-code-review bot added the Review effort 2/5 label Aug 25, 2025

cgoldberg approved these changes Aug 25, 2025

View reviewed changes

navin772 added 2 commits August 25, 2025 18:01

Merge branch 'trunk' into fix-bidi-network-tests-py

ec740de

Merge branch 'trunk' into fix-bidi-network-tests-py

f9541c7

navin772 merged commit 0754c5a into SeleniumHQ:trunk Aug 26, 2025
4 checks passed

navin772 deleted the fix-bidi-network-tests-py branch August 26, 2025 03:54

This was referenced Oct 6, 2025

Bump Selenium.WebDriver from 4.35.0 to 4.36.0 appium/dotnet-client#964

Merged

Bump Selenium.WebDriver from 4.35.0 to 4.36.0 Heniks07/PanoScrobblerAlbumFixer#42

Closed

dependabot bot mentioned this pull request Oct 28, 2025

Bump the selenium group with 2 updates 7474/SRC#689

Closed

This was referenced Dec 9, 2025

Bump the selenium group with 2 updates 7474/SRC#693

Open

Bump the stagezero-selenium group with 2 updates mitchHay/StageZero#263

Closed

Bump the functionaltests group with 4 updates samsmithnz/SamsFeatureFlags#1188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[py][bidi]: use bidi `navigate` command in network tests#16251

[py][bidi]: use bidi `navigate` command in network tests#16251
navin772 merged 3 commits intoSeleniumHQ:trunkfrom
navin772:fix-bidi-network-tests-py

navin772 commented Aug 25, 2025 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Aug 25, 2025

Uh oh!

qodo-code-review bot commented Aug 25, 2025 •

edited

Loading

Uh oh!

cgoldberg left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

navin772 commented Aug 25, 2025 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Aug 25, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review bot commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

cgoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

navin772 commented Aug 25, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Aug 25, 2025 •

edited

Loading