Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands Selenium Python BiDi test coverage (script/log/integration/error paths) in preparation for moving to CDDL-generated code.
Changes:
- Adds multiple new test suites for BiDi script execution behaviors, logging, integration scenarios, and error handling.
- Introduces new test modules:
bidi_log_tests.py,bidi_integration_tests.py, andbidi_errors_tests.py. - Applies some formatting refactors in existing BiDi script tests and appends additional script-oriented test classes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 32 comments.
| File | Description |
|---|---|
py/test/selenium/webdriver/common/bidi_script_tests.py |
Adds many new script tests and some formatting changes; introduces an unused import and has new blocks that will be reformatted by ruff format. |
py/test/selenium/webdriver/common/bidi_log_tests.py |
New BiDi logging tests; contains an unused import and at least one test that doesn’t assert behavior. |
py/test/selenium/webdriver/common/bidi_integration_tests.py |
New integration tests across modules; currently uses multiple non-existent/incorrect BiDi and WebDriver APIs (would fail at runtime/lint). |
py/test/selenium/webdriver/common/bidi_errors_tests.py |
New BiDi error-handling tests; currently calls many non-existent methods and incorrect signatures (would fail at runtime). |
| class TestBidiJavaScriptErrors: | ||
| """Test class for JavaScript error logging.""" | ||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def setup(self, driver, pages): | ||
| """Setup for each test in this class.""" | ||
| pages.load("blank.html") |
There was a problem hiding this comment.
This class currently only defines an autouse setup fixture and contains no tests. It looks incomplete and adds dead code/maintenance overhead. Either add the intended JavaScript error logging tests here or remove this class (and the autouse fixture).
| def test_cookie_operations(self, driver, pages): | ||
| """Test basic cookie operations.""" | ||
| pages.load("blank.html") | ||
|
|
||
| # Set cookie | ||
| driver.add_cookie({"name": "test", "value": "data"}) | ||
|
|
There was a problem hiding this comment.
These tests mutate the browser cookie jar but don’t clear/restore cookies before starting. If the driver/session is reused, a pre-existing cookie with the same name can turn add_cookie into a replace (making later assertions brittle) and can pollute subsequent tests. Add cookie cleanup (e.g., driver.delete_all_cookies() at the start of each test or an autouse setup fixture for this class).
| user_contexts = [user_context] | ||
|
|
||
| script_id = driver.script._add_preload_script(function_declaration, user_contexts=user_contexts) | ||
| script_id = driver.script._add_preload_script( | ||
| function_declaration, user_contexts=user_contexts | ||
| ) |
There was a problem hiding this comment.
This test creates a new user context and browsing context/window earlier in the function, but it never closes the created browsing context or removes the user context (and doesn’t switch back to the original window). This can leak resources and make later tests flaky. Add a try/finally that closes the browsing context, switches back to the original handle, and calls browser.remove_user_context(user_context).
| """Test adding multiple preload scripts.""" | ||
| id1 = driver.script._add_preload_script("() => { window.test1 = 'loaded'; }") | ||
| id2 = driver.script._add_preload_script("() => { window.test2 = 'loaded'; }") | ||
|
|
There was a problem hiding this comment.
The try/finally starts after both preload scripts are added. If the second _add_preload_script call fails/raises, id1 will be left installed and the test will skip cleanup. Consider starting the try/finally immediately after creating id1 (or initializing id1/id2 to None and conditionally removing in finally) so cleanup always runs for any successfully-added script.
| """Test console message handler with actual logging.""" | ||
| log_entries = [] | ||
| driver.script.add_console_message_handler(log_entries.append) | ||
|
|
||
| try: | ||
| pages.load("bidi/logEntryAdded.html") |
There was a problem hiding this comment.
This test adds a console message handler but never removes it (finally is empty). Please capture the handler id from add_console_message_handler and remove it in finally to keep tests isolated and avoid accumulating handlers over the run.
| @pytest.fixture(autouse=True) | ||
| def setup(self, driver, pages): | ||
| """Setup for each test in this class.""" | ||
| pages.load("blank.html") | ||
|
|
There was a problem hiding this comment.
Cookie state can leak between tests when the same browser session is reused. Other BiDi cookie/storage tests clear cookies in setup (e.g., TestBidiStorage in bidi_storage_tests.py clears via driver.delete_all_cookies) to keep counts/deltas deterministic. Consider adding driver.delete_all_cookies() in this autouse setup (or in each test) before asserting on cookie counts.
| log_entries = [] | ||
| driver.script.add_console_message_handler(log_entries.append) | ||
|
|
||
| try: | ||
| pages.load("bidi/logEntryAdded.html") |
There was a problem hiding this comment.
This test registers a console message handler but doesn’t remove it; the finally block is effectively a no-op. To avoid handler accumulation/flaky cross-test interactions, capture the handler id returned by add_console_message_handler and call remove_console_message_handler(handler_id) in the finally block.
| errors = [] | ||
| driver.script.add_javascript_error_handler(errors.append) | ||
|
|
||
| try: | ||
| pages.load("bidi/logEntryAdded.html") | ||
| # Click element that triggers JS error | ||
| driver.find_element(By.ID, "jsException").click() |
There was a problem hiding this comment.
This test registers a JavaScript error handler but never removes it (the finally block just passes). For isolation and to prevent handler buildup across tests, store the callback id returned by add_javascript_error_handler and call remove_javascript_error_handler(callback_id) in finally.
|
nice 👍 |
|
below is the plan I have created and working against Selenium BiDi Test Coverage Plan - WPT ComparisonPrepared: 2026-03-07 Executive SummarySelenium currently has 10 BiDi test modules while WPT has 13 main test modules with an extensive sub-module structure. Key gaps include:
Detailed Module-by-Module Comparison✅ 1. Browser ModuleFile: WPT Sub-modules:
Selenium Test Coverage:
Missing Tests:
Estimated Gap: 5-10 tests✅ 2. Browsing Context ModuleFile: WPT Sub-modules (32 total):
Selenium Test Coverage:Covered (✓):
Missing/Gaps (✗):
Estimated Gap: 10-15 tests✅ 3. Emulation ModuleFile: WPT Sub-modules (10 total):
Selenium Test Coverage:Likely basic coverage of:
Missing/Gaps:
Estimated Gap: 8-12 tests✅ 4. Input ModuleFile: WPT Sub-modules (4 total):
Selenium Test Coverage:Likely basic coverage of:
Missing/Gaps:
Estimated Gap: 5-10 tests✅ 5. Network ModuleFile: WPT Sub-modules (18 total):
Selenium Test Coverage:Basic coverage likely includes:
Missing/Gaps:
Estimated Gap: 15-20 tests✅ 6. Script ModuleFile: WPT Sub-modules (8 total):
Selenium Test Coverage:Likely basic coverage:
Missing/Gaps:
Estimated Gap: 10-15 tests✅ 7. Session ModuleFile: WPT Sub-modules (5 total):
Selenium Test Coverage:Basic coverage likely includes:
Missing/Gaps:
Estimated Gap: 5-8 tests✅ 8. Storage ModuleFile: WPT Sub-modules (3 total):
Selenium Test Coverage:Basic coverage likely:
Missing/Gaps:
Estimated Gap: 8-12 tests✅ 9. Web Extension ModuleFile: WPT Sub-modules (2 total):
Selenium Test Coverage:Basic coverage:
Missing/Gaps:
Estimated Gap: 4-6 tests❌ Completely Missing Modules10. Errors ModuleFile: MISSING - WPT Content:
What It Should Test:
Estimated Tests Needed: 15-20 testsAction Items:
11. Integration ModuleFile: MISSING - WPT Content:
What It Should Test:
Estimated Tests Needed: 10-15 testsAction Items:
12. Log ModuleFile: MISSING - WPT Content:
What It Should Test:
Estimated Tests Needed: 12-18 testsAction Items:
13. External ModuleStatus: Unknown - appears in WPT but unclear if it contains tests What Needs Investigation:
Additional Module: PermissionsFile: WPT Coverage:Not found in WPT BiDi tests - appears to be Selenium-specific Current Status:
Recommendation:
Summary Table
Overall Gap AssessmentTotal Estimated Missing Tests
Priorities for Implementation🔴 High Priority (Blocks spec compliance)
🟡 Medium Priority (Improves coverage)
🟢 Low Priority (Nice to have)
Recommended Implementation PlanPhase 1: Critical Gaps (2-3 weeks)
Phase 2: Core Coverage Expansion (3-4 weeks)
Phase 3: Edge Cases & Polish (2-3 weeks)
Files to Create/ModifyNew Files to CreateFiles to ExpandBazel Integration - Automatic Test DiscoveryHow New BiDi Tests Are Automatically Picked UpThe Selenium build system automatically includes all BiDi test files without requiring changes to the Bazel configuration. Key Mechanism (py/BUILD.bazel:633): BIDI_TESTS = glob(["test/selenium/webdriver/common/**/*bidi*_tests.py"])This glob pattern automatically matches any file in
Test Target OrganizationNon-BiDi Tests (
BiDi Tests (
Naming Convention for New TestsTo ensure automatic inclusion, follow this pattern: Examples:
Test Execution# Run all BiDi tests for a browser
bazel test //py:test-chrome-bidi
bazel test //py:test-firefox-bidi
bazel test //py:test-edge-bidi
# Run with filter
bazel test //py:test-chrome-bidi --test_filter=test_specific_test_name
# Verify cache after creating new files
bazel clean && bazel test //py:test-chrome-bidiNo Bazel Configuration Changes Required ✅When creating the new test files (
Reference Links
Style and StandardsCode Style ComplianceAll new BiDi tests must follow the established style guide:
Key Style Points
Notes
|
| """Test script execution with console message handler active.""" | ||
| log_entries = [] | ||
| driver.script.add_console_message_handler(log_entries.append) | ||
|
|
There was a problem hiding this comment.
add_console_message_handler returns a callback id that should be removed at the end of the test. In BiDi mode the driver fixture is reused across tests (see py/conftest.py), so leaving the handler registered can leak subscriptions and make later tests flaky (multiple handlers firing). Capture the returned id and call driver.script.remove_console_message_handler(...) in the finally block (or mark the test needs_fresh_driver).
| def error_handler(entry): | ||
| errors.append(entry) | ||
|
|
||
| driver.script.add_javascript_error_handler(error_handler) | ||
|
|
There was a problem hiding this comment.
This test registers a JavaScript error handler but never removes it. Since the BiDi test driver is shared across tests by default, the handler can leak into later tests and change their behavior. Store the handler id returned by add_javascript_error_handler and remove it in finally (or use needs_fresh_driver).
| def test_console_message_with_logging(self, driver, pages): | ||
| """Test console message handler with actual logging.""" | ||
| log_entries = [] | ||
| driver.script.add_console_message_handler(log_entries.append) | ||
|
|
There was a problem hiding this comment.
This test registers a console message handler but never removes it. With the shared BiDi driver, leftover handlers keep the log subscription active and can make later log-related assertions flaky (multiple callbacks). Capture the handler id from add_console_message_handler and call remove_console_message_handler in finally.
| @pytest.fixture(autouse=True) | ||
| def setup(self, driver, pages): | ||
| """Setup for each test in this class.""" | ||
| pages.load("blank.html") | ||
|
|
There was a problem hiding this comment.
The driver fixture is reused across BiDi tests by default, so cookies created here can leak into later tests. Add cookie cleanup (e.g., driver.delete_all_cookies()) in this autouse setup fixture or ensure each test deletes any cookies it creates.
| def test_cookie_operations(self, driver, pages): | ||
| """Test basic cookie operations.""" | ||
| pages.load("blank.html") | ||
|
|
||
| # Set cookie | ||
| driver.add_cookie({"name": "test", "value": "data"}) | ||
|
|
There was a problem hiding this comment.
These tests create cookies but the class has no setup/teardown to clear them, which can pollute the shared BiDi driver state for subsequent tests in this and other files. Add an autouse fixture for this class (or clear cookies at the start/end of each test) to keep the suite isolated.
| """Test script execution with error handler active.""" | ||
| errors = [] | ||
| driver.script.add_javascript_error_handler(errors.append) | ||
|
|
There was a problem hiding this comment.
add_javascript_error_handler also returns an id that should be removed. Because the BiDi driver is reused across tests, this handler can persist and accumulate, causing unexpected extra callbacks in later tests. Capture the id and call driver.script.remove_javascript_error_handler(id) (alias of remove_console_message_handler) in the finally block.
| """Test multiple error handlers can be registered.""" | ||
| errors1 = [] | ||
| errors2 = [] | ||
|
|
||
| driver.script.add_javascript_error_handler(errors1.append) | ||
| driver.script.add_javascript_error_handler(errors2.append) | ||
|
|
There was a problem hiding this comment.
This test registers two JavaScript error handlers but does not remove either one. Because the BiDi driver fixture is reused across tests, both handlers will remain active and can cause later tests to receive extra events. Capture both ids returned by add_javascript_error_handler and remove them in finally.
| def test_delete_cookies_multiple_filters(self, driver, pages, webserver): | ||
| """Test deleting cookies with multiple filter criteria.""" | ||
| assert_no_cookies_are_present(driver) | ||
|
|
||
| key1 = "http_only_delete_test" | ||
| key2 = "normal_delete_test" | ||
| value = BytesValue(BytesValue.TYPE_STRING, "test_value") | ||
|
|
||
| cookie1 = PartialCookie(key1, value, webserver.host, http_only=True) | ||
| cookie2 = PartialCookie(key2, value, webserver.host, http_only=False) | ||
|
|
||
| driver.storage.set_cookie(cookie=cookie1) | ||
| driver.storage.set_cookie(cookie=cookie2) | ||
|
|
||
| # Delete only http_only cookies | ||
| driver.storage.delete_cookies(filter=CookieFilter(name=key1, http_only=True)) | ||
|
|
There was a problem hiding this comment.
test_delete_cookies_multiple_filters claims to validate multiple filter criteria, but using CookieFilter(name=key1, http_only=True) makes the http_only flag effectively irrelevant because the cookie name alone uniquely selects the cookie. To actually exercise the http_only filtering behavior, avoid filtering by name (or add an additional cookie with the same name but different http_only) and assert that only the intended cookies are deleted.
🔗 Related Issues
💥 What does this PR do?
Add more tests to the Python Bidi code to improve coverage for when we move to the CDDL generated code
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes