[py] BiDi Network implementation of Intercepts and Auth in Python #14592

shbenzer · 2024-10-13T17:28:29Z

User description

Add implementations for BiDi Network's Auth and Interception to Python

Description

Added network.py: Network, Request, and Response classes w/ related methods
Added bidi_network_tests.py

Motivation and Context

Issue #13993

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

PR Type

Enhancement, Tests

Description

Added BiDi Network support for request/response interception and authentication.
Implemented Network, Request, and Response classes with related methods.
Integrated new network commands into WebDriver's remote command structure.
Added comprehensive tests for BiDi Network features, including request/response handling and authentication.

Changes walkthrough 📝

Relevant files

Enhancement

network.py `Implement BiDi Network operations and data encapsulation` py/selenium/webdriver/common/bidi/network.py Introduced `Network` class for BiDi Network operations. Added methods for request/response interception and authentication handling. Implemented `Request` and `Response` classes for encapsulating network data.	+258/-0
command.py `Add network-related commands for BiDi support` py/selenium/webdriver/remote/command.py Added new network-related commands for BiDi operations. Defined constants for intercept, request, and response handling.	+5/-0
remote_connection.py `Map BiDi network commands to HTTP endpoints` py/selenium/webdriver/remote/remote_connection.py Mapped BiDi network commands to HTTP endpoints. Supported intercept, request, and response handling in remote connection.	+5/-0
webdriver.py `Integrate BiDi Network into WebDriver` py/selenium/webdriver/remote/webdriver.py Integrated `Network` class into WebDriver. Added property to initialize and access BiDi Network functionality.	+12/-0
websocket_connection.py `Enhance callback handling for BiDi network events` py/selenium/webdriver/remote/websocket_connection.py Enhanced callback handling for BiDi network events. Adjusted event name handling for network operations.	+1/-1

Tests

bidi_network_tests.py `Add tests for BiDi Network features` py/test/selenium/webdriver/common/bidi_network_tests.py Added tests for BiDi Network request/response interception. Verified authentication handling and callback functionality. Marked tests as expected to fail on Safari.	+88/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

qodo-merge-pro · 2024-10-13T17:29:00Z

PR Reviewer Guide 🔍

(Review updated until commit `2b3bb2a`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 Security concerns Credentials Exposure: The authentication handler stores username and password in plaintext in the subscriptions dictionary. This could potentially expose sensitive credentials if the object is serialized or logged. Consider storing credentials more securely or clearing them after use.
⚡ Recommended focus areas for review Missing UUID Import The code uses uuid.uuid4() but the uuid module is not imported, which would cause a NameError when generating request/response IDs request_id = data['request'].get('requestId', uuid.uuid4()) Command Iterator Issue The command_iterator method is used but not properly defined as an instance method, which could cause issues with command execution self.network.conn.execute(self.command_iterator(command)) Race Condition Potential race condition in callback handling where callbacks dict is modified while being iterated in __handle_event def __handle_event(self, event, data): """Perform callback function on event.""" if event in self.callbacks: self.callbacks[event](data)

qodo-merge-pro · 2024-10-13T17:29:35Z

PR Code Suggestions ✨

Latest suggestions up to 2b3bb2a

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	✅ ~~Add missing import statement for a module used in the code~~ Suggestion Impact: The uuid import statement was added at the top of the file as suggested code diff: +import uuid Import the missing 'uuid' module at the top of the file, as it's used in request and response handlers for generating unique IDs. py/selenium/webdriver/common/bidi/network.py [150] +import uuid +... request_id = data['request'].get('requestId', uuid.uuid4()) Suggestion importance[1-10]: 8 Why: The code uses uuid.uuid4() but the uuid module is not imported, which would cause a NameError at runtime. This is a critical issue that would break the request/response handling functionality.	8
General	Remove inconsistent subscription handling to prevent potential state issues The session_subscribe call is commented out but the session_unsubscribe is still active, which could lead to inconsistent subscription state - either uncomment the subscribe call or remove both. py/selenium/webdriver/common/bidi/network.py [113-114] -# session_subscribe(self.conn, event, self.__handle_event) return self.conn.add_callback(event, self.__handle_event) Apply this suggestion Suggestion importance[1-10]: 7 Why: The inconsistency between commented subscribe and active unsubscribe calls could lead to subscription state issues. This needs to be resolved to ensure proper event handling.	7
Security	Add validation for authentication credentials to prevent silent failures Add error handling for invalid credentials in the authentication handler to prevent silent failures when authentication fails. py/selenium/webdriver/common/bidi/network.py [93-106] def __continue_with_auth(self, request_id, username, password): + if not username or not password: + raise ValueError("Both username and password must be provided") command = {'method': 'network.continueWithAuth', 'params': { 'request': request_id, 'action': 'provideCredentials', 'credentials': { 'type': 'password', 'username': username, 'password': password } } } self.conn.execute(self.command_iterator(command)) Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding input validation for authentication credentials would prevent potential security issues and improve error handling, though the current implementation would still work for valid credentials.	6

Previous suggestions

✅ Suggestions up to commit 01f6aaf

Category	Suggestion	Score
Best practice	Add input validation to network operation methods to prevent issues caused by invalid parameters Consider adding input validation for the parameters in methods like `continue_response`, `continue_request`, and `add_intercept`. This can help prevent issues caused by invalid input and improve the overall robustness of the code. py/selenium/webdriver/common/bidi/network.py [39-48] def continue_response(self, request_id, status_code, headers=None, body=None): + if not isinstance(request_id, str) or not request_id: + raise ValueError("request_id must be a non-empty string") + if not isinstance(status_code, int) or status_code < 100 or status_code >= 600: + raise ValueError("status_code must be a valid HTTP status code") params = { 'requestId': request_id, 'status': status_code } if headers is not None: + if not isinstance(headers, dict): + raise ValueError("headers must be a dictionary") params['headers'] = headers if body is not None: + if not isinstance(body, (str, bytes)): + raise ValueError("body must be a string or bytes") params['body'] = body self.conn.execute('network.continueResponse', params) Suggestion importance[1-10]: 8 Why: Input validation is crucial for preventing errors and ensuring that methods receive valid data. This suggestion enhances the robustness and reliability of the code by checking parameter types and values before proceeding with network operations.	8
Enhancement	Implement robust error handling for network operations to improve reliability and debugging Consider using a more robust error handling mechanism for the network operations. Wrap the self.conn.execute() calls in try-except blocks to catch and handle potential exceptions, providing more informative error messages or logging. py/selenium/webdriver/common/bidi/network.py [48] -self.conn.execute('network.continueResponse', params) +try: + self.conn.execute('network.continueResponse', params) +except Exception as e: + logging.error(f"Failed to continue response: {str(e)}") + raise Suggestion importance[1-10]: 7 Why: Adding error handling around network operations can improve the robustness of the code by catching and logging exceptions, which aids in debugging and ensures that the application can handle unexpected issues gracefully.	7
	✅ Add a method to remove event listeners, improving flexibility in event management Suggestion Impact: The commit introduced methods to remove event handlers, such as `remove_authentication_handler`, `remove_request_handler`, and `remove_response_handler`, which align with the suggestion to provide more flexibility in managing event subscriptions. code diff: + def remove_authentication_handler(self, username,): + """Removes an authentication handler.""" + self.__remove_intercept(intercept='auth_required') + del self.subscriptions['auth_required'] + session_unsubscribe(self.conn, self.EVENTS['auth_required']) + + def add_request_handler(self, callback, url_pattern=''): + """ + Adds a request handler that executes a callback function when a request matches the given URL pattern. + + Parameters: + callback (function): A function to be executed when url is matched by a URL pattern + The callback function receives a `Response` object as its argument. + url_pattern (str, optional): A substring to match against the response URL. + Default is an empty string, which matches all URLs. + + Returns: + str: The request ID of the intercepted response. + """ + self.__add_intercept(phases=[self.PHASES['before_request']]) + def callback_on_url_match(data): + if url_pattern in data['request']['url']: + # create request object to pass to callback + request_id = data['request'].get('requestId') + url = data['request'].get('url') + method = data['request'].get('method') + headers = data['request'].get('headers', {}) + body = data['request'].get('postData', None) + request = Request(request_id, url, method, headers, body, self) + callback(request) + self.__on('before_request', callback_on_url_match) + self.callbacks[request_id] = callback + if 'before_request' not in self.subscriptions or not self.subscriptions.get('before_request'): + self.subscriptions['before_request'] = [request_id] + else: + self.subscriptions['before_request'].append(request_id) + return request_id + + def remove_request_handler(self, request_id): + """Removes a request handler.""" + self.__remove_intercept(request_id=request_id) + self.subscriptions['before_request'].remove(request_id) + del self.callbacks[request_id] + if len(self.subscriptions['before_request']) == 0: + session_unsubscribe(self.conn, self.EVENTS['before_request']) + + def add_response_handler(self, callback, url_pattern=''): + """ + Adds a response handler that executes a callback function when a response matches the given URL pattern. + + Parameters: + callback (function): A function to be executed when url is matched by a url_pattern + The callback function receives a `Response` object as its argument. + url_pattern (str, optional): A substring to match against the response URL. + Default is an empty string, which matches all URLs. + + Returns: + str: The request ID of the intercepted response. + """ + self.__add_intercept(phases=[self.PHASES['response_started']]) + def callback_on_url_match(data): + # create response object to pass to callback + if url_pattern in data['response']['url']: + request_id = data['request'].get('requestId') + url = data['response'].get('url') + status_code = data['response'].get('status') + body = data['response'].get('body', None) + headers = data['response'].get('headers', {}) + response = Response(request_id, url, status_code, headers, body, self) + callback(response) + self.__on('response_started', callback_on_url_match) + self.callbacks[request_id] = callback + if 'response_started' not in self.subscriptions or not self.subscriptions.get('response_started'): + self.subscriptions['response_started'] = [request_id] + else: + self.subscriptions['response_started'].append(request_id) + return request_id + + def remove_response_handler(self, response_id): + """Removes a response handler.""" + self.__remove_intercept(request_id=response_id) + self.subscriptions['response_started'].remove(response_id) + del self.callbacks[response_id] + if len(self.subscriptions['response_started']) == 0: + session_unsubscribe(self.conn, self.EVENTS['response_started']) Consider implementing a method to remove all event listeners for a specific event or all events. This would provide more flexibility in managing event subscriptions and prevent potential memory leaks. py/selenium/webdriver/common/bidi/network.py [91-94] def on(self, event, callback): event = self.EVENTS.get(event, event) self.callbacks[event] = callback session_subscribe(self.conn, event, self.handle_event) +def off(self, event=None): + if event: + event = self.EVENTS.get(event, event) + if event in self.callbacks: + del self.callbacks[event] + session_unsubscribe(self.conn, event, self.handle_event) + else: + for event in list(self.callbacks.keys()): + session_unsubscribe(self.conn, event, self.handle_event) + self.callbacks.clear() + Suggestion importance[1-10]: 7 Why: Implementing a method to remove event listeners provides better control over event management and can prevent potential memory leaks. This enhancement increases the flexibility and maintainability of the code.	7
	✅ Expand test coverage to include various scenarios and edge cases for network operations Suggestion Impact: The commit implements expanded test coverage by adding multiple new test cases for network operations, including request handlers, response handlers, and authentication scenarios. While not exactly matching the suggested parametrized test, it achieves the same goal of comprehensive testing. code diff: +@pytest.mark.xfail_safari +def test_network_initialized(driver): + assert driver.network is not None -def test_add_intercept(driver, network): - network.add_intercept(phases=[Network.PHASES['before_request']]) +@pytest.mark.xfail_safari +def test_add_response_handler(driver, pages): + passed = [False] -def test_remove_intercept(driver, network): - intercept = network.add_intercept(phases=[Network.PHASES['before_request']]) - network.remove_intercept(intercept) + def callback(response): + passed[0] = True + response.continue_response() -def test_continue_response(driver, network): - network.add_intercept(phases=[Network.PHASES['before_request']]) - network.on('before_request', lambda event: network.continue_response(event['requestId'], 200)) - - driver.get(url_for("basicAuth")) - assert driver.find_element_by_tag_name('h1').text == 'authorized' + driver.network.add_response_handler(callback) + pages.load("basicAuth") + assert passed[0], "Callback was NOT successful" -def test_continue_request(driver, network): - network.add_intercept(phases=[Network.PHASES['before_request']]) - network.on('before_request', lambda event: network.continue_request(event['requestId'], url=url_for("basicAuth"))) - - driver.get(url_for("basicAuth")) - assert driver.find_element_by_tag_name('h1').text == 'authorized' +@pytest.mark.xfail_safari +def test_remove_response_handler(driver, pages): + passed = [False] -def test_continue_with_auth(driver, network): - username = 'your_username' - password = 'your_password' - - network.add_intercept(phases=[Network.PHASES['auth_required']]) - network.on('auth_required', lambda event: network.continue_with_auth(event['requestId'], username, password)) - - driver.get(url_for("basicAuth")) - assert driver.find_element_by_tag_name('h1').text == 'authorized' + def callback(response): + passed[0] = True + response.continue_response() + + test_response_id = driver.network.add_response_handler(callback) + driver.network.remove_response_handler(response_id=test_response_id) + pages.load("basicAuth") + assert not passed[0], "Callback should NOT be successful" + +@pytest.mark.xfail_safari +def test_add_request_handler(driver, pages): + passed = [False] + + def callback(request): + passed[0] = True + request.continue_request() + + driver.network.add_request_handler(callback) + pages.load("basicAuth") + assert passed[0], "Callback was NOT successful" + +@pytest.mark.xfail_safari +def test_remove_request_handler(driver, pages): + passed = [False] + + def callback(request): + passed[0] = True + request.continue_request() + + test_request_id = driver.network.add_request_handler(callback) + driver.network.remove_request_handler(request_id=test_request_id) + pages.load("basicAuth") + assert not passed[0], "Callback should NOT be successful" + +@pytest.mark.xfail_safari +def test_add_authentication_handler(driver, pages): + driver.network.add_authentication_handler('test','test') + pages.load("basicAuth") + assert driver.find_element(By.TAG_NAME, 'h1').text == 'authorized', "Authentication was NOT successful" + +@pytest.mark.xfail_safari +def test_remove_authentication_handler(driver, pages): + driver.network.add_authentication_handler('test', 'test') + driver.network.remove_authentication_handler() + pages.load("basicAuth") + assert driver.find_element(By.TAG_NAME, 'h1').text != 'authorized', "Authentication was successful" Enhance the test coverage by adding more comprehensive test cases for different scenarios, including edge cases and error conditions. This will help ensure the robustness of the network functionality. py/test/selenium/webdriver/common/bidi_network_tests.py [27-32] -def test_continue_response(driver, network): +@pytest.mark.parametrize("status_code,expected_text", [ + (200, "authorized"), + (401, "unauthorized"), + (500, "server error") +]) +def test_continue_response(driver, network, status_code, expected_text): network.add_intercept(phases=[Network.PHASES['before_request']]) - network.on('before_request', lambda event: network.continue_response(event['requestId'], 200)) + network.on('before_request', lambda event: network.continue_response(event['requestId'], status_code)) driver.get(url_for("basicAuth")) - assert driver.find_element_by_tag_name('h1').text == 'authorized' + assert expected_text in driver.page_source.lower() Suggestion importance[1-10]: 6 Why: Enhancing test coverage by including different scenarios and edge cases can significantly improve the reliability of the network functionality. This suggestion is valuable for ensuring that the code behaves correctly under various conditions.	6

✅ Suggestions up to commit 14bf971

Category	Suggestion	Score
Possible issue	Validate input parameters to prevent potential issues with invalid data being passed to the BiDi protocol Consider implementing input validation for the `add_intercept` method to ensure that the provided phases are valid according to the `PHASES` dictionary. This can prevent potential issues with invalid phase names being passed to the BiDi protocol. py/selenium/webdriver/common/bidi/network.py [68-76] def add_intercept(self, phases=None, contexts=None, url_patterns=None): if phases is None: phases = [] + else: + valid_phases = set(self.PHASES.values()) + if not all(phase in valid_phases for phase in phases): + raise ValueError(f"Invalid phase(s) provided. Valid phases are: {', '.join(valid_phases)}") params = { 'phases': phases, 'contexts': contexts, 'urlPatterns': url_patterns } self.conn.execute('network.addIntercept', params) Suggestion importance[1-10]: 8 Why: Implementing input validation for the `add_intercept` method is a valuable enhancement that prevents invalid phase names from causing issues. This suggestion addresses a potential bug and improves the method's robustness.	8
Enhancement	Implement error handling for network operations to improve robustness and prevent unexpected crashes Consider using a more robust error handling mechanism for the network operations. Wrap the self.conn.execute() calls in try-except blocks to catch and handle potential exceptions, ensuring graceful error handling and preventing crashes. py/selenium/webdriver/common/bidi/network.py [43-52] def continue_response(self, request_id, status_code, headers=None, body=None): params = { 'requestId': request_id, 'status': status_code } if headers is not None: params['headers'] = headers if body is not None: params['body'] = body - self.conn.execute('network.continueResponse', params) + try: + self.conn.execute('network.continueResponse', params) + except Exception as e: + logging.error(f"Error in continue_response: {str(e)}") + raise Suggestion importance[1-10]: 7 Why: Adding error handling to network operations can improve the robustness of the code by preventing crashes due to unhandled exceptions. This suggestion is relevant and enhances the reliability of the `continue_response` method.	7
Enhancement	✅ Enhance test assertions to provide more comprehensive verification of network interception behavior Suggestion Impact: The commit added more comprehensive assertions to verify network interception behavior, such as checking the status of responses and the method of requests, which aligns with the suggestion to enhance test assertions. code diff: +def test_add_response_handler(response): + passed = [False] -def test_continue_request(driver, network): - network.add_intercept(phases=[Network.PHASES['before_request']]) - network.on('before_request', lambda event: network.continue_request(event['requestId'], url=url_for("basicAuth"))) - - driver.get(url_for("basicAuth")) - assert driver.find_element_by_tag_name('h1').text == 'authorized' + def callback(event): + if event['response']['status'] == 200: + passed[0] = True -def test_continue_with_auth(driver, network): - username = 'your_username' - password = 'your_password' - - network.add_intercept(phases=[Network.PHASES['auth_required']]) - network.on('auth_required', lambda event: network.continue_with_auth(event['requestId'], username, password)) - - driver.get(url_for("basicAuth")) - assert driver.find_element_by_tag_name('h1').text == 'authorized'+ network_response.add_response_handler(callback) + pages.load("basicAuth") + assert passed[0] == True, "Callback was NOT successful" + +def test_remove_response_handler(response): + passed = [False] + + def callback(event): + if event['response']['status'] == 200: + passed[0] = True + + network_response.add_response_handler(callback) + network_response.remove_response_handler(callback) + pages.load("basicAuth") + assert passed[0] == False, "Callback was successful" + +def test_add_request_handler(request): + passed = [False] + + def callback(event): + if event['request']['method'] == 'GET': + passed[0] = True + + network_request.add_request_handler(callback) + pages.load("basicAuth") + assert passed[0] == True, "Callback was NOT successful" + +def test_remove_request_handler(request): + passed = [False] + + def callback(event): + if event['request']['method'] == 'GET': + passed[0] = True + + network_request.add_request_handler(callback) + network_request.remove_request_handler(callback) + pages.load("basicAuth") + assert passed[0] == False, "Callback was successful" + +def test_add_authentication_handler(network): + network.add_authentication_handler('test','test') + pages.load("basicAuth") + assert driver.find_element_by_tag_name('h1').text == 'authorized', "Authentication was NOT successful" + +def test_remove_authentication_handler(network): + network.add_authentication_handler('test', 'test') + network.remove_authentication_handler() + pages.load("basicAuth") Consider adding more comprehensive assertions to verify the behavior of network interceptions. This could include checking the content of intercepted requests or responses, verifying headers, or ensuring that the correct number of interceptions occur. py/test/selenium/webdriver/common/bidi_network_tests.py [29-34] def test_continue_response(driver, network): + intercepted_requests = [] network.add_intercept(phases=[Network.PHASES['before_request']]) + network.on('before_request', lambda event: intercepted_requests.append(event)) network.on('before_request', lambda event: network.continue_response(event['requestId'], 200)) driver.get(url_for("basicAuth")) assert driver.find_element_by_tag_name('h1').text == 'authorized' + assert len(intercepted_requests) > 0, "No requests were intercepted" + assert intercepted_requests[0]['request']['url'] == url_for("basicAuth"), "Unexpected URL intercepted" Suggestion importance[1-10]: 7 Why: Adding more comprehensive assertions in tests improves the verification of network interception behavior, ensuring the tests are more thorough and reliable. This suggestion enhances the quality of the test suite.	7
Best practice	Use a context manager for WebDriver instances to ensure proper resource cleanup Consider using a context manager for the WebDriver instance to ensure proper cleanup, even if an exception occurs during the test. This can help prevent resource leaks and improve test reliability. py/test/selenium/webdriver/common/bidi_network_tests.py [11-16] @pytest.fixture def driver(): options = webdriver.ChromeOptions() options.add_argument('--remote-debugging-port=9222') - driver = webdriver.Chrome(options=options) - yield driver - driver.quit() + with webdriver.Chrome(options=options) as driver: + yield driver Suggestion importance[1-10]: 6 Why: Using a context manager for the WebDriver instance ensures proper cleanup and resource management, even if exceptions occur. This suggestion follows best practices for resource handling in tests.	6

Thought I might need it, but didn't

shbenzer · 2024-10-18T23:19:51Z

was adding this to BUILD.bazel for easy testing:

py_test(
    name = "bidi_network_tests",
    srcs = ["test/selenium/webdriver/common/bidi_network_tests.py"],
    deps = [
        ":init-tree",
        ":selenium",
        ":webserver",
    ] + TEST_DEPS,
)

so i could run bazel test //py:bidi_network_tests

shbenzer · 2024-10-18T23:20:57Z

Think it's ready now... @AutomatedTester could you give it a look?

qodo-merge-pro · 2024-10-18T23:21:27Z

Persistent review updated to latest commit 01f6aaf

p0deje · 2024-12-21T22:26:49Z

I used iterator because that's how original CDP implementation was written and I wanted to preserve the pattern. We can implement something else if you want. session_subscribe can definitely be re-used across all BiDi APIs.
Yes, let's use connection callbacks for this. You can use them in network or move them there, it's an implementation detail from my point of view.
👍

Also, do we know who wrote websocketconnection.py and/or script.py? I'd love for them to chime in here

I wrote those two 😄

shbenzer · 2024-12-22T19:24:34Z

Switched to using add_callback() in __on(). Getting dropped websocket connections which I'm not sure the cause of, and that might be the cause of the failures. Not sure otherwise - I think we're close though.

------------------------------ live log logreport ------------------------------
ERROR    websocket:_logging.py:77 Connection to remote host was lost. - goodbye

py/test/selenium/webdriver/common/bidi_network_tests.py::test_add_request_handler[chrome] FAILED [ 57%]
_______________________ test_add_request_handler[chrome] _______________________

driver = <selenium.webdriver.chrome.webdriver.WebDriver (session="58c6c81e3fbfa3a444b4a3f1611d88af")>
pages = <conftest.pages.<locals>.Pages object at 0x104e2d2e0>

    @pytest.mark.xfail_safari
    def test_add_request_handler(driver, pages):
        passed = [False]
    
        def callback(request):
            passed[0] = True
            request.continue_request()
    
        driver.network.add_request_handler(callback)
        pages.load("basicAuth")
>       assert passed[0], "Callback was NOT successful"
E       AssertionError: Callback was NOT successful
E       assert False

py/test/selenium/webdriver/common/bidi_network_tests.py:62: AssertionError

------------------------------ live log logreport ------------------------------
ERROR    websocket:_logging.py:77 Connection to remote host was lost. - goodbye

py/test/selenium/webdriver/common/bidi_network_tests.py::test_remove_request_handler[chrome] FAILED [ 71%]
_____________________ test_remove_request_handler[chrome] ______________________

driver = <selenium.webdriver.chrome.webdriver.WebDriver (session="83972e585cd798470b4bce36673f79fc")>
pages = <conftest.pages.<locals>.Pages object at 0x104c51a30>

    @pytest.mark.xfail_safari
    def test_remove_request_handler(driver, pages):
        passed = [False]
    
        def callback(request):
            passed[0] = True
            request.continue_request()
    
        test_request_id = driver.network.add_request_handler(callback)
>       driver.network.remove_request_handler(request_id=test_request_id)

py/test/selenium/webdriver/common/bidi_network_tests.py:73: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
py/selenium/webdriver/common/bidi/network.py:169: in remove_request_handler
    self.__remove_intercept(request_id=request_id)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <selenium.webdriver.common.bidi.network.Network object at 0x104c51610>
intercept = None, request_id = None

    def __remove_intercept(self, intercept=None, request_id=None):
        """Remove an intercept from the network."""
        if request_id is not None:
            command = {'method': 'network.removeIntercept', 'requestId': request_id}
            self.conn.execute(self.command_iterator(command))
        elif intercept is not None:
            command = {'method': 'network.removeIntercept', 'intercept': intercept}
            self.conn.execute(self.command_iterator(command))
        else:
>           raise ValueError('Either requestId or intercept must be specified')
E           ValueError: Either requestId or intercept must be specified

py/selenium/webdriver/common/bidi/network.py:91: ValueError

py/selenium/webdriver/common/bidi/network.py

shbenzer · 2024-12-23T19:49:39Z

hmm... @p0deje I unified request/response_id with the callback_id returned by add_callback (by updating __on() to return it) - however the issue persists. It was a good catch, but seem there is a deeper flaw.

shbenzer · 2024-12-24T17:54:21Z

I updated the callback_on_url() construction in add_request/response_handler() to create a unique request_id if not provided with one in data, but it might be better to throw an exception instead (or simply set it as None):

def callback_on_url_match(data):
  # create response object to pass to callback
  if url_pattern in data['response']['url']:
      request_id = data['request'].get('requestId', uuid.uuid4())
      ...

p0deje · 2025-01-16T00:49:39Z

@shbenzer Do you not want to continue working on the PR?

shbenzer · 2025-01-16T01:23:30Z

@shbenzer Do you not want to continue working on the PR?

I accidentally deleted the branch when I was cleaning up my fork (didn't think it'd close the pr but it's been reopened now) - I'm still working on it. However I'd love if someone could pull and give it a look to see if they can figure out the current blockage.

qodo-merge-pro · 2025-01-16T01:24:01Z

Persistent review updated to latest commit 2b3bb2a

shbenzer · 2025-02-12T15:43:11Z

Adding this to Build.Bazel for easy testing:

[
    py_test_suite(
        name = "common-%s-bidi-network" % browser,
        size = "large",
        srcs = glob(
            [
                "test/selenium/webdriver/common/**/bidi_network_tests.py",
            ],
        ),
        args = [
            "--instafail",
            "--bidi=true",
        ] + BROWSERS[browser]["args"],
        data = BROWSERS[browser]["data"],
        env_inherit = ["DISPLAY"],
        tags = ["no-sandbox"] + BROWSERS[browser]["tags"],
        deps = [
            ":init-tree",
            ":selenium",
            ":webserver",
        ] + TEST_DEPS,
    )
    for browser in BROWSERS.keys()
]

shbenzer added 2 commits October 12, 2024 17:48

added network.py, updated webdriver.py, added bidi_network_tests.py

4cde788

Tests are passing

14bf971

qodo-merge-pro bot added enhancement tests Review effort [1-5]: 4 labels Oct 13, 2024

shbenzer and others added 8 commits October 13, 2024 13:32

Merge branch 'trunk' into network_implementation_python

32d309d

Removed redundant bazel test

a83ff21

Merge branch 'trunk' into network_implementation_python

bfe622b

Merge branch 'trunk' into network_implementation_python

1072df0

deleted unused leftover function

981a1db

deleting other leftover function

1c7dc79

Thought I might need it, but didn't

removed unused imports

739ec81

cleanup

5284ad4

shbenzer marked this pull request as draft October 18, 2024 22:52

fixed url_for() - tests passing

01f6aaf

shbenzer marked this pull request as ready for review October 18, 2024 23:21

shbenzer and others added 9 commits October 22, 2024 13:27

Merge branch 'trunk' into network_implementation_python

17e7099

added instantiation of self._network = None

2325788

Merge branch 'trunk' into network_implementation_python

26379a0

Made functions async

8bc75c3

Update network.py

090e471

linting

3cb5762

linting

e397d89

linting

004ef85

remove debugging port from fixture

c57b56c

shbenzer and others added 2 commits December 22, 2024 14:20

switched to using add_callback

46ef051

Merge branch 'trunk' into network_implementation_python

475e877

p0deje reviewed Dec 23, 2024

View reviewed changes

py/selenium/webdriver/common/bidi/network.py Show resolved Hide resolved

unified callback_id and request/response_id

1e9dc81

shbenzer and others added 3 commits December 24, 2024 12:23

Merge branch 'trunk' into network_implementation_python

622c61e

removed wasted memort

29a5af9

removed nonlocal usage

b7be10d

Merge branch 'trunk' into network_implementation_python

2b3bb2a

shbenzer closed this Jan 16, 2025

shbenzer deleted the network_implementation_python branch January 16, 2025 00:38

shbenzer restored the network_implementation_python branch January 16, 2025 01:23

shbenzer reopened this Jan 16, 2025

shbenzer and others added 5 commits January 15, 2025 20:24

Merge branch 'trunk' into network_implementation_python

1f035d2

added import, gave abstractions command_iterator class, added .to_json()

ea577f5

added session_subscribe() to __on()

994fe95

shift ooo

5c5c0aa

format.sh

d53dc25

shbenzer marked this pull request as draft February 12, 2025 04:04

shbenzer and others added 4 commits February 12, 2025 10:43

spinning wheel of death

8e79940

Merge branch 'trunk' into network_implementation_python

eed84c6

Merge branch 'trunk' into network_implementation_python

ceb6468

removed unused import

1ac681e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[py] BiDi Network implementation of Intercepts and Auth in Python #14592

[py] BiDi Network implementation of Intercepts and Auth in Python #14592

shbenzer commented Oct 13, 2024 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Oct 13, 2024 •

edited

Loading

qodo-merge-pro bot commented Oct 13, 2024 •

edited

Loading

shbenzer commented Oct 18, 2024 •

edited

Loading

shbenzer commented Oct 18, 2024 •

edited

Loading

qodo-merge-pro bot commented Oct 18, 2024

p0deje commented Dec 21, 2024 •

edited

Loading

shbenzer commented Dec 22, 2024

shbenzer commented Dec 23, 2024

shbenzer commented Dec 24, 2024 •

edited

Loading

p0deje commented Jan 16, 2025

shbenzer commented Jan 16, 2025 •

edited

Loading

qodo-merge-pro bot commented Jan 16, 2025

shbenzer commented Feb 12, 2025

[py] BiDi Network implementation of Intercepts and Auth in Python #14592

Are you sure you want to change the base?

[py] BiDi Network implementation of Intercepts and Auth in Python #14592

Conversation

shbenzer commented Oct 13, 2024 • edited by qodo-merge-pro bot Loading

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

qodo-merge-pro bot commented Oct 13, 2024 • edited Loading

PR Reviewer Guide 🔍

(Review updated until commit 2b3bb2a)

qodo-merge-pro bot commented Oct 13, 2024 • edited Loading

PR Code Suggestions ✨

Previous suggestions

shbenzer commented Oct 18, 2024 • edited Loading

shbenzer commented Oct 18, 2024 • edited Loading

qodo-merge-pro bot commented Oct 18, 2024

p0deje commented Dec 21, 2024 • edited Loading

shbenzer commented Dec 22, 2024

shbenzer commented Dec 23, 2024

shbenzer commented Dec 24, 2024 • edited Loading

p0deje commented Jan 16, 2025

shbenzer commented Jan 16, 2025 • edited Loading

qodo-merge-pro bot commented Jan 16, 2025

shbenzer commented Feb 12, 2025

shbenzer commented Oct 13, 2024 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Oct 13, 2024 •

edited

Loading

(Review updated until commit `2b3bb2a`)

qodo-merge-pro bot commented Oct 13, 2024 •

edited

Loading

shbenzer commented Oct 18, 2024 •

edited

Loading

shbenzer commented Oct 18, 2024 •

edited

Loading

p0deje commented Dec 21, 2024 •

edited

Loading

shbenzer commented Dec 24, 2024 •

edited

Loading

shbenzer commented Jan 16, 2025 •

edited

Loading