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

Add preference to enable CDP in Firefox by default #14091

Merged
merged 11 commits into from
Jun 12, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jun 6, 2024

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

  • 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 preference to enable CDP and WebDriver BiDi by default in Firefox options across multiple languages (Java, C#, JavaScript, Ruby, Python).
  • Updated tests to check for the new default preference in various languages.
  • Included comments with a link to the deprecation notice for CDP in Firefox.
  • Added IntelliJ IDEA workspace configuration file.

Changes walkthrough 📝

Relevant files
Enhancement
5 files
FirefoxOptions.java
Enable CDP and WebDriver BiDi by default in FirefoxOptions

java/src/org/openqa/selenium/firefox/FirefoxOptions.java

  • Added preference to enable CDP and WebDriver BiDi by default.
  • Included a comment with a link to the deprecation notice.
  • +4/-0     
    FirefoxOptions.cs
    Enable CDP and WebDriver BiDi by default in FirefoxOptions

    dotnet/src/webdriver/Firefox/FirefoxOptions.cs

  • Added preference to enable CDP and WebDriver BiDi by default.
  • Included a comment with a link to the deprecation notice.
  • +3/-0     
    firefox.js
    Enable CDP and WebDriver BiDi by default in Firefox Options

    javascript/node/selenium-webdriver/firefox.js

  • Added preference to enable CDP and WebDriver BiDi by default.
  • Included a comment with a link to the deprecation notice.
  • +3/-0     
    options.rb
    Enable CDP and WebDriver BiDi by default in Firefox Options

    rb/lib/selenium/webdriver/firefox/options.rb

  • Added preference to enable CDP and WebDriver BiDi by default.
  • Included a comment with a link to the deprecation notice.
  • +3/-0     
    options.py
    Enable CDP and WebDriver BiDi by default in Firefox Options

    py/selenium/webdriver/firefox/options.py

  • Added preference to enable CDP and WebDriver BiDi by default.
  • Included a comment with a link to the deprecation notice.
  • +3/-0     
    Tests
    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.
    +7/-1     
    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.
    +5/-2     
    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.
    +4/-4     
    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.
    +1/-1     
    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.
    +2/-2     
    Configuration changes
    1 files
    workspace.xml
    Add IntelliJ IDEA workspace configuration                               

    java/.idea/workspace.xml

    • Added IntelliJ IDEA workspace configuration file.
    +49/-0   

    💡 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 Review 🔍

    ⏱️ 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

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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 magic
    strings and improve maintainability.

    java/src/org/openqa/selenium/firefox/FirefoxOptions.java [69]

    -addPreference("remote.active-protocols", 2);
    +private static final String REMOTE_PROTOCOLS_KEY = "remote.active-protocols";
    +addPreference(REMOTE_PROTOCOLS_KEY, 2);
     
    Suggestion importance[1-10]: 7

    Why: 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

    Copy link

    codecov bot commented Jun 11, 2024

    Codecov Report

    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.

    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 ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #14091      +/-   ##
    ==========================================
    - Coverage   58.48%   57.87%   -0.61%     
    ==========================================
      Files          86       87       +1     
      Lines        5270     5403     +133     
      Branches      220      228       +8     
    ==========================================
    + Hits         3082     3127      +45     
    - Misses       1968     2048      +80     
    - Partials      220      228       +8     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @pujagani pujagani marked this pull request as ready for review June 11, 2024 13:40
    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
    Best practice
    Correct the typo in the test function name

    Correct the typo in the test function name from test_acepts_options_to_remote_driver to
    test_accepts_options_to_remote_driver.

    py/test/unit/selenium/webdriver/remote/new_session_tests.py [55]

    -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]: 10

    Why: 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]

    -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]: 7

    Why: 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 setPreference method exists before calling it to avoid runtime errors

    Consider checking if the setPreference method exists on this to avoid potential runtime
    errors.

    javascript/node/selenium-webdriver/firefox.js [251]

    -this.setPreference('remote.active-protocols', 3)
    +if (typeof this.setPreference === 'function') {
    +    this.setPreference('remote.active-protocols', 3);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Checking for the existence of a method before invoking it can prevent runtime errors in JavaScript, especially in dynamic or loosely-typed environments. This is a valid precaution, though typically not required in well-defined class structures.

    5
    Add a null check before setting the preference to avoid potential NullReferenceException

    Consider adding a null check for this.SetPreference to ensure it does not throw a
    NullReferenceException if this is null.

    dotnet/src/webdriver/Firefox/FirefoxOptions.cs [93]

    -this.SetPreference("remote.active-protocols", 3);
    +if (this != null)
    +{
    +    this.SetPreference("remote.active-protocols", 3);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    Why: The suggestion to add a null check for this in C# is unnecessary because this cannot be null in a non-static method context. This suggestion shows a misunderstanding of C# context.

    2

    @titusfortner
    Copy link
    Member

    @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
    Copy link
    Contributor

    whimboo commented Jun 11, 2024

    @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.

    @pujagani pujagani merged commit a4d1b02 into SeleniumHQ:trunk Jun 12, 2024
    4 checks passed
    yahonda added a commit to yahonda/rails that referenced this pull request Jun 21, 2024
    This pull request supports selenium-webdriver 4.22.0 that enables CDP in Firefox by default.
    because Firefox 129 deprecates Chrome DevTools Protocol (CDP).
    selenium-webdriver 4.22.0 enables CDP explicitly by adding "remote.active-protocols"=>3 .
    
    - Steps to reproduce and this commit addresses these failures.
    ```ruby
    $ bundle update selenium-webdriver --conservative
    $ git diff main ../Gemfile.lock
    diff --git a/Gemfile.lock b/Gemfile.lock
    index 4e1c049ac0..e05f4b3b3c 100644
    --- a/Gemfile.lock
    +++ b/Gemfile.lock
    @@ -512,8 +512,9 @@ GEM
           google-protobuf (~> 3.25)
         sass-embedded (1.69.6-x86_64-linux-gnu)
           google-protobuf (~> 3.25)
    -    selenium-webdriver (4.20.1)
    +    selenium-webdriver (4.22.0)
           base64 (~> 0.2)
    +      logger (~> 1.4)
           rexml (~> 3.2, >= 3.2.5)
           rubyzip (>= 1.2.2, < 3.0)
           websocket (~> 1.0)
    $ cd actionpack
    $ bin/test test/dispatch/system_testing/driver_test.rb test/dispatch/system_testing/driver_test.rb
    Running 18 tests in a single process (parallelization threshold is 50)
    Run options: --seed 58668
    
    .....F
    
    Failure:
    DriverTest#test_define_extra_capabilities_using_firefox [test/dispatch/system_testing/driver_test.rb:127]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"moz:firefoxOptions"=>{"args"=>["--host=127.0.0.1"], "prefs"=>{"browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    +{"moz:firefoxOptions"=>{"args"=>["--host=127.0.0.1"], "prefs"=>{"remote.active-protocols"=>3, "browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    
    bin/test test/dispatch/system_testing/driver_test.rb:113
    
    .F
    
    Failure:
    DriverTest#test_define_extra_capabilities_using_headless_firefox [test/dispatch/system_testing/driver_test.rb:144]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"moz:firefoxOptions"=>{"args"=>["-headless", "--host=127.0.0.1"], "prefs"=>{"browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    +{"moz:firefoxOptions"=>{"args"=>["-headless", "--host=127.0.0.1"], "prefs"=>{"remote.active-protocols"=>3, "browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    
    bin/test test/dispatch/system_testing/driver_test.rb:130
    
    ..........
    
    Finished in 0.007717s, 2332.3654 runs/s, 4794.3066 assertions/s.
    18 runs, 37 assertions, 2 failures, 0 errors, 0 skips
    ```
    
    - Planned Deprecation of CDP in Firefox
    https://groups.google.com/a/mozilla.org/g/dev-platform/c/Z6Qu3ZT1MJ0?pli=1
    
    - Add preference to enable CDP in Firefox by default
    SeleniumHQ/selenium#14091
    
    - [rb] Add logger gem as a runtime dependency rails#14082
    SeleniumHQ/selenium#14082
    jianbo pushed a commit to jianbo/fix-association-initialize-order that referenced this pull request Jul 8, 2024
    This pull request supports selenium-webdriver 4.22.0 that enables CDP in Firefox by default.
    because Firefox 129 deprecates Chrome DevTools Protocol (CDP).
    selenium-webdriver 4.22.0 enables CDP explicitly by adding "remote.active-protocols"=>3 .
    
    - Steps to reproduce and this commit addresses these failures.
    ```ruby
    $ bundle update selenium-webdriver --conservative
    $ git diff main ../Gemfile.lock
    diff --git a/Gemfile.lock b/Gemfile.lock
    index 4e1c049ac0..e05f4b3b3c 100644
    --- a/Gemfile.lock
    +++ b/Gemfile.lock
    @@ -512,8 +512,9 @@ GEM
           google-protobuf (~> 3.25)
         sass-embedded (1.69.6-x86_64-linux-gnu)
           google-protobuf (~> 3.25)
    -    selenium-webdriver (4.20.1)
    +    selenium-webdriver (4.22.0)
           base64 (~> 0.2)
    +      logger (~> 1.4)
           rexml (~> 3.2, >= 3.2.5)
           rubyzip (>= 1.2.2, < 3.0)
           websocket (~> 1.0)
    $ cd actionpack
    $ bin/test test/dispatch/system_testing/driver_test.rb test/dispatch/system_testing/driver_test.rb
    Running 18 tests in a single process (parallelization threshold is 50)
    Run options: --seed 58668
    
    .....F
    
    Failure:
    DriverTest#test_define_extra_capabilities_using_firefox [test/dispatch/system_testing/driver_test.rb:127]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"moz:firefoxOptions"=>{"args"=>["--host=127.0.0.1"], "prefs"=>{"browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    +{"moz:firefoxOptions"=>{"args"=>["--host=127.0.0.1"], "prefs"=>{"remote.active-protocols"=>3, "browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    
    bin/test test/dispatch/system_testing/driver_test.rb:113
    
    .F
    
    Failure:
    DriverTest#test_define_extra_capabilities_using_headless_firefox [test/dispatch/system_testing/driver_test.rb:144]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"moz:firefoxOptions"=>{"args"=>["-headless", "--host=127.0.0.1"], "prefs"=>{"browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    +{"moz:firefoxOptions"=>{"args"=>["-headless", "--host=127.0.0.1"], "prefs"=>{"remote.active-protocols"=>3, "browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    
    bin/test test/dispatch/system_testing/driver_test.rb:130
    
    ..........
    
    Finished in 0.007717s, 2332.3654 runs/s, 4794.3066 assertions/s.
    18 runs, 37 assertions, 2 failures, 0 errors, 0 skips
    ```
    
    - Planned Deprecation of CDP in Firefox
    https://groups.google.com/a/mozilla.org/g/dev-platform/c/Z6Qu3ZT1MJ0?pli=1
    
    - Add preference to enable CDP in Firefox by default
    SeleniumHQ/selenium#14091
    
    - [rb] Add logger gem as a runtime dependency rails#14082
    SeleniumHQ/selenium#14082
    DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024
    This pull request supports selenium-webdriver 4.22.0 that enables CDP in Firefox by default.
    because Firefox 129 deprecates Chrome DevTools Protocol (CDP).
    selenium-webdriver 4.22.0 enables CDP explicitly by adding "remote.active-protocols"=>3 .
    
    - Steps to reproduce and this commit addresses these failures.
    ```ruby
    $ bundle update selenium-webdriver --conservative
    $ git diff main ../Gemfile.lock
    diff --git a/Gemfile.lock b/Gemfile.lock
    index 4e1c049ac0..e05f4b3b3c 100644
    --- a/Gemfile.lock
    +++ b/Gemfile.lock
    @@ -512,8 +512,9 @@ GEM
           google-protobuf (~> 3.25)
         sass-embedded (1.69.6-x86_64-linux-gnu)
           google-protobuf (~> 3.25)
    -    selenium-webdriver (4.20.1)
    +    selenium-webdriver (4.22.0)
           base64 (~> 0.2)
    +      logger (~> 1.4)
           rexml (~> 3.2, >= 3.2.5)
           rubyzip (>= 1.2.2, < 3.0)
           websocket (~> 1.0)
    $ cd actionpack
    $ bin/test test/dispatch/system_testing/driver_test.rb test/dispatch/system_testing/driver_test.rb
    Running 18 tests in a single process (parallelization threshold is 50)
    Run options: --seed 58668
    
    .....F
    
    Failure:
    DriverTest#test_define_extra_capabilities_using_firefox [test/dispatch/system_testing/driver_test.rb:127]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"moz:firefoxOptions"=>{"args"=>["--host=127.0.0.1"], "prefs"=>{"browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    +{"moz:firefoxOptions"=>{"args"=>["--host=127.0.0.1"], "prefs"=>{"remote.active-protocols"=>3, "browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    
    bin/test test/dispatch/system_testing/driver_test.rb:113
    
    .F
    
    Failure:
    DriverTest#test_define_extra_capabilities_using_headless_firefox [test/dispatch/system_testing/driver_test.rb:144]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"moz:firefoxOptions"=>{"args"=>["-headless", "--host=127.0.0.1"], "prefs"=>{"browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    +{"moz:firefoxOptions"=>{"args"=>["-headless", "--host=127.0.0.1"], "prefs"=>{"remote.active-protocols"=>3, "browser.startup.homepage"=>"http://www.seleniumhq.com/"}}, "browserName"=>"firefox"}
    
    bin/test test/dispatch/system_testing/driver_test.rb:130
    
    ..........
    
    Finished in 0.007717s, 2332.3654 runs/s, 4794.3066 assertions/s.
    18 runs, 37 assertions, 2 failures, 0 errors, 0 skips
    ```
    
    - Planned Deprecation of CDP in Firefox
    https://groups.google.com/a/mozilla.org/g/dev-platform/c/Z6Qu3ZT1MJ0?pli=1
    
    - Add preference to enable CDP in Firefox by default
    SeleniumHQ/selenium#14091
    
    - [rb] Add logger gem as a runtime dependency rails#14082
    SeleniumHQ/selenium#14082
    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.

    3 participants