Skip to content

[rb] add low-level BiDi protocol integration specs - #17878

Open
titusfortner wants to merge 10 commits into
trunkfrom
rb-bidi-protocol-specs
Open

[rb] add low-level BiDi protocol integration specs#17878
titusfortner wants to merge 10 commits into
trunkfrom
rb-bidi-protocol-specs

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

  • Adds low-level BiDi Protocol integration specs covering every command across all domains, exercised happy-path against real browsers.
  • Wire-deserialization mismatches now raise a typed SerializationError instead of a bare WebDriverError.

🔧 Implementation Notes

  • Guards assert the specific error the remote returns, so tests notify when behavior changes rather than masking it:
    • UnknownCommandError — command or whole module the browser doesn't recognize
    • UnsupportedOperationError — recognized but not implemented / not permitted
    • SerializationError — malformed or incomplete response (surfaced by strict mode)
  • Runs the suite in strict serialization mode, so a browser response missing a required field fails loudly instead of being silently tolerated as an omitted value.
  • Per-browser gaps are recorded as pending guards that flip to a failure the moment a browser starts supporting the command — a built-in "support landed" signal.
  • Timeout-prone commands are set to skip per browser+OS (not pending), so they don't burn the full timeout every run.
  • Support status verified against each browser's current BiDi implementation (Chrome/Firefox betas, stable Edge). Edge tracks Chromium on an older mapper, so it diverges on a few commands. Safari is immature and guarded separately.

Cells show the error class the guard asserts (linked where a bug is filed); n/a = works. * describes Windows behavior where different (TimeoutError* = works elsewhere but times out on Windows and is skipped there).

BiDi command Chrome Edge Firefox
emulation.setForcedColorsModeThemeOverride UnsupportedOperationError UnsupportedOperationError UnknownCommandError
emulation.setGeolocationOverride (error:) n/a n/a InvalidArgumentError
emulation.setScriptingEnabled n/a n/a UnknownCommandError
emulation.setScrollbarTypeOverride n/a UnknownCommandError UnknownCommandError
emulation.setTouchOverride n/a n/a UnknownCommandError
browsingContext.setBypassCSP UnsupportedOperationError UnknownCommandError UnknownCommandError
browsingContext.startScreencast UnsupportedOperationError UnknownCommandError n/a
browsingContext.reload (ignoreCache: true) n/a n/a UnsupportedOperationError
input.setFiles n/a n/a UnsupportedOperationError
webExtension.install (archivePath / base64) UnsupportedOperationError UnsupportedOperationError n/a
userAgentClientHints.setClientHintsOverride n/a n/a UnknownCommandError
bluetooth.* (entire module) n/a n/a UnknownCommandError
bluetooth.* (device-response commands) TimeoutError* TimeoutError* UnknownCommandError
browser.setDownloadBehavior (file download) TimeoutError* TimeoutError* TimeoutError*
browser.close n/a n/a UnsupportedOperationError
session.end n/a n/a UnsupportedOperationError

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: protocol specs scaffolded from the BiDi schema; browser support status and error classes verified against upstream source and CI runs
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • Safari's BiDi is immature — under strict mode most of its commands return malformed data (SerializationError) or are unimplemented; those are guarded pending so they signal when Safari catches up.
  • Add test depth:
    • verify the reset/clear branch actually took effect (re-read state instead of asserting an empty result);
    • add browser-side error responses (invalid or closed context, denied permission);
    • broaden alternate-value coverage (multiple enum variants, both boolean branches).

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@selenium-ci selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Aug 5, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add Ruby BiDi protocol integration specs with strict serialization errors

🧪 Tests ✨ Enhancement ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add BiDi Protocol integration specs covering commands across multiple domains.
• Run the BiDi protocol suite in strict wire-deserialization mode via Bazel.
• Raise typed SerializationError for schema mismatches instead of generic WebDriverError.
Diagram

graph TD
  specs["BiDi protocol integration specs"] --> harness["Bazel rb_integration_test (SE_BIDI_STRICT=true)"] --> browsers["Real browsers (Chrome/Edge/Firefox/Safari)"]
  specs --> protocol["BiDi Protocol Ruby client"] --> ser["Serialization (Record/Union)"] --> serr["Error::SerializationError"]
  browsers --> ser
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Mocked wire-payload contract tests (no browsers)
  • ➕ Much faster and more deterministic than real-browser integration runs
  • ➕ Easier to cover edge cases and rare malformed payloads
  • ➖ Does not validate actual browser/driver behavior or error codes
  • ➖ Higher risk of drifting from real implementations and missing regressions
2. Keep strict mode off; only assert happy-path typing
  • ➕ Fewer CI failures when browsers lag or return incomplete payloads
  • ➕ Lower maintenance burden for per-browser guards
  • ➖ Schema mismatches can be silently tolerated, masking breakages
  • ➖ Weaker signal when browser behavior changes unexpectedly
3. Reuse WebDriverError for deserialization mismatches
  • ➕ No new public error type to document/handle
  • ➕ Less surface-area for downstream exception handling
  • ➖ Conflates local schema/serialization failures with remote protocol errors
  • ➖ Harder for callers and specs to assert the correct failure mode

Recommendation: The current approach (real-browser integration specs + strict serialization mode + typed SerializationError) is the best fit for catching BiDi drift early. It provides a clear separation between remote protocol errors and local schema mismatches, while pending/skip guards keep the suite actionable across uneven browser support.

Files changed (20) +2784 / -43

Enhancement (3) +13 / -9
error.rbIntroduce Error::SerializationError for BiDi schema mismatches +4/-0

Introduce Error::SerializationError for BiDi schema mismatches

• Adds a dedicated SerializationError subclass to represent local BiDi wire (de)serialization failures distinct from protocol error codes.

rb/lib/selenium/webdriver/bidi/error.rb

record.rbRaise SerializationError for record deserialization violations +7/-7

Raise SerializationError for record deserialization violations

• Switches record deserialization failures (wrong wire shape, missing required fields in strict mode, nullability/type mismatches) from WebDriverError to SerializationError.

rb/lib/selenium/webdriver/bidi/serialization/record.rb

union.rbRaise SerializationError for union variant/shape mismatches +2/-2

Raise SerializationError for union variant/shape mismatches

• Updates union deserialization to raise SerializationError when object-only unions receive scalars or when no schema variant matches the inbound payload.

rb/lib/selenium/webdriver/bidi/serialization/union.rb

Tests (13) +2741 / -30
bluetooth_spec.rbAdd Bluetooth protocol integration specs with browser/OS guards +596/-0

Add Bluetooth protocol integration specs with browser/OS guards

• Adds happy-path Bluetooth command coverage (adapter/device simulation, GATT events, characteristic/descriptor operations) plus per-browser pending/skip guards and cleanup hooks.

rb/spec/integration/selenium/webdriver/bidi/protocol/bluetooth_spec.rb

browser_spec.rbAdd Browser domain protocol integration specs (contexts/windows/downloads/close) +180/-0

Add Browser domain protocol integration specs (contexts/windows/downloads/close)

• Covers Browser domain commands including user contexts, client window enumeration/state changes, download behavior, and close handling with Safari/Firefox guards.

rb/spec/integration/selenium/webdriver/bidi/protocol/browser_spec.rb

browsing_context_spec.rbAdd BrowsingContext protocol integration specs for core navigation and tooling +361/-0

Add BrowsingContext protocol integration specs for core navigation and tooling

• Adds broad coverage for browsing context operations (create/activate/close/tree, screenshot/print/navigate/reload, locateNodes, viewport, bypassCSP, screencast, history) with cross-browser pending/skip expectations.

rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb

emulation_spec.rbAdd Emulation domain protocol integration specs with support assertions +251/-0

Add Emulation domain protocol integration specs with support assertions

• Exercises emulation commands (geolocation, locale/timezone, network conditions, screen settings/orientation, scripting/touch/user agent) and asserts expected unsupported/unknown behaviors per browser.

rb/spec/integration/selenium/webdriver/bidi/protocol/emulation_spec.rb

input_spec.rbAdd Input domain protocol integration specs (actions, setFiles) +130/-0

Add Input domain protocol integration specs (actions, setFiles)

• Adds tests for pointer actions, releasing actions, and setting file input values via input.setFiles with known Firefox path limitations guarded.

rb/spec/integration/selenium/webdriver/bidi/protocol/input_spec.rb

network_spec.rbAdd Network domain protocol integration specs (intercepts, collectors, headers) +403/-0

Add Network domain protocol integration specs (intercepts, collectors, headers)

• Covers network intercept lifecycle, request/response continuation, auth challenges, failing/providing responses, data collection retrieval/disown, and extra headers with Safari strict-mode guards.

rb/spec/integration/selenium/webdriver/bidi/protocol/network_spec.rb

permissions_spec.rbAdd Permissions domain protocol integration specs (setPermission) +103/-0

Add Permissions domain protocol integration specs (setPermission)

• Validates permission setting (geolocation) and supports embedded origin/user context parameters with Safari guards for strict deserialization gaps.

rb/spec/integration/selenium/webdriver/bidi/protocol/permissions_spec.rb

script_spec.rbAdd Script domain protocol integration specs (evaluate/call/disown/preload/realms) +197/-0

Add Script domain protocol integration specs (evaluate/call/disown/preload/realms)

• Adds coverage for common Script commands and options (preload scripts, callFunction, evaluate with ownership/serialization, disown, getRealms) with Safari strict deserialization guards.

rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb

session_spec.rbAdd Session domain protocol integration specs (status/subscribe/unsubscribe/end) +111/-0

Add Session domain protocol integration specs (status/subscribe/unsubscribe/end)

• Exercises session status and subscription lifecycle plus negative coverage for session.new on established sessions; includes Safari strict-mode and Firefox classic-session limitations.

rb/spec/integration/selenium/webdriver/bidi/protocol/session_spec.rb

storage_spec.rbAdd Storage domain protocol integration specs for cookie operations +153/-0

Add Storage domain protocol integration specs for cookie operations

• Adds tests for setting, reading, and deleting cookies (including partition descriptors) and guards known Safari internal-error behavior.

rb/spec/integration/selenium/webdriver/bidi/protocol/storage_spec.rb

user_agent_client_hints_spec.rbAdd UserAgentClientHints protocol integration specs (override) +107/-0

Add UserAgentClientHints protocol integration specs (override)

• Covers client hints override behavior and user-context filtering, with Firefox/Safari unknown-command and strict-mode guards.

rb/spec/integration/selenium/webdriver/bidi/protocol/user_agent_client_hints_spec.rb

web_extension_spec.rbAdd WebExtension protocol integration specs (install/uninstall) +133/-0

Add WebExtension protocol integration specs (install/uninstall)

• Adds extension install/uninstall tests supporting directory, archive path, and base64 payloads; includes Chromium limitations and Safari unknown-command guards.

rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb

protocol_browsing_context_spec.rbRefocus handleUserPrompt tests to assert payload acceptance without UI prompts +16/-30

Refocus handleUserPrompt tests to assert payload acceptance without UI prompts

• Replaces interactive alert/prompt flows with assertions that BiDi handle_user_prompt requests are accepted and return NoSuchAlertError when no prompt is open.

rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb

Other (4) +30 / -4
ci-ruby.ymlRun Edge BiDi-tagged Ruby CI jobs on Windows +1/-1

Run Edge BiDi-tagged Ruby CI jobs on Windows

• Adjusts the Windows job tag filters to include Edge BiDi-targeted runs alongside existing beta/local browser tags.

.github/workflows/ci-ruby.yml

BUILD.bazelInclude BiDi protocol integration spec targets in Bazel test graph +1/-0

Include BiDi protocol integration spec targets in Bazel test graph

• Registers the new rb/spec/integration/selenium/webdriver/bidi/protocol filegroup so the protocol specs are built and discoverable in Bazel.

rb/spec/BUILD.bazel

BUILD.bazelAdd Bazel targets for BiDi protocol integration specs (strict mode) +24/-0

Add Bazel targets for BiDi protocol integration specs (strict mode)

• Defines Bazel integration test targets for each *_spec.rb in the BiDi protocol directory, enabling BiDi-only execution with SE_BIDI_STRICT=true and required data deps.

rb/spec/integration/selenium/webdriver/bidi/protocol/BUILD.bazel

tests.bzlAllow per-test env overrides and merge into browser env +4/-3

Allow per-test env overrides and merge into browser env

• Extends rb_integration_test to accept an env map and merges it into each generated Bazel test rule, enabling suites (like BiDi protocol specs) to force strict serialization mode.

rb/spec/tests.bzl

@qodo-code-review

qodo-code-review Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Unit specs expect WebDriverError ✓ Resolved 📘 Rule violation ≡ Correctness
Description
The serialization layer now raises Error::SerializationError for schema/wire mismatches, but
existing unit specs still assert Error::WebDriverError, so the test suite (and downstream
expectations) will be inconsistent with the new behavior. Update unit tests (and any docs/examples)
to expect SerializationError where applicable.
Code

rb/lib/selenium/webdriver/bidi/serialization/record.rb[R229-232]

            def missing_required(field)
              message = "#{name}##{field.name} is required but was missing from the response"
-              raise Error::WebDriverError, message if Serialization.strict?
+              raise Error::SerializationError, message if Serialization.strict?
Evidence
PR Compliance ID 3 requires updating tests alongside behavior changes. The PR changes strict
deserialization to raise Error::SerializationError (e.g., missing_required), while unit specs
still assert raise_error(Error::WebDriverError, ...), so tests and expectations are no longer
aligned with the new behavior.

AGENTS.md: Write/Update Tests for Fixes and Prefer Small Unit Tests Over Browser Tests
rb/lib/selenium/webdriver/bidi/serialization/record.rb[225-234]
rb/lib/selenium/webdriver/bidi/serialization/union.rb[59-70]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[78-88]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[644-650]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The BiDi serialization code now raises `Error::SerializationError`, but unit tests still expect `Error::WebDriverError`, causing failing/incorrect assertions.

## Issue Context
This PR changes strict deserialization failures (and other schema mismatches) from `Error::WebDriverError` to the new typed `Error::SerializationError`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/serialization/record.rb[229-233]
- rb/lib/selenium/webdriver/bidi/serialization/union.rb[59-70]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[78-88]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[603-650]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unqualified Safari pending guard ✗ Dismissed 🐞 Bug ☼ Reliability ⭐ New
Description
In Script#call_function specs, the new Safari pending_if guards omit an exception: matcher, so
any Safari failure in those examples is treated as expected and won’t signal behavior changes. This
bypasses the guard framework’s deferred exception validation path and can hide real regressions or
infrastructure errors under a pending status.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb[R85-87]

+            it 'calls a function with local value arguments',
+               pending_if: {browser_family: :safari,
+                            reason: 'Safari remote value fails deserialization'} do
Evidence
The new specs add Safari pending_if guards without exception:, and the guard framework marks
such guards pending up-front (without validating the failure) because Guard#exception? is false
when :exception is missing. Only exception-qualified pending guards are deferred and validated via
pending_exception_guard/resolve_pending_exception.

rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb[84-115]
rb/lib/selenium/webdriver/support/guards.rb[51-65]
rb/lib/selenium/webdriver/support/guards/guard.rb[87-100]
rb/spec/integration/selenium/webdriver/spec_helper.rb[64-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The two new Safari `pending_if` guards in `Script#call_function` are unconditional (no `exception:`), so any failure is marked pending instead of validating that Safari fails in the expected way.

### Issue Context
The integration spec harness supports *exception-qualified* pending guards that only apply when the observed failure matches the expected class/message; otherwise the example is treated as a real failure.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb[85-115]

### Suggested fix
- Update both `pending_if` entries to include `exception: { class: ..., message: ... }` for the known Safari failure mode.
 - For the “fails deserialization” case, prefer matching `Error::SerializationError` (and add a message regex once you’ve captured the actual error text from Safari).
 - For the “unexpected result” case, either:
   - make Safari return a known protocol error and guard on that exception, or
   - (less ideal) guard on `RSpec::Expectations::ExpectationNotMetError` with a targeted message regex so only the known mismatch is treated as pending.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Probe uninstalls nonexistent extension ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The Safari web extensions support probe calls webExtension.uninstall with a hard-coded id that is
never installed, so once Safari implements the module the probe will likely still fail with a “no
such extension” style error instead of going green.
This prevents the probe from reliably detecting that WebExtension support landed.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[R89-90]

+            expect(WebExtension.new(driver).uninstall(extension: 'ruby-bidi-probe')).to be_empty
+          end
Evidence
Existing integration coverage shows uninstall is performed against an installed extension id, and
the protocol implementation has no built-in mechanism to resolve or create the extension id being
uninstalled.

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[86-90]
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb[81-89]
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb[117-126]
rb/lib/selenium/webdriver/bidi/protocol/web_extension.rb[104-110]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Safari WebExtension support probe uninstalls a hard-coded extension id (`'ruby-bidi-probe'`) without ever installing it. If/when Safari implements `webExtension.uninstall`, a compliant implementation will likely error for an unknown extension id, so the probe will not flip to “pending fixed” when support lands.

### Issue Context
- The real integration tests install an extension and then uninstall using the returned `result.extension` id.
- `WebExtension#uninstall` is a direct protocol call that accepts only the provided `extension` string.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[86-90]

### Suggested change
Update the probe to install a known test extension from `//common/extensions` first, capture the returned id, then uninstall it:
- Build the extension path similarly to `web_extension_spec.rb` (e.g., `File.expand_path("../../../../../../../common/extensions/webextensions-selenium-example-signed", __dir__)`).
- `id = WebExtension.new(driver).install(extension_data: WebExtension::ExtensionPath.new(path: path)).extension`
- `expect(WebExtension.new(driver).uninstall(extension: id)).to be_empty`
- Ensure cleanup in an `ensure` block if needed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Invalid origin in probe 🐞 Bug ≡ Correctness
Description
The Safari permissions support probe passes url_for('blank.html') (a full URL with a path) as
origin, so when Safari implements permissions.setPermission the probe can still fail with
invalid-argument behavior and never signal support.
This breaks the probe’s purpose of flipping from pending to fixed when support lands.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[R63-66]

+            result = Permissions.new(driver).set_permission(
+              descriptor: Permissions::PermissionDescriptor.new(name: 'geolocation'),
+              state: :granted,
+              origin: url_for('blank.html')
Evidence
The probe currently passes a full URL as origin, while the existing permissions integration spec
explicitly uses window.location.origin, and url_for is implemented as a full URL generator via
the app server.

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]
rb/spec/integration/selenium/webdriver/bidi/protocol/permissions_spec.rb[53-65]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[220-222]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Safari permissions support probe uses `origin: url_for('blank.html')`, but `url_for` returns a full URL (including a path). The existing permissions integration spec derives `origin` from `window.location.origin` (origin-only). If Safari implements the command and validates `origin`, the probe may continue failing and will not provide the intended “support landed” signal.

### Issue Context
- The probe should send a *valid* payload so that success indicates module support.
- Existing integration coverage shows `origin` should be `window.location.origin`.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]

### Suggested change
1. Navigate to a known page (e.g., `blank.html`).
2. Compute origin via JS: `driver.execute_script('return window.location.origin')`.
3. Pass that value as `origin:` to `set_permission` (and similarly for `embedded_origin` if used later).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
5. Prompt success path untested ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The PR replaces the existing handle_user_prompt integration tests with assertions that only cover
the "no prompt open" error case, removing success-path coverage for accepting/dismissing a real
alert/prompt. This reduces regression detection for browsingContext.handleUserPrompt behavior
(including user_text handling) when a prompt is actually present.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[R87-90]

+          it 'returns a no such alert error when no prompt is open' do
+            expect {
+              browsing_context.handle_user_prompt(context: driver.window_handle, accept: true)
+            }.to raise_error(Error::NoSuchAlertError)
Evidence
The updated high-level spec only asserts NoSuchAlertError without opening any prompt, and the new
low-level protocol spec similarly only tests the no-prompt error path, leaving no integration
coverage for successful prompt handling.

rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[87-107]
rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[154-170]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`handle_user_prompt` specs no longer exercise a real JavaScript alert/prompt flow (open dialog → call `handle_user_prompt` → verify the dialog closes / text is submitted). The new tests only assert `NoSuchAlertError` when no prompt is open, which can’t catch regressions in the actual prompt-handling behavior.

### Issue Context
This change appears to have been made to avoid known browser-specific failures, but it removed all visible success-path coverage from the Ruby BiDi integration specs.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[87-107]
- rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[154-170]

### Suggested fix
Reintroduce at least one success-path test that:
- Navigates to `alerts.html`, opens an alert/prompt.
- Calls `browsing_context.handle_user_prompt(...)`.
- Waits for the prompt to close and asserts expected page state.

If specific browsers are currently broken, gate only those browsers behind existing `pending_if`/`skip_if` guards rather than removing the success-path entirely for all browsers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Previous review results

Review updated until commit 8094039

Results up to commit a6e40d6 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Unit specs expect WebDriverError ✓ Resolved 📘 Rule violation ≡ Correctness
Description
The serialization layer now raises Error::SerializationError for schema/wire mismatches, but
existing unit specs still assert Error::WebDriverError, so the test suite (and downstream
expectations) will be inconsistent with the new behavior. Update unit tests (and any docs/examples)
to expect SerializationError where applicable.
Code

rb/lib/selenium/webdriver/bidi/serialization/record.rb[R229-232]

            def missing_required(field)
              message = "#{name}##{field.name} is required but was missing from the response"
-              raise Error::WebDriverError, message if Serialization.strict?
+              raise Error::SerializationError, message if Serialization.strict?
Evidence
PR Compliance ID 3 requires updating tests alongside behavior changes. The PR changes strict
deserialization to raise Error::SerializationError (e.g., missing_required), while unit specs
still assert raise_error(Error::WebDriverError, ...), so tests and expectations are no longer
aligned with the new behavior.

AGENTS.md: Write/Update Tests for Fixes and Prefer Small Unit Tests Over Browser Tests
rb/lib/selenium/webdriver/bidi/serialization/record.rb[225-234]
rb/lib/selenium/webdriver/bidi/serialization/union.rb[59-70]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[78-88]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[644-650]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The BiDi serialization code now raises `Error::SerializationError`, but unit tests still expect `Error::WebDriverError`, causing failing/incorrect assertions.

## Issue Context
This PR changes strict deserialization failures (and other schema mismatches) from `Error::WebDriverError` to the new typed `Error::SerializationError`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/serialization/record.rb[229-233]
- rb/lib/selenium/webdriver/bidi/serialization/union.rb[59-70]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[78-88]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[603-650]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
2. Prompt success path untested ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The PR replaces the existing handle_user_prompt integration tests with assertions that only cover
the "no prompt open" error case, removing success-path coverage for accepting/dismissing a real
alert/prompt. This reduces regression detection for browsingContext.handleUserPrompt behavior
(including user_text handling) when a prompt is actually present.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[R87-90]

+          it 'returns a no such alert error when no prompt is open' do
+            expect {
+              browsing_context.handle_user_prompt(context: driver.window_handle, accept: true)
+            }.to raise_error(Error::NoSuchAlertError)
Evidence
The updated high-level spec only asserts NoSuchAlertError without opening any prompt, and the new
low-level protocol spec similarly only tests the no-prompt error path, leaving no integration
coverage for successful prompt handling.

rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[87-107]
rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[154-170]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`handle_user_prompt` specs no longer exercise a real JavaScript alert/prompt flow (open dialog → call `handle_user_prompt` → verify the dialog closes / text is submitted). The new tests only assert `NoSuchAlertError` when no prompt is open, which can’t catch regressions in the actual prompt-handling behavior.

### Issue Context
This change appears to have been made to avoid known browser-specific failures, but it removed all visible success-path coverage from the Ruby BiDi integration specs.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb[87-107]
- rb/spec/integration/selenium/webdriver/bidi/protocol/browsing_context_spec.rb[154-170]

### Suggested fix
Reintroduce at least one success-path test that:
- Navigates to `alerts.html`, opens an alert/prompt.
- Calls `browsing_context.handle_user_prompt(...)`.
- Waits for the prompt to close and asserts expected page state.

If specific browsers are currently broken, gate only those browsers behind existing `pending_if`/`skip_if` guards rather than removing the success-path entirely for all browsers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 4f5d62b ⚖️ Balanced


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Invalid origin in probe 🐞 Bug ≡ Correctness
Description
The Safari permissions support probe passes url_for('blank.html') (a full URL with a path) as
origin, so when Safari implements permissions.setPermission the probe can still fail with
invalid-argument behavior and never signal support.
This breaks the probe’s purpose of flipping from pending to fixed when support lands.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[R63-66]

+            result = Permissions.new(driver).set_permission(
+              descriptor: Permissions::PermissionDescriptor.new(name: 'geolocation'),
+              state: :granted,
+              origin: url_for('blank.html')
Evidence
The probe currently passes a full URL as origin, while the existing permissions integration spec
explicitly uses window.location.origin, and url_for is implemented as a full URL generator via
the app server.

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]
rb/spec/integration/selenium/webdriver/bidi/protocol/permissions_spec.rb[53-65]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[220-222]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Safari permissions support probe uses `origin: url_for('blank.html')`, but `url_for` returns a full URL (including a path). The existing permissions integration spec derives `origin` from `window.location.origin` (origin-only). If Safari implements the command and validates `origin`, the probe may continue failing and will not provide the intended “support landed” signal.

### Issue Context
- The probe should send a *valid* payload so that success indicates module support.
- Existing integration coverage shows `origin` should be `window.location.origin`.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]

### Suggested change
1. Navigate to a known page (e.g., `blank.html`).
2. Compute origin via JS: `driver.execute_script('return window.location.origin')`.
3. Pass that value as `origin:` to `set_permission` (and similarly for `embedded_origin` if used later).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Probe uninstalls nonexistent extension ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The Safari web extensions support probe calls webExtension.uninstall with a hard-coded id that is
never installed, so once Safari implements the module the probe will likely still fail with a “no
such extension” style error instead of going green.
This prevents the probe from reliably detecting that WebExtension support landed.
Code

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[R89-90]

+            expect(WebExtension.new(driver).uninstall(extension: 'ruby-bidi-probe')).to be_empty
+          end
Evidence
Existing integration coverage shows uninstall is performed against an installed extension id, and
the protocol implementation has no built-in mechanism to resolve or create the extension id being
uninstalled.

rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[86-90]
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb[81-89]
rb/spec/integration/selenium/webdriver/bidi/protocol/web_extension_spec.rb[117-126]
rb/lib/selenium/webdriver/bidi/protocol/web_extension.rb[104-110]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Safari WebExtension support probe uninstalls a hard-coded extension id (`'ruby-bidi-probe'`) without ever installing it. If/when Safari implements `webExtension.uninstall`, a compliant implementation will likely error for an unknown extension id, so the probe will not flip to “pending fixed” when support lands.

### Issue Context
- The real integration tests install an extension and then uninstall using the returned `result.extension` id.
- `WebExtension#uninstall` is a direct protocol call that accepts only the provided `extension` string.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[86-90]

### Suggested change
Update the probe to install a known test extension from `//common/extensions` first, capture the returned id, then uninstall it:
- Build the extension path similarly to `web_extension_spec.rb` (e.g., `File.expand_path("../../../../../../../common/extensions/webextensions-selenium-example-signed", __dir__)`).
- `id = WebExtension.new(driver).install(extension_data: WebExtension::ExtensionPath.new(path: path)).extension`
- `expect(WebExtension.new(driver).uninstall(extension: id)).to be_empty`
- Ensure cleanup in an `ensure` block if needed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread rb/lib/selenium/webdriver/bidi/serialization/record.rb
Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol_browsing_context_spec.rb Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 187b2c4

Comment on lines +63 to +66
result = Permissions.new(driver).set_permission(
descriptor: Permissions::PermissionDescriptor.new(name: 'geolocation'),
state: :granted,
origin: url_for('blank.html')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remediation recommended

1. Invalid origin in probe 🐞 Bug ≡ Correctness

The Safari permissions support probe passes url_for('blank.html') (a full URL with a path) as
origin, so when Safari implements permissions.setPermission the probe can still fail with
invalid-argument behavior and never signal support.
This breaks the probe’s purpose of flipping from pending to fixed when support lands.
Agent Prompt
### Issue description
The Safari permissions support probe uses `origin: url_for('blank.html')`, but `url_for` returns a full URL (including a path). The existing permissions integration spec derives `origin` from `window.location.origin` (origin-only). If Safari implements the command and validates `origin`, the probe may continue failing and will not provide the intended “support landed” signal.

### Issue Context
- The probe should send a *valid* payload so that success indicates module support.
- Existing integration coverage shows `origin` should be `window.location.origin`.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/protocol/safari_support_probe_spec.rb[60-68]

### Suggested change
1. Navigate to a known page (e.g., `blank.html`).
2. Compute origin via JS: `driver.execute_script('return window.location.origin')`.
3. Pass that value as `origin:` to `set_permission` (and similarly for `embedded_origin` if used later).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 4f5d62b

Comment thread rb/spec/integration/selenium/webdriver/bidi/protocol/script_spec.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 8094039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants