[py][bidi]: improve tests to use local no-proxy server #16747

navin772 · 2025-12-18T15:08:47Z

User description

🔗 Related Issues

💥 What does this PR do?

Starts and uses a local http server for the no-proxy tests since occasionally the internet site is down and results in tests failing.

🔧 Implementation Notes

Also enables 2 proxy tests for firefox.

💡 Additional Considerations

🔄 Types of changes

Cleanup (formatting, renaming)

PR Type

Enhancement, Tests

Description

Refactor proxy test infrastructure to use local HTTP servers
Replace external site dependency with local no-proxy server
Remove Firefox and remote xfail markers from proxy tests
Generalize HTTP server handler for reusable test utilities

Diagram Walkthrough

flowchart LR
  A["External Site Dependency<br/>the-internet.herokuapp.com"] -->|Replace| B["Local HTTP Server<br/>localhost:port"]
  C["FakeProxyHandler<br/>Single Purpose"] -->|Refactor| D["start_local_http_server<br/>Reusable Handler"]
  E["Firefox xfail Markers"] -->|Remove| F["Enable Firefox Tests"]
  B -->|Enables| G["Reliable No-Proxy Tests"]

File Walkthrough

Relevant files

Tests

bidi_browser_tests.py `Refactor proxy tests to use local HTTP servers` py/test/selenium/webdriver/common/bidi_browser_tests.py Refactored `FakeProxyHandler` into generic `start_local_http_server()` function with configurable response content and logging Simplified `start_fake_proxy()` to use the new generic server function Replaced external `the-internet.herokuapp.com` dependency with local HTTP server for no-proxy testing Removed `@pytest.mark.xfail_firefox` and `@pytest.mark.xfail_remote` decorators from proxy tests Added proper server cleanup with `shutdown()` and `server_close()` calls in test teardown Updated proxy configuration to use local server ports instead of external URLs	+27/-22

qodo-code-review · 2025-12-18T15:09:23Z

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

🟡

🎫 #1234

🔴	Ensure that `click()` triggers JavaScript in a link’s `href` (regression from Selenium 2.47.1 to 2.48.x) in Firefox (notably Firefox 42).
	Provide/validate a reproducible test case demonstrating the fix (alert expected) for the described scenario.

🟡

🎫 #5678

🔴	Prevent/resolve repeated ChromeDriver instantiation failures causing "ConnectFailure (Connection refused)" on Ubuntu/Chrome/ChromeDriver as described.
	Ensure multiple successive `ChromeDriver` instantiations do not emit the reported console errors.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured print logging: The new HTTP server handler emits unstructured log output via print(...) rather than using
structured logging, reducing auditability and parsability.

Referred Code

if log_prefix:
    print(f"[{log_prefix}] Intercepted request to: {self.path}")

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Server startup failures: The new start_local_http_server() starts a TCPServer without handling bind/startup errors
(e.g., port already in use), which may cause less actionable failures in CI.

Referred Code

def start_local_http_server(port, response_content=b"local server response", log_prefix=None):
    class Handler(http.server.SimpleHTTPRequestHandler):
        def do_GET(self):
            if log_prefix:
                print(f"[{log_prefix}] Intercepted request to: {self.path}")
            self.send_response(200)
            self.end_headers()
            self.wfile.write(response_content)

    server = socketserver.TCPServer(("localhost", port), Handler)
    threading.Thread(target=server.serve_forever, daemon=True).start()
    return server

Learn more about managing compliance generic rules or creating your own custom rules

Update

Compliance status legend

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

qodo-code-review · 2025-12-18T15:10:48Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Prevent race condition in server startup~~ Suggestion Impact: Instead of adding a threading.Event to start_local_http_server, the commit removed the custom start_local_http_server/start_fake_proxy helpers entirely and refactored tests to use an existing proxy_server fixture. This eliminates the original startup code path (and its potential race) rather than implementing the suggested synchronization directly. code diff: -import http.server import os -import socketserver -import threading import pytest @@ -28,27 +25,8 @@ from selenium.webdriver.common.bidi.session import UserPromptHandler, UserPromptHandlerType from selenium.webdriver.common.by import By from selenium.webdriver.common.proxy import Proxy, ProxyType -from selenium.webdriver.common.utils import free_port from selenium.webdriver.common.window import WindowTypes from selenium.webdriver.support.ui import WebDriverWait - - -def start_local_http_server(port, response_content=b"local server response", log_prefix=None): - class Handler(http.server.SimpleHTTPRequestHandler): - def do_GET(self): - if log_prefix: - print(f"[{log_prefix}] Intercepted request to: {self.path}") - self.send_response(200) - self.end_headers() - self.wfile.write(response_content) - - server = socketserver.TCPServer(("localhost", port), Handler) - threading.Thread(target=server.serve_forever, daemon=True).start() - return server - - -def start_fake_proxy(port): - return start_local_http_server(port, response_content=b"proxied response", log_prefix="Fake Proxy") def test_browser_initialized(driver): @@ -168,15 +146,13 @@ @pytest.mark.xfail_chrome(reason="Chrome auto upgrades HTTP to HTTPS in untrusted networks like CI environments") -def test_create_user_context_with_manual_proxy_all_params(driver): +def test_create_user_context_with_manual_proxy_all_params(driver, proxy_server): """Test creating a user context with manual proxy configuration.""" - # Start a fake proxy server - proxy_port = free_port() - fake_proxy_server = start_fake_proxy(port=proxy_port) - - # Start a local HTTP server for the no_proxy test - no_proxy_port = free_port() - no_proxy_server = start_local_http_server(port=no_proxy_port, response_content=b"direct connection - not proxied") + create_proxy_server = proxy_server(response_content=b"proxied response") + no_proxy_server = proxy_server(response_content=b"direct connection - not proxied") + + proxy_port = create_proxy_server["port"] + no_proxy_port = no_proxy_server["port"] proxy = Proxy() proxy.proxy_type = ProxyType.MANUAL @@ -206,18 +182,13 @@ finally: driver.browser.remove_user_context(user_context) - no_proxy_server.shutdown() - no_proxy_server.server_close() - fake_proxy_server.shutdown() - fake_proxy_server.server_close() @pytest.mark.xfail_chrome(reason="Chrome auto upgrades HTTP to HTTPS in untrusted networks like CI environments") -def test_create_user_context_with_proxy_and_accept_insecure_certs(driver): +def test_create_user_context_with_proxy_and_accept_insecure_certs(driver, proxy_server): """Test creating a user context with both acceptInsecureCerts and proxy parameters.""" - # Start fake proxy server - port = free_port() - fake_proxy_server = start_fake_proxy(port=port) + create_proxy_server = proxy_server(response_content=b"proxied response") + port = create_proxy_server["port"] proxy = Proxy() proxy.proxy_type = ProxyType.MANUAL @@ -243,8 +214,6 @@ finally: driver.browser.remove_user_context(user_context) - fake_proxy_server.shutdown() - fake_proxy_server.server_close() To prevent a race condition in `start_local_http_server`, use a `threading.Event` to ensure the server is fully initialized before the function returns. py/test/selenium/webdriver/common/bidi_browser_tests.py [36-47] def start_local_http_server(port, response_content=b"local server response", log_prefix=None): class Handler(http.server.SimpleHTTPRequestHandler): def do_GET(self): if log_prefix: print(f"[{log_prefix}] Intercepted request to: {self.path}") self.send_response(200) self.end_headers() self.wfile.write(response_content) server = socketserver.TCPServer(("localhost", port), Handler) - threading.Thread(target=server.serve_forever, daemon=True).start() + started = threading.Event() + + def serve(): + started.set() + server.serve_forever() + + threading.Thread(target=serve, daemon=True).start() + started.wait() return server `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a race condition in the new `start_local_http_server` function and proposes a robust solution using `threading.Event` to prevent flaky tests.	Medium
Possible issue	Add Content-Length header Add a `Content-Length` header to the HTTP response to specify the response size and prevent potential client-side issues. py/test/selenium/webdriver/common/bidi_browser_tests.py [41-43] self.send_response(200) +self.send_header("Content-Length", str(len(response_content))) self.end_headers() self.wfile.write(response_content) Apply / Chat Suggestion importance[1-10]: 5 __ Why: This is a good suggestion that improves the HTTP compliance of the mock server by adding the `Content-Length` header, which is a best practice.	Low
General	✅ ~~Enable address reuse~~ Suggestion Impact: The commit removed the custom TCPServer-based helpers (start_local_http_server/start_fake_proxy) entirely and switched the affected tests to use a proxy_server fixture instead. While it does not directly set allow_reuse_address as suggested, it changes the server setup mechanism in the exact area the suggestion targeted, likely addressing the same flakiness/port-reuse problem via a different implementation. code diff: @@ -15,10 +15,7 @@ # specific language governing permissions and limitations # under the License. -import http.server import os -import socketserver -import threading import pytest @@ -28,27 +25,8 @@ from selenium.webdriver.common.bidi.session import UserPromptHandler, UserPromptHandlerType from selenium.webdriver.common.by import By from selenium.webdriver.common.proxy import Proxy, ProxyType -from selenium.webdriver.common.utils import free_port from selenium.webdriver.common.window import WindowTypes from selenium.webdriver.support.ui import WebDriverWait - - -def start_local_http_server(port, response_content=b"local server response", log_prefix=None): - class Handler(http.server.SimpleHTTPRequestHandler): - def do_GET(self): - if log_prefix: - print(f"[{log_prefix}] Intercepted request to: {self.path}") - self.send_response(200) - self.end_headers() - self.wfile.write(response_content) - - server = socketserver.TCPServer(("localhost", port), Handler) - threading.Thread(target=server.serve_forever, daemon=True).start() - return server - - -def start_fake_proxy(port): - return start_local_http_server(port, response_content=b"proxied response", log_prefix="Fake Proxy") Set `allow_reuse_address = True` on the `TCPServer` to prevent "address already in use" errors during rapid test execution. py/test/selenium/webdriver/common/bidi_browser_tests.py [45] -server = socketserver.TCPServer(("localhost", port), Handler) +class ReusableTCPServer(socketserver.TCPServer): + allow_reuse_address = True +server = ReusableTCPServer(("localhost", port), Handler) + `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: This suggestion improves the robustness of the test setup by setting `allow_reuse_address`, which helps prevent "address already in use" errors when tests are run in quick succession.	Low
Learned best practice	✅ ~~Guarantee cleanup on early failures~~ Suggestion Impact: The commit refactors the tests to use a `proxy_server` fixture to create the proxy and no-proxy HTTP servers and removes the explicit server shutdown/close calls from the test. This shifts cleanup responsibility to the fixture, which typically guarantees teardown even on early failures, addressing the core resource-leak concern without implementing the exact try/finally structure proposed. code diff: @pytest.mark.xfail_chrome(reason="Chrome auto upgrades HTTP to HTTPS in untrusted networks like CI environments") -def test_create_user_context_with_manual_proxy_all_params(driver): +def test_create_user_context_with_manual_proxy_all_params(driver, proxy_server): """Test creating a user context with manual proxy configuration.""" - # Start a fake proxy server - proxy_port = free_port() - fake_proxy_server = start_fake_proxy(port=proxy_port) - - # Start a local HTTP server for the no_proxy test - no_proxy_port = free_port() - no_proxy_server = start_local_http_server(port=no_proxy_port, response_content=b"direct connection - not proxied") + create_proxy_server = proxy_server(response_content=b"proxied response") + no_proxy_server = proxy_server(response_content=b"direct connection - not proxied") + + proxy_port = create_proxy_server["port"] + no_proxy_port = no_proxy_server["port"] proxy = Proxy() proxy.proxy_type = ProxyType.MANUAL @@ -206,18 +182,13 @@ finally: driver.browser.remove_user_context(user_context) - no_proxy_server.shutdown() - no_proxy_server.server_close() - fake_proxy_server.shutdown() - fake_proxy_server.server_close() @pytest.mark.xfail_chrome(reason="Chrome auto upgrades HTTP to HTTPS in untrusted networks like CI environments") -def test_create_user_context_with_proxy_and_accept_insecure_certs(driver): +def test_create_user_context_with_proxy_and_accept_insecure_certs(driver, proxy_server): """Test creating a user context with both acceptInsecureCerts and proxy parameters.""" - # Start fake proxy server - port = free_port() - fake_proxy_server = start_fake_proxy(port=port) + create_proxy_server = proxy_server(response_content=b"proxied response") + port = create_proxy_server["port"] proxy = Proxy() proxy.proxy_type = ProxyType.MANUAL @@ -243,8 +214,6 @@ finally: driver.browser.remove_user_context(user_context) - fake_proxy_server.shutdown() - fake_proxy_server.server_close() Ensure servers are shut down even if `create_user_context`/`browsing_context.create` fails by wrapping server creation and later steps in a broader try/finally (or a contextmanager) so cleanup always runs. py/test/selenium/webdriver/common/bidi_browser_tests.py [174-212] proxy_port = free_port() +no_proxy_port = free_port() fake_proxy_server = start_fake_proxy(port=proxy_port) - -# Start a local HTTP server for the no_proxy test -no_proxy_port = free_port() no_proxy_server = start_local_http_server(port=no_proxy_port, response_content=b"direct connection - not proxied") -proxy = Proxy() -proxy.proxy_type = ProxyType.MANUAL -proxy.http_proxy = f"localhost:{proxy_port}" -proxy.ssl_proxy = f"localhost:{proxy_port}" -proxy.socks_proxy = f"localhost:{proxy_port}" -proxy.socks_version = 5 -proxy.no_proxy = [f"localhost:{no_proxy_port}"] +user_context = None +try: + proxy = Proxy() + proxy.proxy_type = ProxyType.MANUAL + proxy.http_proxy = f"localhost:{proxy_port}" + proxy.ssl_proxy = f"localhost:{proxy_port}" + proxy.socks_proxy = f"localhost:{proxy_port}" + proxy.socks_version = 5 + proxy.no_proxy = [f"localhost:{no_proxy_port}"] -user_context = driver.browser.create_user_context(proxy=proxy) + user_context = driver.browser.create_user_context(proxy=proxy) -# Create and switch to a new browsing context using this proxy -bc = driver.browsing_context.create(type=WindowTypes.WINDOW, user_context=user_context) -driver.switch_to.window(bc) + bc = driver.browsing_context.create(type=WindowTypes.WINDOW, user_context=user_context) + driver.switch_to.window(bc) -try: - # Visit no proxy site, it should bypass proxy driver.get(f"http://localhost:{no_proxy_port}/") body_text = driver.find_element(By.TAG_NAME, "body").text.lower() assert "direct connection - not proxied" in body_text - # Visit a site that should be proxied driver.get("http://example.com/") - body_text = driver.find_element("tag name", "body").text assert "proxied response" in body_text.lower() - finally: - driver.browser.remove_user_context(user_context) + if user_context is not None: + driver.browser.remove_user_context(user_context) no_proxy_server.shutdown() no_proxy_server.server_close() fake_proxy_server.shutdown() fake_proxy_server.server_close() `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always wrap creation of external contexts/resources in try/finally and perform explicit cleanup in finally to prevent leaks when assertions/exceptions occur.	Low
Learned best practice	Avoid invalid no-proxy entries Use a valid `no_proxy` host value (typically hostnames/IPs without ports) to avoid passing an API-invalid entry that may be ignored by some drivers. py/test/selenium/webdriver/common/bidi_browser_tests.py [187] -proxy.no_proxy = [f"localhost:{no_proxy_port}"] +proxy.no_proxy = ["localhost"] Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Validate and sanitize external inputs (URLs, names, headers) and encode where needed to conform to APIs and avoid invalid requests.	Low
Update

cgoldberg

This looks great.

What do you think about moving the proxy server stuff into fixtures (something like proxy_server and no_proxy_server)? It would be a little nicer as fixtures instead of functions where you have to remember to stop the server.

It also might be a good idea to keep them in ./py/conftest.py. That way we could expand the other proxy tests (./py/test/selenium/webdriver/chrome/proxy_tests.py) to use them when we convert the high level API to use BiDi.

navin772 · 2025-12-19T07:51:01Z

What do you think about moving the proxy server stuff into fixtures (something like proxy_server and no_proxy_server)? It would be a little nicer as fixtures instead of functions where you have to remember to stop the server.

Yes, that's a good idea, we can have 1 fixture - proxy_server() and use that in tests to create both proxy and no-proxy server since setup of both is similar. I will add this.

improve tests to use local no-proxy

2623427

selenium-ci added the C-py Python Bindings label Dec 18, 2025

qodo-code-review bot added the Review effort 2/5 label Dec 18, 2025

navin772 requested a review from cgoldberg December 18, 2025 15:28

cgoldberg reviewed Dec 18, 2025

View reviewed changes

Merge branch 'trunk' into improve-bidi-proxy-tests

8da4ee6

navin772 added 2 commits December 19, 2025 13:23

create and utilize proxy_server fixture

1eea73e

Merge branch 'trunk' into improve-bidi-proxy-tests

3dc6f55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[py][bidi]: improve tests to use local no-proxy server #16747

[py][bidi]: improve tests to use local no-proxy server #16747

navin772 commented Dec 18, 2025 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

cgoldberg left a comment •

edited

Loading

Uh oh!

navin772 commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

[py][bidi]: improve tests to use local no-proxy server #16747

Are you sure you want to change the base?

[py][bidi]: improve tests to use local no-proxy server #16747

Conversation

navin772 commented Dec 18, 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 Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Dec 18, 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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navin772 commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

navin772 commented Dec 18, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Dec 18, 2025 •

edited

Loading

qodo-code-review bot commented Dec 18, 2025 •

edited

Loading

cgoldberg left a comment •

edited

Loading