Closed pujagani closed 3 weeks ago
โฑ๏ธ Estimated effort to review [1-5] | 2, because the PR involves a straightforward addition of a new preference across multiple language bindings in the Selenium project. The changes are consistent and similar across all files, making it easier to review. |
๐งช Relevant tests | No |
โก Possible issues | Hardcoded Value: The preference "remote.active-protocols" is set to 2 across all bindings. It's important to ensure this value is correct and intended for all future versions of Firefox where CDP might be disabled by default. |
๐ Security concerns | No |
Category | Suggestion | Score |
Maintainability |
Use a constant for the preference key to avoid magic strings___ **Consider using a constant for the preference key "remote.active-protocols" to avoid magicstrings and improve maintainability.** [java/src/org/openqa/selenium/firefox/FirefoxOptions.java [69]](https://github.com/SeleniumHQ/selenium/pull/14091/files#diff-d011ae7f625544736a498af510308e81e40d69d9d85e7fee18c14aedc4ed4d9bR69-R69) ```diff -addPreference("remote.active-protocols", 2); +private static final String REMOTE_PROTOCOLS_KEY = "remote.active-protocols"; +addPreference(REMOTE_PROTOCOLS_KEY, 2); ``` Suggestion importance[1-10]: 7Why: Using a constant for "remote.active-protocols" improves code maintainability and readability by avoiding magic strings. This is a good practice, especially in larger projects where the same string might be reused. | 7 |
Attention: Patch coverage is 0%
with 1 line
in your changes missing coverage. Please review.
Project coverage is 57.87%. Comparing base (
57f8398
) to head (66084be
). Report is 429 commits behind head on trunk.:exclamation: Current head 66084be differs from pull request most recent head ae8bd16
Please upload reports for the commit ae8bd16 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
py/selenium/webdriver/firefox/options.py | 0.00% | 0 Missing and 1 partial :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
โฑ๏ธ Estimated effort to review [1-5] | 2 |
๐งช Relevant tests | Yes |
๐ Security concerns | No |
โก Key issues to review | None |
Category | Suggestion | Score |
Best practice |
Correct the typo in the test function name___ **Correct the typo in the test function name fromtest_acepts_options_to_remote_driver to test_accepts_options_to_remote_driver .**
[py/test/unit/selenium/webdriver/remote/new_session_tests.py [55]](https://github.com/SeleniumHQ/selenium/pull/14091/files#diff-796ccf287d5fe4869871971128599eb2c753a904cc8181399272687dea26441eR55-R55)
```diff
-def test_acepts_options_to_remote_driver(mocker, browser_name):
+def test_accepts_options_to_remote_driver(mocker, browser_name):
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 10Why: Correcting typos in function names is crucial as it affects the readability and maintainability of the code. It also ensures that the intended functionality is clear and that automated tools can correctly identify and run the tests. | 10 |
Maintainability |
Use a constant for the preference key to avoid hardcoding the string___ **Consider using a constant for the preference key"remote.active-protocols" to avoid hardcoding the string multiple times and to make future changes easier.** [java/src/org/openqa/selenium/firefox/FirefoxOptions.java [69]](https://github.com/SeleniumHQ/selenium/pull/14091/files#diff-d011ae7f625544736a498af510308e81e40d69d9d85e7fee18c14aedc4ed4d9bR69-R69) ```diff -addPreference("remote.active-protocols", 3); +private static final String REMOTE_ACTIVE_PROTOCOLS = "remote.active-protocols"; +addPreference(REMOTE_ACTIVE_PROTOCOLS, 3); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Using a constant for the preference key enhances maintainability and reduces the risk of errors in future modifications. This is a good practice, especially in a setting where the key might be reused. | 7 |
Possible issue |
Check if the
___
**Consider checking if the | 5 |
Add a null check before setting the preference to avoid potential
___
**Consider adding a null check for | 2 |
@whimboo if we send this command on an old version of Firefox will it fail? Are we creating a new minimum supported version of Firefox with this addition?
@whimboo if we send this command on an old version of Firefox will it fail? Are we creating a new minimum supported version of Firefox with this addition?
No. Setting this preference for an older version of Firefox is a no-op because 3
was the default value since the early days of BiDi support.
User description
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
Related to #11736. Ensuring CDP is enabled by default in Firefox.
Motivation and Context
CDP is going to be deprecated in Firefox. Setting this preference will ensure it is enabled by default until its removal. Enabling this will not force Selenium users to pass this preference and avoid any breaking changes.
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Changes walkthrough ๐
5 files
FirefoxOptions.java
Enable CDP and WebDriver BiDi by default in FirefoxOptions
java/src/org/openqa/selenium/firefox/FirefoxOptions.java
FirefoxOptions.cs
Enable CDP and WebDriver BiDi by default in FirefoxOptions
dotnet/src/webdriver/Firefox/FirefoxOptions.cs
firefox.js
Enable CDP and WebDriver BiDi by default in Firefox Options
javascript/node/selenium-webdriver/firefox.js
options.rb
Enable CDP and WebDriver BiDi by default in Firefox Options
rb/lib/selenium/webdriver/firefox/options.rb
options.py
Enable CDP and WebDriver BiDi by default in Firefox Options
py/selenium/webdriver/firefox/options.py
5 files
options_test.js
Update tests for CDP and WebDriver BiDi preference
javascript/node/selenium-webdriver/test/firefox/options_test.js - Updated tests to check for the new default preference.
driver_spec.rb
Update tests for CDP and WebDriver BiDi preference
rb/spec/unit/selenium/webdriver/firefox/driver_spec.rb - Updated tests to check for the new default preference.
options_spec.rb
Update tests for CDP and WebDriver BiDi preference
rb/spec/unit/selenium/webdriver/firefox/options_spec.rb - Updated tests to check for the new default preference.
firefox_options_tests.py
Update tests for CDP and WebDriver BiDi preference
py/test/unit/selenium/webdriver/firefox/firefox_options_tests.py - Updated tests to check for the new default preference.
new_session_tests.py
Update tests for CDP and WebDriver BiDi preference
py/test/unit/selenium/webdriver/remote/new_session_tests.py - Updated tests to check for the new default preference.
1 files
workspace.xml
Add IntelliJ IDEA workspace configuration
java/.idea/workspace.xml - Added IntelliJ IDEA workspace configuration file.