Skip to content

Conversation

@aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 13, 2025

User description

Motivation and Context

The purpose of this PR is to add the ability for ruby tests to run using chrome-beta to make it easier to debug

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 support for running tests with Chrome Beta.

  • Introduced chrome_beta method to configure Chrome Beta driver.

  • Updated Bazel build files to include Chrome Beta configuration.

  • Enhanced test environment to handle Chrome Beta driver setup.


Changes walkthrough 📝

Relevant files
Enhancement
test_environment.rb
Add Chrome Beta driver support in test environment             

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb

  • Added chrome_beta method to configure Chrome Beta driver.
  • Modified create_driver! to handle Chrome Beta driver.
  • Adjusted driver method selection logic for Chrome Beta.
  • +8/-1     
    Configuration changes
    tests.bzl
    Add Chrome Beta configuration to Bazel tests                         

    rb/spec/tests.bzl

  • Added Chrome Beta configuration to Bazel test definitions.
  • Included environment variables for Chrome Beta setup.
  • Configured Chrome Beta dependencies and data.
  • +23/-0   
    BUILD.bazel
    Include Chrome Beta in supported browsers list                     

    rb/spec/integration/BUILD.bazel

    • Added chrome-beta to the list of supported browsers.
    +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Method Naming

    The chrome_beta method modifies the @driver instance variable to :chrome but the PR is adding support for :chrome_beta. This might cause inconsistency in driver identification.

    def chrome_beta(opts = {})
      @driver = :chrome
      opts[:browser_version] = 'beta'
    end
    Configuration Mismatch

    The Bazel configuration uses chrome-beta (with hyphen) while the Ruby code uses :chrome_beta (with underscore). This naming inconsistency could lead to integration issues.

    "chrome-beta": {
        "data": chrome_data,
        "deps": ["//rb/lib/selenium/webdriver:chrome"],
        "tags": [],
        "target_compatible_with": [],
        "env": {
            "WD_REMOTE_BROWSER": "chrome",
            "WD_SPEC_DRIVER": "chrome-beta",
        } | select({
            "@selenium//common:use_pinned_linux_chrome": {
                "CHROME_BINARY": "$(location @linux_chrome//:chrome-linux64/chrome)",
                "CHROMEDRIVER_BINARY": "$(location @linux_chromedriver//:chromedriver)",
            },
            "@selenium//common:use_pinned_macos_chrome": {
                "CHROME_BINARY": "$(location @mac_chrome//:Chrome.app)/Contents/MacOS/Chrome",
                "CHROMEDRIVER_BINARY": "$(location @mac_chromedriver//:chromedriver)",
            },
            "//conditions:default": {},
        }) | select({
            "@selenium//common:use_headless_browser": {"HEADLESS": "true"},
            "//conditions:default": {},
        }),
    },

    @aguspe aguspe added C-rb Ruby Bindings and removed Review effort 2/5 labels Mar 13, 2025
    @aguspe aguspe requested a review from titusfortner March 13, 2025 21:09
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 13, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix driver initialization parameter
    Suggestion Impact:The commit addressed the issue differently by restructuring the code to properly handle the chrome_beta driver case. Instead of passing 'method' to WebDriver::Driver.for, it now sets 'method = driver' for non-chrome_beta cases, which effectively implements the suggestion's intent in a different way.

    code diff:

    +          if driver == :chrome_beta
    +            opts[:browser_version] = 'beta'
    +            method = :chrome_driver
    +          else
    +            method = driver
    +          end

    The WebDriver::Driver.for method should receive the driver symbol, not the
    method symbol. You're passing method instead of driver, which will cause an
    error when trying to initialize non-private driver methods.

    rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [166-176]

     def create_driver!(listener: nil, **opts, &block)
       check_for_previous_error
     
       chrome_beta(opts) if driver == :chrome_beta
     
       method = :"#{driver}_driver"
       instance = if private_methods.include?(method)
                    send(method, listener: listener, options: build_options(**opts))
                  else
    -               WebDriver::Driver.for(method, listener: listener, options: build_options(**opts))
    +               WebDriver::Driver.for(driver, listener: listener, options: build_options(**opts))
                  end

    [Suggestion has been applied]

    Suggestion importance[1-10]: 10

    __

    Why: This is a critical bug fix. The PR incorrectly changed the parameter from driver to method in the WebDriver::Driver.for call, which would cause the driver initialization to fail for non-private driver methods. The suggestion correctly restores the original parameter.

    High
    • Update

    @aguspe aguspe changed the title Add support for beta chrome [rb] Add support for beta chrome Mar 27, 2025
    Copy link
    Member

    @p0deje p0deje left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    The changes make sense, though I am not sure what version should be in repositories. However, if we want to auto-update the version of pinned beta Chrome, we need to change scripts/pinned_browsers.py so it automatically switches changes repositories.bzl to a new beta version.

    @aguspe aguspe requested a review from p0deje May 29, 2025 22:31
    @aguspe aguspe merged commit 1b0ab9b into SeleniumHQ:trunk May 31, 2025
    21 of 22 checks passed
    @aguspe aguspe deleted the rb_add_support_for_chrome_beta branch May 31, 2025 12:03
    aguspe added a commit that referenced this pull request May 31, 2025
    aguspe added a commit that referenced this pull request May 31, 2025
    Revert "[rb] Add support for beta chrome (#15417)"
    
    This reverts commit 1b0ab9b.
    @aguspe aguspe restored the rb_add_support_for_chrome_beta branch May 31, 2025 17:59
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    C-rb Ruby Bindings

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants