[py]: reuse driver in case of bidi tests

[navin772]

User description

🔗 Related Issues

💥 What does this PR do?

Reuses the driver instance while running bidi tests. Currently, we restart the driver between every bidi test, which increase the test execution time.
This change will reduce the test execution times for all browsers.

Although, we need to make sure all the tests, especially event handler tests are properly cleaned up in the end.
If a test requires a fully isolated driver, then we can use the needs_fresh_driver marker added in this PR.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)
• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement, Tests

---

Description

• Reuse driver instance across BiDi tests to reduce execution time

• Add needs_fresh_driver marker for tests requiring isolation

• Implement window validity check instead of full driver restart

• Clean up event handlers and network callbacks in BiDi tests

---

Diagram Walkthrough

flowchart LR
  A["BiDi Test Execution"] --> B{"needs_fresh_driver marker?"}
  B -->|Yes| C["Restart Driver"]
  B -->|No| D["Check Window Validity"]
  D -->|Invalid| E["Restart Driver"]
  D -->|Valid| F["Reuse Driver"]
  C --> G["Next Test"]
  E --> G
  F --> G

Loading

File Walkthrough

	Relevant files
Enhancement	conftest.py BiDi driver reuse with selective restart logic                      py/conftest.py Modified BiDi driver fixture to reuse driver instance across tests instead of always restarting Added needs_fresh_driver marker check to allow selective full driver restart when needed Implemented ensure_valid_window() function to verify window handle validity before reusing driver Replaces unconditional driver stop with conditional cleanup logic +21/-5
Bug fix	bidi_browsing_context_tests.py Add event handler callback wait condition                                py/test/selenium/webdriver/common/bidi_browsing_context_tests.py Added explicit wait for event handler to receive callback before assertion Ensures event is captured before checking events_received list length +1/-0      bidi_network_tests.py Add network handler cleanup in BiDi tests                                py/test/selenium/webdriver/common/bidi_network_tests.py Added cleanup calls to remove network intercepts after test completion Added cleanup calls to remove request handlers after test completion Added cleanup call to remove auth handler after test completion Ensures proper isolation when driver is reused across tests +12/-1
Configuration changes	pyproject.toml Add needs_fresh_driver pytest marker                                          py/pyproject.toml Added new needs_fresh_driver pytest marker definition Updated marker documentation to describe fresh driver isolation requirement +2/-1

---

[qodo-code-review]

PR Compliance Guide 🔍

(Compliance updated until commit dad0ca9)

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 🔴 Provide a fix or regression test demonstrating correct alert behavior when clicking such links. ⚪ Investigate and ensure JavaScript in a link's href is triggered on click() in Firefox 42 where it worked in 2.47.1 but not in 2.48.x.
🟡	🎫 #5678 🔴 Provide a reliable workaround or code change ensuring multiple ChromeDriver instances can be created without connection failures. ⚪ Diagnose and resolve "Error: ConnectFailure (Connection refused)" occurring on subsequent ChromeDriver instantiations on Ubuntu with specific versions.
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: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. 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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Broad Exception: The finalizer ensure_valid_window swallows all exceptions with bare except Exception: pass, which may hide failures and hinder robust error handling. Referred Code try: driver = selenium_driver._driver if driver: try: # Check if current window is still valid driver.current_window_handle except Exception: # restart driver selenium_driver.stop_driver() except Exception: pass Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

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

---

Previous compliance checks

Compliance check up to commit bcd7095

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #1234 🔴 Identify and fix a regression where clicking a link with JavaScript in the href no longer triggers the JS in Selenium 2.48.x (worked in 2.47.1) on Firefox 42. ⚪ Provide reproduction/verification that alerts are triggered as expected.
🟡	🎫 #5678 🔴 Investigate and resolve "Error: ConnectFailure (Connection refused)" occurring on subsequent ChromeDriver instantiations on Ubuntu 16.04.4 with Selenium 3.9.0 and Chrome 65/ChromeDriver 2.35. Ensure multiple sequential ChromeDriver instances can be created without connection failures. Provide steps or code changes to prevent the connection issue from recurring.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	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: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. 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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Auditing: The new BiDi driver reuse and cleanup logic performs critical actions (driver restart/stop) without adding any audit logging, which may hinder reconstructing events. Referred Code # For BiDi tests, only restart driver when explicitly marked as needing fresh driver. # Tests marked with @pytest.mark.needs_fresh_driver get full driver restart for test isolation. # Cleanup after every test is recommended. if selenium_driver is not None and selenium_driver.bidi: if request.node.get_closest_marker("needs_fresh_driver"): request.addfinalizer(selenium_driver.stop_driver) else: def ensure_valid_window(): try: driver = selenium_driver._driver if driver: try: # Check if current window is still valid driver.current_window_handle except Exception: # restart driver selenium_driver.stop_driver() except Exception: pass ... (clipped 1 lines) 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: Broad Except: The ensure_valid_window finalizer swallows all exceptions with bare except blocks, potentially hiding errors and edge cases without logging. Referred Code def ensure_valid_window(): try: driver = selenium_driver._driver if driver: try: # Check if current window is still valid driver.current_window_handle except Exception: # restart driver selenium_driver.stop_driver() except Exception: pass Learn more about managing compliance generic rules or creating your own custom rules

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Automate test state cleanup centrally Instead of manually adding cleanup code to each test, centralize the BiDi state cleanup logic within the driver fixture in conftest.py. This will automatically clear event handlers and network intercepts after each test, ensuring better test isolation. Examples: py/test/selenium/webdriver/common/bidi_network_tests.py [32-38] def test_add_intercept(driver, pages): result = driver.network._add_intercept() assert result is not None, "Intercept not added" # Clean up driver.network._remove_intercept(result["intercept"]) py/conftest.py [349-367] if selenium_driver is not None and selenium_driver.bidi: if request.node.get_closest_marker("needs_fresh_driver"): request.addfinalizer(selenium_driver.stop_driver) else: def ensure_valid_window(): try: driver = selenium_driver._driver if driver: try: ... (clipped 9 lines) Solution Walkthrough: Before: # py/test/selenium/webdriver/common/bidi_network_tests.py def test_add_intercept(driver, pages): result = driver.network._add_intercept() assert result is not None, "Intercept not added" # Manual cleanup driver.network._remove_intercept(result["intercept"]) def test_continue_request(driver, pages): # ... setup callback ... callback_id = driver.network.add_request_handler("before_request", callback) # ... test logic ... assert len(exceptions) == 0 # Manual cleanup driver.network.remove_request_handler("before_request", callback_id) After: # py/conftest.py @pytest.fixture(scope="function") def driver(request): # ... if selenium_driver is not None and selenium_driver.bidi: if request.node.get_closest_marker("needs_fresh_driver"): request.addfinalizer(selenium_driver.stop_driver) else: def cleanup_bidi_state(): driver = selenium_driver._driver if not driver: return try: # Reset BiDi state (network intercepts, event handlers, etc.) # This might require new methods on the driver/bidi modules. driver.bidi_cleanup() except Exception: # If cleanup fails, restart the driver for safety selenium_driver.stop_driver() request.addfinalizer(cleanup_bidi_state) # ... yield selenium_driver.driver # ... Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that manual cleanup is error-prone and proposes a robust, centralized solution in conftest.py, which significantly improves test stability and maintainability.	High
General	Wait for exact event count To prevent potential race conditions, change the WebDriverWait condition from len(events_received) > 0 to len(events_received) == 1 to match the subsequent assertion. py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [723-726] WebDriverWait(driver, 5).until(EC.alert_is_present()) -WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0) +WebDriverWait(driver, 5).until(lambda d: len(events_received) == 1) assert len(events_received) == 1 Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential race condition where more than one event could be received, making the subsequent assertion assert len(events_received) == 1 fail. Changing the wait condition to len(events_received) == 1 makes the test more robust and precise.	Medium
	Remove redundant exception suppression Remove the outer try-except Exception: pass block in ensure_valid_window to avoid silently suppressing all exceptions, or at least log them. py/conftest.py [354-365] def ensure_valid_window(): - try: - driver = selenium_driver._driver - if driver: - try: - # Check if current window is still valid - driver.current_window_handle - except Exception: - # restart driver - selenium_driver.stop_driver() - except Exception: - pass + driver = selenium_driver._driver + if driver: + try: + # Check if current window is still valid + driver.current_window_handle + except Exception: + # restart driver + selenium_driver.stop_driver() Apply / Chat Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that swallowing all exceptions with except Exception: pass can hide issues, but it overlooks that this code is in a pytest finalizer, where suppressing exceptions is often a deliberate choice to ensure test suite stability.	Low
Update

[cgoldberg]

As long as all tests pass and we are cleaning up everything, this should be a pretty huge improvement in the time it takes to run tests.

[navin772]

As long as all tests pass and we are cleaning up everything, this should be a pretty huge improvement in the time it takes to run tests.

Yup, all the tests ran much quicker than ever except the 2 windows tests, I don't know what's wrong with them. Maybe, just a windows thing!

Edit: there is significant reduction in windows tests too when compared to previous runs, from 1h40m to 37m approx.