Skip to content

[rb] Allow symbols again to be passed on delete_cookie #15519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
aguspe merged 3 commits into from
Mar 26, 2025
Merged

[rb] Allow symbols again to be passed on delete_cookie #15519

aguspe merged 3 commits into from
Mar 26, 2025

Conversation

@aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 26, 2025 •
edited
Loading

User description

Motivation and Context

Regarding the issue raised on #15386 by Akarzim deleting a cookie as a symbol fails

This PR adds support for symbols by converting the value to a string on the conditional check

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix, Tests

Description

  • Fixed bug when deleting cookies with symbol names.

  • Updated delete_cookie method to handle symbols correctly.

  • Added test case for deleting cookies using symbols.

  • Modified Bazel configuration for remote test tag filters.

Changes walkthrough 📝

Relevant files
Bug fix
bridge.rb
Fix symbol handling in `delete_cookie` method                       

rb/lib/selenium/webdriver/remote/bridge.rb

  • Updated delete_cookie method to convert symbol names to strings.
  • Ensured proper validation for cookie name input.
    		• +1/-1     
    Tests
    manager_spec.rb
    Add test for deleting cookies with symbols                             

    rb/spec/integration/selenium/webdriver/manager_spec.rb

  • Added test case for deleting cookies using symbol names.
  • Verified that cookies can be deleted and list becomes empty.
    		• +6/-0     
    Configuration changes
    .bazelrc.remote
    Update Bazel remote test tag filters                                         

    .bazelrc.remote

  • Adjusted test tag filters to include exclusive-if-local.
  • Updated configuration for remote testing environment.
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @aguspe aguspe added the C-rb Ruby Bindings label Mar 26, 2025
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 26, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Tag Filter Change

    The PR removes the '-exclusive-if-local' tag filter from remote tests. This might allow tests that should only run locally to be executed in remote environments, potentially causing unexpected failures.

     
    test:remote --test_tag_filters=-skip-rbe,-remote

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 26, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Convert name to string

    The delete_cookie method now converts the name parameter to a string using to_s,
    but it doesn't pass the converted string to the execute method. This could cause
    issues when a symbol is passed as the cookie name.

    rb/lib/selenium/webdriver/remote/bridge.rb [325-329]

     def delete_cookie(name)
   raise ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.to_s.strip.empty?
 
-  execute :delete_cookie, name: name
+  execute :delete_cookie, name: name.to_s
 end
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical inconsistency in the PR. While the validation logic was updated to handle symbol names with to_s, the actual parameter passed to execute wasn't updated, which would cause issues when symbols are used as cookie names (as shown in the new test case).

    		High
    Learned
    best practice
    Ensure consistent type handling by converting parameters to the expected type before use

    The current implementation calls to_s on name in the validation check but
    doesn't ensure the same conversion when passing the name to execute. This could
    lead to inconsistent behavior when symbols are passed as cookie names. Ensure
    the name is consistently converted to string before using it.

    rb/lib/selenium/webdriver/remote/bridge.rb [325-329]

     def delete_cookie(name)
   raise ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.to_s.strip.empty?
-
-  execute :delete_cookie, name: name
+  
+  execute :delete_cookie, name: name.to_s
 end
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • Update

    @aguspe aguspe changed the title Rb delete cookie issue [rb] Allow symbols again to be passed on delete_cookie Mar 26, 2025
    @aguspe aguspe merged commit 8f09638 into SeleniumHQ:trunk Mar 26, 2025
    23 checks passed
    @aguspe aguspe mentioned this pull request Mar 26, 2025
    Merged
    8 tasks
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    This was referenced Dec 21, 2025
    Open
    Open
    Open
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    C-rb Ruby Bindings Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @aguspe @titusfortner