Skip to content

Conversation

@asolntsev
Copy link
Contributor

@asolntsev asolntsev commented Dec 6, 2025

User description

🔗 Related Issues

e.g. failed build: https://github.com/SeleniumHQ/selenium/actions/runs/19987626355/job/57324241360?pr=13739

💥 What does this PR do?

Fixes few flaky Ruby tests

🔧 Implementation Notes

This PR adds few "waits" to Ruby tests.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Adds message_provider option to Wait class for better error messages

  • Replaces hardcoded sleeps with proper wait conditions in Ruby tests

  • Improves wait_for_element helper with timeout parameter and detailed error output

  • Refactors devtools and element specs to use wait helpers instead of sleep


Diagram Walkthrough

flowchart LR
  A["Wait class"] -->|adds message_provider| B["Better error messages"]
  C["Test helpers"] -->|enhance wait_for_element| D["Timeout + detailed output"]
  E["Ruby tests"] -->|replace sleep with wait| F["Reliable test execution"]
  B --> G["Reduced flakiness"]
  D --> G
  F --> G
Loading

File Walkthrough

Relevant files
Enhancement
wait.rb
Add message_provider option to Wait class                               

rb/lib/selenium/webdriver/common/wait.rb

  • Adds message_provider option to constructor for custom error message
    generation
  • Updates until method to call message_provider.call when provided
  • Allows dynamic error messages based on application state at timeout
+4/-1     
helpers.rb
Improve wait helpers with better error messages and flexibility

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb

  • Enhances wait_for_element with optional timeout parameter and detailed
    error messages
  • Adds message_provider lambda that includes page URL and HTML source on
    timeout
  • Renames wait_for_new_url to wait_for_url with simplified logic
  • Adds new wait_for_title helper method for title-based waits
+13/-5   
Bug fix
element_spec.rb
Replace direct element finding with wait helpers                 

rb/spec/integration/selenium/webdriver/element_spec.rb

  • Replaces direct find_element calls with wait_for_element helper
    throughout
  • Removes hardcoded sleep calls and replaces with wait_for_url helper
  • Adds timeout parameter to wait_for_element for element click
    intercepted test
  • Improves reliability of element interaction tests by ensuring elements
    are rendered
+55/-47 
devtools_spec.rb
Remove sleeps and improve log event test reliability         

rb/spec/integration/selenium/webdriver/devtools_spec.rb

  • Removes hardcoded sleep calls between console.log executions
  • Changes log collection to store only log.args[0] instead of full log
    object
  • Replaces object attribute matching with direct value inclusion checks
  • Adds wait.until condition to ensure all logs are collected before
    assertions
+7/-13   

@asolntsev asolntsev requested a review from p0deje December 6, 2025 20:49
@selenium-ci selenium-ci added the C-rb Ruby Bindings label Dec 6, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 6, 2025

PR Compliance Guide 🔍

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
🎫 No ticket provided
  • Create ticket/issue
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:
No audit logs: The added test code performs actions and assertions without introducing any audit logging
for critical actions, though as test code this may be acceptable and outside the scope of
production audit trails.

Referred Code
it 'notifies about log messages' do
  logs = []
  driver.on_log_event(:console) { |log| logs.push(log.args[0]) }
  driver.navigate.to url_for('javascriptPage.html')

  driver.execute_script("console.log('I like cheese');")
  driver.execute_script('console.log(true);')
  driver.execute_script('console.log(null);')
  driver.execute_script('console.log(undefined);')
  driver.execute_script('console.log(document);')

  wait.until { logs.include?(true) }
  wait.until { logs.include?(nil) }
  wait.until { logs.include?({ 'type' => 'undefined' }) }
  wait.until { logs.include?('I like cheese')}
end

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:
Minimal wait handling: The new wait_for_title helper lacks explicit error context on timeout, relying on generic
wait exceptions which may hinder debugging though this may be standard in test utilities.

Referred Code
def wait_for_title(title:)
  wait = Wait.new(timeout: 5)
  wait.until { driver.title == title }
end

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Consolidate multiple waits into one

Consolidate multiple wait.until calls into a single block. This can be achieved
by defining an array of expected logs and waiting until all of them are present
in the logs array.

rb/spec/integration/selenium/webdriver/devtools_spec.rb [112-115]

-wait.until { logs.include?(true) }
-wait.until { logs.include?(nil) }
-wait.until { logs.include?({ 'type' => 'undefined' }) }
-wait.until { logs.include?('I like cheese')}
+expected_logs = [true, nil, { 'type' => 'undefined' }, 'I like cheese']
+wait.until { (expected_logs - logs).empty? }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly proposes consolidating multiple wait.until calls into a single, more efficient and readable check, which is a good practice for improving test code quality.

Low
Learned
best practice
Replace hardcoded wait timeout

Use a shared, configurable timeout (and a shorter poll interval) instead of a
hardcoded 5 seconds to keep waits consistent and adjustable across tests.

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb [105-108]

-def wait_for_title(title:)
-  wait = Wait.new(timeout: 5)
-  wait.until { driver.title == title }
+DEFAULT_WAIT_TIMEOUT = 10 # seconds
+
+def wait_for_title(title:, timeout: DEFAULT_WAIT_TIMEOUT, interval: 0.1)
+  Wait.new(timeout: timeout, interval: interval).until { driver.title == title }
 end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Guard time-based waits by computing and clamping timeouts and centralizing timeout values to avoid brittle tests.

Low
  • Update

@asolntsev asolntsev force-pushed the fix/flaky-ruby-tests-2 branch 2 times, most recently from 275cf06 to 92fbd13 Compare December 6, 2025 22:42
@asolntsev asolntsev marked this pull request as draft December 6, 2025 22:43
@asolntsev asolntsev force-pushed the fix/flaky-ruby-tests-2 branch 2 times, most recently from 830dd21 to a7512f2 Compare December 7, 2025 19:12
Wait until the page gets loaded and the element ges rendered.

Example of failure:

```ruby
Selenium::WebDriver::Element raises if different element receives click
     Failure/Error: expect { driver.find_element(id: 'contents').click }.to raise_error(Error::ElementClickInterceptedError)

       expected Selenium::WebDriver::Error::ElementClickInterceptedError, got #<Selenium::WebDriver::Error::NoSuchElementError:"no such element: Unable to locate element: {\"metho...it: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#nosuchelementexception"> with backtrace:
         # ./rb/lib/selenium/webdriver/remote/response.rb:63:in `add_cause'
         # ./rb/lib/selenium/webdriver/remote/response.rb:41:in `error'
         # ./rb/lib/selenium/webdriver/remote/response.rb:52:in `assert_ok'
         # ./rb/lib/selenium/webdriver/remote/response.rb:34:in `initialize'
         # ./rb/lib/selenium/webdriver/remote/http/common.rb:103:in `new'
         # ./rb/lib/selenium/webdriver/remote/http/common.rb:103:in `create_response'
         # ./rb/lib/selenium/webdriver/remote/http/default.rb:103:in `request'
         # ./rb/lib/selenium/webdriver/remote/http/common.rb:68:in `call'
         # ./rb/lib/selenium/webdriver/remote/bridge.rb:625:in `execute'
         # ./rb/lib/selenium/webdriver/remote/bridge.rb:493:in `find_element_by'
         # ./rb/lib/selenium/webdriver/common/search_context.rb:71:in `find_element'
         # ./rb/spec/integration/selenium/webdriver/element_spec.rb:34:in `block (3 levels) in <module:WebDriver>'
         # ./rb/spec/integration/selenium/webdriver/element_spec.rb:34:in `block (2 levels) in <module:WebDriver>'
     # ./rb/spec/integration/selenium/webdriver/element_spec.rb:34:in `block (2 levels) in <module:WebDriver>'
```

See https://github.com/SeleniumHQ/selenium/actions/runs/19987626355/job/57324241360?pr=13739 for example.
instead of sleeping for 0.5s, just wait for the needed URL.

NB! The previous helper method `wait_for_new_url(old_url)` caused flaky tests because in Firefox, browsers changes URLs like this: "<old url>" -> "about:blank" -> "<new url>".
instead of sleeping for 2.5s, just wait for the needed logs.
@asolntsev asolntsev force-pushed the fix/flaky-ruby-tests-2 branch from a7512f2 to 6efb912 Compare December 7, 2025 22:46
@asolntsev asolntsev marked this pull request as ready for review December 8, 2025 05:25
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

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
🎫 No ticket provided
  • Create ticket/issue
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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

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: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Sensitive Exposure: The new wait_for_element message_provider includes full page HTML source and current URL
in error messages, potentially exposing sensitive internal details to users of test
output.

Referred Code
def wait_for_element(locator, timeout = 25)
  wait = Wait.new(timeout: timeout, ignore: Error::NoSuchElementError, message_provider: lambda {
    url = "page url: #{driver.current_url};\n"
    source = "page source: #{driver.find_element(css: 'body').attribute('innerHTML')}\n"
    "could not find element #{locator} in #{timeout} seconds;\n#{url}#{source}"
  })
  wait.until { driver.find_element(locator) }
end

def wait_for_url(new_url)
  wait = Wait.new(timeout: 5)
  wait.until do
    driver.current_url.include?(new_url)
  end
end

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:
No Audit Logs: The added waiting logic and test helpers introduce no logging of critical actions, but
since these are test utilities and not production-critical actions, applicability of audit
trail requirements is unclear.

Referred Code
def initialize(opts = {})
  @timeout  = opts.fetch(:timeout, DEFAULT_TIMEOUT)
  @interval = opts.fetch(:interval, DEFAULT_INTERVAL)
  @message  = opts[:message]
  @message_provider = opts[:message_provider]
  @ignored = Array(opts[:ignore] || Error::NoSuchElementError)
end

#
# Wait until the given block returns a true value.
#
# @raise [Error::TimeoutError]
# @return [Object] the result of the block
#

def until
  end_time = current_time + @timeout
  last_error = nil

  until current_time > end_time
    begin


 ... (clipped 17 lines)

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:
Verbose Logging: Error messages constructed in message_provider capture and emit the entire page source and
URL, which could be written to logs by the test framework and may include sensitive data
depending on pages under test.

Referred Code
def wait_for_element(locator, timeout = 25)
  wait = Wait.new(timeout: timeout, ignore: Error::NoSuchElementError, message_provider: lambda {
    url = "page url: #{driver.current_url};\n"
    source = "page source: #{driver.find_element(css: 'body').attribute('innerHTML')}\n"
    "could not find element #{locator} in #{timeout} seconds;\n#{url}#{source}"
  })
  wait.until { driver.find_element(locator) }
end

def wait_for_url(new_url)
  wait = Wait.new(timeout: 5)
  wait.until do
    driver.current_url.include?(new_url)
  end
end

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

@asolntsev asolntsev merged commit b2cfa18 into SeleniumHQ:trunk Dec 8, 2025
22 checks passed
@asolntsev asolntsev deleted the fix/flaky-ruby-tests-2 branch December 8, 2025 05:25
@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Improve error message generation resilience

Add begin/rescue blocks within the message_provider lambda to prevent crashes
when fetching the current URL or page source for the error message.

rb/spec/integration/selenium/webdriver/spec_support/helpers.rb [87-94]

 def wait_for_element(locator, timeout = 25)
   wait = Wait.new(timeout: timeout, ignore: Error::NoSuchElementError, message_provider: lambda {
-    url = "page url: #{driver.current_url};\n"
-    source = "page source: #{driver.find_element(css: 'body').attribute('innerHTML')}\n"
+    url = begin
+            "page url: #{driver.current_url};\n"
+          rescue StandardError
+            "page url: <not available>;\n"
+          end
+    source = begin
+               "page source: #{driver.page_source}\n"
+             rescue StandardError
+               "page source: <not available>\n"
+             end
     "could not find element #{locator} in #{timeout} seconds;\n#{url}#{source}"
   })
   wait.until { driver.find_element(locator) }
 end
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the message_provider lambda can fail, which would mask the original timeout error, and proposes a robust solution using begin/rescue to make the error reporting more resilient.

Medium
Learned
best practice
Safeguard message provider exceptions

Protect calls to a custom message_provider so exceptions there don't hide the
original timeout; rescue and fall back to a default message with a hint.

rb/lib/selenium/webdriver/common/wait.rb [66-72]

 msg = if @message
         @message.dup
       elsif @message_provider
-        @message_provider.call
+        begin
+          @message_provider.call
+        rescue StandardError => e
+          "timed out after #{@timeout} seconds (message_provider failed: #{e.class}: #{e.message})"
+        end
       else
         "timed out after #{@timeout} seconds"
       end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Pattern 1: Ensure resource acquisition in error-message hooks can't raise and mask the original timeout error.

Low
  • More

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants