Skip to content

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented Oct 21, 2025

User description

Tweak logic on the auth handler to improve errors.

  • Should we error if both block and username/password are sent? The block is currently ignored which seemed confusing to me, but the username, password are ignored if it just grabs the block, so maybe it needs another tweak.

  • The rescue in #send_cmd is mostly to rescue timeout errors and make them WebDriverError; maybe we should only be checking for those instead of all StandardError? What do you think?


PR Type

Enhancement, Bug fix


Description

  • Improve error handling in BiDi command sending with StandardError rescue

  • Enforce explicit authentication handler configuration validation

  • Raise ArgumentError when neither block nor credentials provided

  • Wrap caught errors as WebDriverError with descriptive messages


Diagram Walkthrough

flowchart LR
  A["send_cmd method"] -->|"catches StandardError"| B["Wraps as WebDriverError"]
  C["add_authentication_handler"] -->|"validates input"| D["block_given?"]
  D -->|"yes"| E["Use block"]
  D -->|"no"| F["Check credentials"]
  F -->|"both provided"| G["Use credentials"]
  F -->|"missing"| H["Raise ArgumentError"]
Loading

File Walkthrough

Relevant files
Error handling
bidi.rb
Add error handling rescue in send_cmd method                         

rb/lib/selenium/webdriver/bidi.rb

  • Added StandardError rescue clause in send_cmd method
  • Catches all StandardError exceptions and converts to WebDriverError
  • Provides descriptive error message including method name and original
    error
+2/-0     
network.rb
Validate authentication handler parameters explicitly       

rb/lib/selenium/webdriver/common/network.rb

  • Refactored add_authentication_handler logic to validate inputs
    explicitly
  • Prioritizes block parameter over username/password credentials
  • Raises ArgumentError if neither block nor both credentials are
    provided
  • Improves clarity by checking block_given? first
+7/-6     

@titusfortner titusfortner requested a review from aguspe October 21, 2025 14:29
@selenium-ci selenium-ci added the C-rb Ruby Bindings label Oct 21, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 21, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Overbroad exception catch

Description: Broadly rescuing StandardError in send_cmd may mask programming errors and convert them to
WebDriverError, potentially hiding actionable exceptions and complicating debugging;
consider narrowing to specific transport/timeouts.
bidi.rb [64-65]

Referred Code
rescue StandardError => e
  raise Error::WebDriverError, "Unable to send command #{method}: #{e.message}"
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

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

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 21, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve original backtrace in rescued exception
Suggestion Impact:The commit changed the re-raise to use `raise e, "..."`, which preserves the original exception and its backtrace, aligning with the suggestion's intent to keep the backtrace.

code diff:

       rescue StandardError => e
-        raise Error::WebDriverError, "Unable to send command #{method}: #{e.message}"
+        raise e, "Unable to send command #{method}: #{e.message}"

Preserve the original exception's backtrace when rescuing a StandardError and
re-raising it as a WebDriverError to improve debuggability.

rb/lib/selenium/webdriver/bidi.rb [64-65]

 rescue StandardError => e
-  raise Error::WebDriverError, "Unable to send command #{method}: #{e.message}"
+  raise Error::WebDriverError, "Unable to send command #{method}: #{e.message}", e.backtrace

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the original backtrace is lost and proposes a valid fix to preserve it, which significantly improves the debuggability of WebSocket communication errors.

Medium
General
Simplify argument validation logic

Refactor the argument validation logic by replacing !(username && password) with
username.nil? || password.nil? for stylistic preference.

rb/lib/selenium/webdriver/common/network.rb [48-54]

 selected_block = if block_given?
                    block
-                 elsif !(username && password)
+                 elsif username.nil? || password.nil?
                    raise ArgumentError, 'Need to provide either a block or both username and password'
                  else
                    proc { |auth| auth.authenticate(username, password) }
                  end
  • Apply / Chat
Suggestion importance[1-10]: 2

__

Why: The suggestion proposes a functionally equivalent change from !(username && password) to username.nil? || password.nil?, which is a minor stylistic preference with negligible impact on readability or maintainability.

Low
  • Update

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