SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
30.42k stars 8.15k forks source link

[java] Add convenience for enabling BiDi #14029

Closed pujagani closed 4 months ago

pujagani commented 4 months ago

User description

Related to #13991

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.

Description

Add a method to enable BiDi

Motivation and Context

Types of changes

Checklist


PR Type

Enhancement, Tests


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
ChromiumOptions.java
Add `enableBiDi` method to ChromiumOptions                             

java/src/org/openqa/selenium/chromium/ChromiumOptions.java
  • Added enableBiDi method to set the webSocketUrl capability.
  • Returns the current instance for method chaining.
  • +5/-0     
    FirefoxOptions.java
    Add `enableBiDi` method to FirefoxOptions                               

    java/src/org/openqa/selenium/firefox/FirefoxOptions.java
  • Added enableBiDi method to set the webSocketUrl capability.
  • Returns the current instance for method chaining.
  • +5/-0     
    Tests
    BiDiSessionCleanUpTest.java
    Use `enableBiDi` method in BiDiSessionCleanUpTest               

    java/test/org/openqa/selenium/bidi/BiDiSessionCleanUpTest.java
  • Replaced direct capability setting with enableBiDi method in tests.
  • +2/-2     
    Browser.java
    Use `enableBiDi` method in Browser configurations               

    java/test/org/openqa/selenium/testing/drivers/Browser.java
  • Replaced direct capability setting with enableBiDi method in browser
    configurations.
  • +3/-3     

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

    codiumai-pr-agent-pro[bot] commented 4 months ago

    PR Review ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 2, because the PR introduces a new method in multiple classes to encapsulate an existing capability setting. The changes are straightforward and localized, but they span multiple files which slightly increases the review effort.
    ๐Ÿงช Relevant tests Yes
    โšก Possible issues Consistency Issue: The method `enableBiDi()` sets the capability "webSocketUrl" to true. The naming suggests it should be setting a URL, not a boolean. This could lead to confusion or misuse of the method.
    ๐Ÿ”’ Security concerns No
    codiumai-pr-agent-pro[bot] commented 4 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract repeated code for enabling BiDi into a helper method for better readability ___ **To ensure consistency and readability, consider extracting the repeated code for enabling
    BiDi into a separate helper method within the test class.** [java/test/org/openqa/selenium/bidi/BiDiSessionCleanUpTest.java [39]](https://github.com/SeleniumHQ/selenium/pull/14029/files#diff-1b2ac2efc18ef0c906bc159179e683bb9d22b805302dc8b2bdb54fe6a66d2837R39-R39) ```diff -options.enableBiDi(); +enableBiDiForOptions(options); ```
    Suggestion importance[1-10]: 7 Why: The suggestion is valid as it addresses code duplication and improves maintainability by suggesting the extraction of repeated code into a helper method.
    7