Skip to content

Conversation

cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Oct 13, 2025

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

  • 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         

py/test/selenium/webdriver/common/bidi_network_tests.py

  • Adds new test test_handler_with_classic_navigation to verify BiDi
    request handlers work with classic navigation
  • Implements browser-specific skip logic for Chrome and Edge where
    feature is not yet supported
  • Uses callback with exception tracking to validate request continuation
+21/-0   

@selenium-ci selenium-ci added the C-py Python Bindings label Oct 13, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 13, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 13, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure test resource cleanup

Add a try...finally block to ensure the network request handler is removed after
the test completes, preventing state leakage between tests.

py/test/selenium/webdriver/common/bidi_network_tests.py [117-120]

-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 logic
Suggestion 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.

py/test/selenium/webdriver/common/bidi_network_tests.py [104-107]

-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.

Low
  • Update

@cgoldberg cgoldberg requested a review from navin772 October 13, 2025 16:26
Copy link
Member

@navin772 navin772 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Though, the test needs to be fixed so CI passes

@cgoldberg cgoldberg merged commit 1b7f3b0 into SeleniumHQ:trunk Oct 13, 2025
4 checks passed
@cgoldberg cgoldberg deleted the py-bidi-request-handlers-classic-navigation branch October 13, 2025 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants