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] Adds support for the w3c silent option for the ruby library #14152

Merged

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jun 18, 2024

User description

Description

While working on improving the documentation for the ruby library on this pr: SeleniumHQ/seleniumhq.github.io#1748

I noticed that the ruby library was throwing an error when the option silent was set to true for internet explorer, and that it was not implemented as it is in the js or java bindings

Motivation and Context

It's important that all the bindings support the same w3c options and that our documentation is up to day:

Current docs: https://www.selenium.dev/documentation/webdriver/browsers/internet_explorer/

Screenshot 2024-06-18 at 21 45 07

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 support for the silent option in the Internet Explorer options for the Ruby library.
  • Updated the options hash in options.rb to include the silent key.
  • Added unit tests in options_spec.rb to verify the silent option is correctly set and handled.

Changes walkthrough 📝

Relevant files
Enhancement
options.rb
Add support for `silent` option in IE options                       

rb/lib/selenium/webdriver/ie/options.rb

  • Added support for the silent option in Internet Explorer options.
  • Updated the options hash to include the silent key.
  • +2/-1     
    Tests
    options_spec.rb
    Add tests for `silent` option in IE options                           

    rb/spec/unit/selenium/webdriver/ie/options_spec.rb

  • Added tests for the silent option in Internet Explorer options.
  • Verified the silent option is correctly set in various test cases.
  • +5/-1     

    💡 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 Yes
    🔒 Security concerns No
    ⚡ Key issues to review None

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Consistency
    Prefix the new key with ie. to maintain consistency with existing keys

    To maintain consistency with the existing keys, consider prefixing the new silent key with
    ie..

    rb/lib/selenium/webdriver/ie/options.rb [46]

    -silent: 'silent'
    +silent: 'ie.silent'
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a consistency issue in the naming convention of the keys in the options hash. Prefixing with 'ie.' aligns with existing keys and improves code consistency.

    8
    Test coverage
    Add a test case to verify the default value of the new option

    Add a test case to verify the default value of the silent option to ensure it behaves
    correctly when not explicitly set.

    rb/spec/unit/selenium/webdriver/ie/options_spec.rb [91]

     expect(opts.silent).to be_truthy
    +opts_without_silent = Options.new
    +expect(opts_without_silent.silent).to be_falsey
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a test case to verify the default behavior of the new silent option is important for ensuring expected functionality when the option is not set. This enhances test coverage and reliability.

    7
    Add a test to verify that the new option is passed to the driver

    Ensure that the silent option is correctly passed to the underlying driver by adding a
    test that checks the final capabilities.

    rb/spec/unit/selenium/webdriver/ie/options_spec.rb [63]

     expect(opts.args.to_a).to eq(%w[foo bar])
    +expect(opts.capabilities['silent']).to be(true)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to verify that the silent option is passed correctly to the driver is valid and improves the test suite by ensuring that the option affects the driver's behavior as expected.

    7
    Add a test to verify that the new option can be disabled

    Consider adding a test that sets the silent option to false to ensure that the option can
    be correctly disabled.

    rb/spec/unit/selenium/webdriver/ie/options_spec.rb [91]

     expect(opts.silent).to be_truthy
    +opts_with_silent_false = Options.new(silent: false)
    +expect(opts_with_silent_false.silent).to be_falsey
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion is useful as it proposes testing the ability to disable the silent option, which is crucial for verifying that the option can be toggled as expected, enhancing the robustness of the test suite.

    7

    @titusfortner titusfortner merged commit 0f7386e into SeleniumHQ:trunk Jun 18, 2024
    27 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    …eniumHQ#14152)
    
    Adds support for the w3c silent option for the ruby library
    
    Co-authored-by: aguspe <agustin.pe94@gmail.com>
    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