Skip to content
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

modified delete cookie added code and test #15386

Merged
merged 6 commits into from
Mar 8, 2025

Conversation

pallavigitwork
Copy link
Member

@pallavigitwork pallavigitwork commented Mar 7, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Fixed delete Cookie and added code and test

Motivation and Context

it is required.

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 validation to delete_cookie to prevent null or empty names.

  • Updated integration tests to cover invalid cookie name scenarios.

  • Ensured existing tests for cookie deletion remain functional.


Changes walkthrough 📝

Relevant files
Bug fix
bridge.rb
Add validation for `delete_cookie` method                               

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

  • Added validation to check for null or empty cookie names.
  • Raised ArgumentError for invalid cookie names.
  • +2/-0     
    Tests
    manager_spec.rb
    Add and update tests for cookie deletion                                 

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

  • Added test for empty cookie name validation.
  • Verified ArgumentError is raised for invalid cookie names.
  • Ensured existing cookie deletion tests remain intact.
  • +5/-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.
  • Sorry, something went wrong.

    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
    ⚡ Recommended focus areas for review

    Validation Logic

    The validation for empty cookie names uses name.strip.empty? which will create an unnecessary string object when the name is already empty. Consider using a more direct check.

    raise ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.strip.empty?

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Include name in error message

    The current implementation will raise an error for whitespace-only cookie names,
    but will still send the stripped name to the server. Consider using the original
    name in the error message for clarity and consistency.

    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: '#{name}'" if name.nil? || name.strip.empty?
     
       execute :delete_cookie, name: name
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion improves error reporting by including the actual value of the cookie name in the error message, which would help with debugging. However, this is a minor enhancement that doesn't address any functional issues or bugs.

    Low
    • Update

    Sorry, something went wrong.

    @pallavigitwork
    Copy link
    Member Author

    @aguspe requesting Review.

    @aguspe
    Copy link
    Contributor

    aguspe commented Mar 7, 2025

    Great work Pallavi, approved

    Copy link
    Contributor

    @aguspe aguspe left a comment

    Choose a reason for hiding this comment

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

    Could you add an extra test that validates what happens when the value is nil

    it 'throws an error when cookie name is an empty string' do
              expect { driver.manage.delete_cookie(nil) }
                .to raise_error(ArgumentError, /Cookie name cannot be null or empty/)
            end
    

    @pallavigitwork
    Copy link
    Member Author

    thanks @aguspe , understood. added the code snippet. Kindly review when you can and let me know. grateful for your continued help.

    @pallavigitwork
    Copy link
    Member Author

    Thank you @p0deje .

    @p0deje p0deje merged commit 709df6d into SeleniumHQ:trunk Mar 8, 2025
    22 of 23 checks passed
    @nvborisenko
    Copy link
    Member

    Contribution to #15044

    @pallavigitwork
    Copy link
    Member Author

    thanks @nvborisenko , i forgot the original PR number.

    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    None yet

    4 participants