Skip to content

[rb] Fix failing firefox beta tests for browser context#17199

Open
aguspe wants to merge 3 commits intoSeleniumHQ:trunkfrom
aguspe:rb_fix_firefox_beta_tests
Open

[rb] Fix failing firefox beta tests for browser context#17199
aguspe wants to merge 3 commits intoSeleniumHQ:trunkfrom
aguspe:rb_fix_firefox_beta_tests

Conversation

@aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 10, 2026

🔗 Related Issues

I made this pr #17188, and I keep getting Firefox Beta errors

💥 What does this PR do?

I'm trying to use event subscription that is natively BiDi for the test instead of alert and see if this fixes the errors

🔧 Implementation Notes

I think using the dismiss alert action was messing with the handle user prompt from bidi

🔄 Types of changes

  • Bug fix (backwards compatible)

Copilot AI review requested due to automatic review settings March 10, 2026 22:13
@selenium-ci selenium-ci added C-rb Ruby Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Mar 10, 2026
@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

Fix flaky Firefox Beta tests using BiDi event subscriptions

🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Replace alert-based waiting with native BiDi event subscriptions
• Group related user prompt tests into shared context
• Use BiDi callbacks to detect prompt opening instead of alert helpers
• Add driver reset with ignore unhandled prompt behavior

Grey Divider

File Changes

1. rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb 🐞 Bug fix +43/-35

Replace alert helpers with BiDi event subscriptions

• Refactored three separate user prompt tests into a grouped context with shared setup
• Replaced wait_for_alert and wait_for_no_alert helpers with BiDi event subscription callbacks
• Added reset_driver!(unhandled_prompt_behavior: :ignore) in before hook to prevent prompt
 handling conflicts
• Changed from driver.window_handles.first to driver.window_handle for window reference
• Implemented bridge.bidi.session.subscribe('browsingContext.userPromptOpened') with
 callback-based waiting

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


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 10, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. No prompt-close wait🐞 Bug ⛯ Reliability
Description
The specs call BrowsingContext#handle_user_prompt and then immediately call driver.title without
waiting for the prompt to be dismissed. Since handle_user_prompt is a thin send_cmd wrapper (no
post-condition wait), driver.title can run while a prompt is still open and raise
Error::UnexpectedAlertOpenError.
Code

rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[R99-103]

+            wait.until { prompt_opened }
+            browsing_context.handle_user_prompt(window, accept: true)

-        it 'accepts users prompts with text',
-           except: {browser: %i[edge chrome],
-                    reason: 'https://github.com/GoogleChromeLabs/chromium-bidi/issues/3281'} do
-          browsing_context = described_class.new(bridge)
-          driver.navigate.to url_for('alerts.html')
-          driver.find_element(id: 'prompt').click
-          wait_for_alert
-          window = driver.window_handles.first
-          browsing_context.handle_user_prompt(window, accept: true, text: 'Hello, world!')
-          wait_for_no_alert
-
-          expect(driver.title).to eq('Testing Alerts')
-        end
+            expect(driver.title).to eq('Testing Alerts')
+          end
Evidence
handle_user_prompt only sends the BiDi command and does not wait for prompt dismissal, while the
test suite already has a dedicated helper (wait_for_no_alert) that retries driver.title
specifically because driver.title can fail with UnexpectedAlertOpenError while an alert/prompt
is open. Removing that wait makes the post-handle assertion timing-dependent.

rb/lib/selenium/webdriver/bidi/browsing_context.rb[105-107]
rb/spec/integration/selenium/webdriver/spec_support/helpers.rb[82-85]
rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[99-103]

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` is asynchronous from the perspective of these tests (it only sends a BiDi command). The specs immediately call `driver.title` after handling the prompt, which can still be blocked by an open prompt and raise `Error::UnexpectedAlertOpenError`.
### Issue Context
The suite already contains `wait_for_no_alert`, which explicitly waits for `driver.title` to succeed while ignoring `UnexpectedAlertOpenError`.
### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[90-132]

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



Remediation recommended

2. Double driver restart 🐞 Bug ➹ Performance
Description
The new before hook calls reset_driver! to set unhandled_prompt_behavior: :ignore, but the
spec already resets the driver in an after hook for every example. This causes two full driver
restarts per example in the new context, increasing runtime and increasing exposure to
driver-startup flakiness.
Code

rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[R84-88]

+        context 'user prompts', except: {browser: %i[edge chrome],
+                                          reason: 'https://github.com/GoogleChromeLabs/chromium-bidi/issues/3281'} do
+          before do
+            reset_driver!(unhandled_prompt_behavior: :ignore)
+          end
Evidence
The file-wide after hook resets the driver after every example, and the newly-added before hook
resets again before each user-prompt example. reset_driver! always quits the current driver and
creates a new one, so each example in this context performs two quit+create cycles.

rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[27-28]
rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[86-88]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[75-82]

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

## Issue description
Each user-prompt example currently triggers two full driver restarts: one in the new `before` hook (to set `unhandled_prompt_behavior`) and another in the existing file-level `after` hook.
### Issue Context
`reset_driver!` always quits and recreates the driver, so double resets increase runtime and increase exposure to driver start/stop instability.
### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[25-28]
- rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[84-89]

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


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@aguspe aguspe changed the title Fix flaky tests [rb] Fix failing firefox beta tests for browser context Mar 10, 2026
@aguspe aguspe self-assigned this Mar 10, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates Ruby BiDi integration tests around user prompt handling to reduce flakiness seen on Firefox Beta by waiting on BiDi prompt events instead of relying on legacy alert helpers.

Changes:

  • Refactors the user-prompt specs into a dedicated context that resets the driver with unhandled_prompt_behavior: :ignore.
  • Switches prompt detection to BiDi event subscription (browsingContext.userPromptOpened) before calling handle_user_prompt.
  • Removes wait_for_alert / wait_for_no_alert usage from these BiDi prompt tests.

You can also share your feedback on Copilot code review. Take the survey.

@cgoldberg
Copy link
Member

@aguspe I'm not totally sure, but I think the failures in Firefox Beta are related to: mozilla/geckodriver#2241
Once our CI pulls the updated version of Firefox Beta with this fix, it might just work again

Copilot AI review requested due to automatic review settings March 11, 2026 18:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +89 to 91
it 'accepts user prompts without text' do
browsing_context = described_class.new(bridge)

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

These user-prompt examples previously had an except for Chrome/Edge (linked to chromium-bidi issue #3281). This PR removes that exclusion, so the specs will now run on Chrome/Edge (the outer only includes them) and may reintroduce known failures. If the upstream issue is still relevant, please restore the except metadata (or update it with the correct reason/issue if it’s been resolved).

Copilot uses AI. Check for mistakes.
@aguspe
Copy link
Contributor Author

aguspe commented Mar 11, 2026

@aguspe I'm not totally sure, but I think the failures in Firefox Beta are related to: mozilla/geckodriver#2241
Once our CI pulls the updated version of Firefox Beta with this fix, it might just work again

@cgoldberg thank you so much for the link! I'm trying to give it a try to only use BiDi events and fix these tests to make it more resilient in the future too, and make it less flaky. Maybe it's not the best approach but if it works we will not have to wait until the 24th to try it out :)

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

Labels

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.

4 participants