Skip to content

Create browser module, added user context related methods #15371

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

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 4, 2025

User description

Motivation and Context

This PR is a continuation to add full support for the BiDi protocol for the ruby bindings

It adds the browser module and the following methods:

  • createUserContext
  • getUserContexts
  • removeUserContext

For more information here is the BiDi documentation: https://w3c.github.io/webdriver-bidi/#command-browser-createUserContext

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

Enhancement, Tests


Description

  • Added a new Browser module to the BiDi implementation.

  • Implemented user context management methods: create_user_context, get_user_contexts, and remove_user_context.

  • Introduced integration tests for the Browser module's user context methods.

  • Added type signatures for the Browser module in RBS.


Changes walkthrough 📝

Relevant files
Enhancement
bidi.rb
Add autoload for Browser module                                                   

rb/lib/selenium/webdriver/bidi.rb

  • Added autoload for the new Browser module.
+1/-0     
browser.rb
Add Browser class with user context methods                           

rb/lib/selenium/webdriver/bidi/browser.rb

  • Introduced a new Browser class under the BiDi module.
  • Implemented methods for managing user contexts: create_user_context,
    get_user_contexts, and remove_user_context.
  • +42/-0   
    browser.rbs
    Add type signatures for Browser module                                     

    rb/sig/lib/selenium/webdriver/bidi/browser.rbs

  • Added type signatures for the Browser class and its methods.
  • Defined method signatures for user context management.
  • +17/-0   
    Tests
    browser_spec.rb
    Add integration tests for Browser module                                 

    rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb

  • Added integration tests for create_user_context, get_user_contexts,
    and remove_user_context.
  • Included error handling tests for invalid user context operations.
  • +74/-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.
  • @aguspe aguspe added the C-rb Ruby Bindings label Mar 4, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 4, 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

    Error Handling

    The browser methods don't include any input validation or error handling. Should validate user_context parameter before making API calls.

    def create_user_context
      @bidi.send_cmd('browser.createUserContext')
    end
    
    def get_user_contexts
      @bidi.send_cmd('browser.getUserContexts')
    end
    
    def remove_user_context(user_context)
      @bidi.send_cmd('browser.removeUserContext', userContext: user_context)
    end
    Type Definition

    The return type for get_user_contexts method appears incorrect. Based on the tests, it returns a Hash with 'userContexts' key containing an array, not an Array directly.

    def get_user_contexts: () -> Array[Hash[String, String]]

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 4, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect return type signature

    The return type for get_user_contexts is incorrect. Based on the integration
    tests, it returns a Hash with 'userContexts' key containing an array

    rb/sig/lib/selenium/webdriver/bidi/browser.rbs [11]

    -def get_user_contexts: () -> Array[Hash[String, String]]
    +def get_user_contexts: () -> Hash[String, Array[Hash[String, String]]]
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The type signature correction is crucial as it fixes a mismatch between the actual implementation and its type definition. The integration tests clearly show that get_user_contexts returns a Hash with a 'userContexts' key, not a plain Array.

    High
    General
    Add parameter validation

    Add input validation for the user_context parameter to ensure it's not nil or
    empty before making the API call

    rb/lib/selenium/webdriver/bidi/browser.rb [36-38]

     def remove_user_context(user_context)
    +  raise ArgumentError, 'user_context cannot be nil or empty' if user_context.nil? || user_context.empty?
       @bidi.send_cmd('browser.removeUserContext', userContext: user_context)
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Adding input validation for the user_context parameter is a good defensive programming practice that can prevent cryptic errors and provide clearer error messages to users. The tests show that this parameter is critical for the method's functionality.

    Medium
    • 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.

    1 participant