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] Update selenium manager types #14189

Merged
merged 12 commits into from
Jul 9, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jun 25, 2024

User description

Description

This PR adds the correct type after the Selenium manager class has been updated for ruby

Motivation and Context

It's important as described here #13989 that we utilize the new options available for us in the Selenium manager implementation and that we update the respective types

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


Description

  • Updated the Selenium manager commands in the Ruby client to use mixed output format instead of JSON.
  • Changed the debug flag to use --log-level debug for better logging control.

Changes walkthrough 📝

Relevant files
Enhancement
selenium_manager.rb
Update Selenium manager command options for Ruby client   

rb/lib/selenium/webdriver/common/selenium_manager.rb

  • Changed command output format from JSON to mixed.
  • Updated debug flag to log-level debug.
  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Output Format Change:
    Ensure that the change from JSON to mixed output format in the Selenium manager does not break existing integrations or data parsing.
    Logging Level:
    Verify that the new --log-level debug flag integrates smoothly with the existing logging system and does not introduce excessive verbosity that could obscure important log messages.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Use a single array concatenation operation to add multiple command options at once

    Consider using a single array concatenation operation to add all the new command options
    at once. This can make the code more concise and potentially improve performance slightly.

    rb/lib/selenium/webdriver/common/selenium_manager.rb [115-117]

    -command += %w[--language-binding ruby]
    -command += %w[--output mixed]
    +command += %w[--language-binding ruby --output mixed]
     command << '--log-level debug' if WebDriver.logger.debug?
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to combine array concatenations into a single operation is valid for improving code conciseness and slightly enhancing performance.

    7
    Possible issue
    Verify the compatibility of the --log-level debug option with the selenium manager

    Ensure that the --log-level debug option is compatible with the rest of the command
    options and the expected behavior of the selenium manager, as changing from --debug to
    --log-level debug might have different effects.

    rb/lib/selenium/webdriver/common/selenium_manager.rb [117]

    -command << '--log-level debug' if WebDriver.logger.debug?
    +command << '--log-level debug' if WebDriver.logger.debug? # Ensure compatibility with selenium manager
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: This is a prudent suggestion to ensure that changes in command line options do not unintentionally alter the behavior of the system, although it lacks specific details on how to verify compatibility.

    6

    @aguspe aguspe changed the title [rb] Start working on updating the ruby client to leverage the new selenium manager implementation [rb] Update selenium manager types Jun 28, 2024
    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jun 28, 2024

    @titusfortner I saw the update you did on the selenium manager class, so I changed my PR to add the right types and update the correspondent RBS file for the selenium manager class

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 5, 2024

    Hello @p0deje I hope you are having a great evening, whenever you have time could you review this PR? thank you for all your help

    @p0deje
    Copy link
    Member

    p0deje commented Jul 7, 2024

    Hello @p0deje I hope you are having a great evening, whenever you have time could you review this PR? thank you for all your help

    Hi, sorry for delay. Can you please take a look at the CI failures?

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 7, 2024

    Hello @p0deje I hope you are having a great evening, whenever you have time could you review this PR? thank you for all your help

    Hi, sorry for delay. Can you please take a look at the CI failures?

    Thank you so for taking the time, and no problem I know you are busy.

    Now I remove the last updates on arguments so this PR just updates the types after the changes done to the selenium manager class

    @p0deje
    Copy link
    Member

    p0deje commented Jul 9, 2024

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 9, 2024

    @p0deje the linter issues are fixed now and only the RBS file is left

    @p0deje p0deje merged commit df75c3a into SeleniumHQ:trunk Jul 9, 2024
    22 of 23 checks passed
    @p0deje
    Copy link
    Member

    p0deje commented Jul 9, 2024

    Thank you!

    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