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

[rb] Add tests for the cookie named, and updates type #14843

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Dec 2, 2024

User description

Description

This PR adds a test to validate that the cookie-named method can throw an error as the specification for get_cookie which is used in the cookie_named method states https://www.w3.org/TR/webdriver1/#get-named-cookie.

It also updates the RBS files and the doc comment to match the expected behavior.

Motivation and Context

All of our users need to understand the expected behavior of the methods, and that the return types, matched that expected behavior.

Thank you so much @luke-hill for raising this up.

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

Tests, Enhancement


Description

  • Added a test to ensure cookie_named method throws NoSuchCookieError when a non-existent cookie is requested.
  • Updated the documentation comment for the cookie_named method to reflect the error handling behavior.
  • Modified the type signature in RBS files to include Error::NoSuchCookieError as a possible return type for cookie_named.

Changes walkthrough 📝

Relevant files
Documentation
manager.rb
Update doc comment for `cookie_named` method                         

rb/lib/selenium/webdriver/common/manager.rb

  • Updated the doc comment for cookie_named method.
  • Clarified that cookie_named throws NoSuchCookieError if the cookie is
    not found.
  • +1/-1     
    Tests
    manager_spec.rb
    Add test for `cookie_named` error handling                             

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

  • Added a test for cookie_named method.
  • Test checks for NoSuchCookieError when cookie is not found.
  • +5/-0     
    Enhancement
    manager.rbs
    Update type signature for `cookie_named` method                   

    rb/sig/lib/selenium/webdriver/common/manager.rbs

  • Updated type signature for cookie_named method.
  • Included Error::NoSuchCookieError in return type.
  • +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Sorry, something went wrong.

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 2, 2024

    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

    Documentation Mismatch
    The doc comment states the method returns nil or throws error, but the implementation doesn't show explicit error throwing. Need to verify error handling implementation.

    Type Signature Inconsistency
    The convert_cookie method signature changed from accepting a Hash to accepting a String parameter. Need to verify this change is intentional and correct.

    @aguspe aguspe changed the title add tests for the cookie named, and updates type [rb] Add tests for the cookie named, and updates type Dec 2, 2024
    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 2, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect parameter type in method signature that could lead to runtime type errors

    The type signature for convert_cookie method parameter is incorrect. It should
    accept a Hash as input, not a String, since it's used to convert cookie data
    structures.

    rb/sig/lib/selenium/webdriver/common/manager.rbs [36]

    -def convert_cookie: (String) -> (Hash[Symbol, untyped] | Error::NoSuchCookieError)
    +def convert_cookie: (Hash[String, untyped]) -> (Hash[Symbol, untyped] | Error::NoSuchCookieError)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a critical type mismatch in the method signature that could cause runtime errors, as the convert_cookie method expects a Hash input but is typed to accept a String.

    9
    Correct method signature to properly represent exception handling rather than including exception in return type

    The return type for cookie_named should not include Error::NoSuchCookieError as it's
    an exception that's raised, not a return value.

    rb/sig/lib/selenium/webdriver/common/manager.rbs [14]

    -def cookie_named: (String name) -> (Hash[Symbol, untyped] | Error::NoSuchCookieError)
    +def cookie_named: (String name) -> Hash[Symbol, untyped]
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion accurately points out that NoSuchCookieError should not be part of the return type since it's an exception that's raised, as evidenced by the test case and documentation changes.

    8

    💡 Need additional feedback ? start a PR chat

    Sorry, something went wrong.

    @aguspe aguspe requested a review from p0deje December 3, 2024 06:40
    @aguspe aguspe merged commit 901dc6c into SeleniumHQ:trunk Dec 3, 2024
    23 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Dec 3, 2024
    * add tests for the cookie named, and updates type
    
    * update gemfile lock
    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

    1 participant