Skip to content

Add fix made by Pallavi #15385

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.

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

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 7, 2025

User description

Motivation and Context

This PR applies the fix done by Pallavi to have a guard clause for the cookie delete method:

#15099

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

  • Added a guard clause to delete_cookie method to handle null or empty cookie names.

  • Introduced tests to validate the new guard clause for delete_cookie.

  • Ensured error handling for invalid cookie names with appropriate exceptions.


Changes walkthrough 📝

Relevant files
Bug fix
bridge.rb
Add guard clause to `delete_cookie` method                             

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

  • Added a guard clause to delete_cookie method.
  • Raised ArgumentError for null or empty cookie names.
  • +2/-0     
    Tests
    manager_spec.rb
    Add tests for `delete_cookie` guard clause                             

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

  • Added tests for delete_cookie with empty string and nil.
  • Validated error handling for invalid cookie names.
  • +10/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Mar 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix potential NoMethodError

    The strip method will raise a NoMethodError if name is not a string. Add a check
    to ensure name is a string before calling strip on it.

    rb/lib/selenium/webdriver/remote/bridge.rb [387-391]

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

    __

    Why: The suggestion correctly identifies a critical bug where calling strip on a non-String object would raise a NoMethodError. The fix properly guards against this by checking if name is a String before calling strip, preventing runtime errors when non-String values are passed.

    High
    • More

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Mar 7, 2025

    This PR was intended as an example to help out and support, now I will close it and merge the actual PR https://github.com/SeleniumHQ/selenium/actions/runs/13719597354/job/38371912931?pr=15386

    @aguspe aguspe closed this Mar 7, 2025
    @pallavigitwork
    Copy link
    Member

    Thank you very much.

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

    Successfully merging this pull request may close these issues.

    2 participants