Skip to content

[rb] Add guard for beta firefox #16153

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

Merged
merged 16 commits into from
Aug 11, 2025

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Aug 9, 2025

User description

💥 What does this PR do?

This PR uses the environment variable WD_BROWSER_VERSION to be able to guard against beta firefox and beta chrome

🔧 Implementation Notes

I implemented this way to follow the guard structure we have and because I did not wanted to hardcode a version of the browser

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Refactored beta_chrome_version to beta_browser_version supporting Firefox

  • Added Firefox beta version fetching from Mozilla API

  • Updated test guards to use new generic method


Diagram Walkthrough

flowchart LR
  A["beta_chrome_version method"] --> B["beta_browser_version method"]
  B --> C["Chrome beta version fetch"]
  B --> D["Firefox beta version fetch"]
  C --> E["Updated test guards"]
  D --> E
Loading

File Walkthrough

Relevant files
Enhancement
test_environment.rb
Refactor version method to support multiple browsers         

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

  • Renamed beta_chrome_version to beta_browser_version with browser
    parameter
  • Added Firefox beta version fetching from Mozilla API
  • Implemented case statement for browser-specific version retrieval
+16/-6   
Tests
manager_spec.rb
Update Firefox beta guard usage                                                   

rb/spec/integration/selenium/webdriver/manager_spec.rb

  • Updated cookie test guard to use new beta_browser_version(:firefox)
    method
+1/-1     
network_spec.rb
Update Chrome beta guard usage                                                     

rb/spec/integration/selenium/webdriver/network_spec.rb

  • Updated network test guard to use new beta_browser_version(:chrome)
    method
+1/-1     

@selenium-ci selenium-ci added the C-rb Ruby Bindings label Aug 9, 2025
Copy link
Contributor

qodo-merge-pro bot commented Aug 9, 2025

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

Possible Issue

Error-handling in beta_browser_version returns a string on HTTP failure, which changes the return type from a version string to an error message and may break guards expecting a version pattern. Consider raising or returning nil and handling upstream.

  return "Failed to fetch Chrome Beta page: #{response.code}" unless response.is_a?(Net::HTTPSuccess)

  response.body.match(/\d+\.\d+\.\d+\.\d+/).to_s

when :firefox
  uri = URI.parse('https://product-details.mozilla.org/1.0/firefox_versions.json')
  response = Net::HTTP.get_response(uri)
  return "Failed to fetch Firefox version API: #{response.code}" unless response.is_a?(Net::HTTPSuccess)

  versions = JSON.parse(response.body)
  versions['LATEST_FIREFOX_RELEASED_DEVEL_VERSION']

else
  raise ArgumentError, "Unsupported browser: #{browser}"
end
Robustness

Chrome beta version parsing relies on the first \d+.\d+.\d+.\d+ match in the blog HTML, which is brittle. Consider a more specific selector or fallback if no match is found to avoid empty strings.

when :chrome
  uri = URI.parse('https://chromereleases.googleblog.com/search/label/Beta%20updates')
  response = Net::HTTP.get_response(uri)
  return "Failed to fetch Chrome Beta page: #{response.code}" unless response.is_a?(Net::HTTPSuccess)

  response.body.match(/\d+\.\d+\.\d+\.\d+/).to_s

when :firefox
Data Source Assumption

Firefox uses LATEST_FIREFOX_RELEASED_DEVEL_VERSION from Mozilla API. Verify this key aligns with desired "beta" for guard logic; consider validating presence and handling nil.

  uri = URI.parse('https://product-details.mozilla.org/1.0/firefox_versions.json')
  response = Net::HTTP.get_response(uri)
  return "Failed to fetch Firefox version API: #{response.code}" unless response.is_a?(Net::HTTPSuccess)

  versions = JSON.parse(response.body)
  versions['LATEST_FIREFOX_RELEASED_DEVEL_VERSION']

else

Copy link
Contributor

qodo-merge-pro bot commented Aug 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Make guards deterministic

The new guards depend on scraping live beta version endpoints at test runtime,
which introduces network flakiness, external dependency drift, and
non-deterministic exclusions across CI runs. Cache or version-pin these values
(e.g., via build-time fetch with fallback, environment variables, or a
maintained mapping) so test selection is stable and doesn’t fail or skip
unpredictably when endpoints change or are unreachable.

Examples:

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [196-216]
def beta_browser_version(browser)
  case browser
  when :chrome
    uri = URI.parse('https://chromereleases.googleblog.com/search/label/Beta%20updates')
    response = Net::HTTP.get_response(uri)
    return "Failed to fetch Chrome Beta page: #{response.code}" unless response.is_a?(Net::HTTPSuccess)

    response.body.match(/\d+\.\d+\.\d+\.\d+/).to_s

  when :firefox

 ... (clipped 11 lines)

Solution Walkthrough:

Before:

def beta_browser_version(browser):
  if browser is :chrome:
    # Make live HTTP request to Google blog
    # Scrape version string from HTML body
    return version
  else if browser is :firefox:
    # Make live HTTP request to Mozilla JSON API
    # Parse JSON and extract version
    return version

After:

# In a config file, environment variable, or fetched at build time
BETA_VERSIONS = {
  chrome: "123.0.6312.46",
  firefox: "124.0b9"
}

def beta_browser_version(browser):
  # Read from a stable, version-controlled source
  return BETA_VERSIONS[browser]
Suggestion importance[1-10]: 9

__

Why: This suggestion addresses a critical design flaw, as relying on live network calls for test configuration introduces non-determinism and flakiness, compromising CI stability.

High
Possible issue
Scope version guard to Firefox
Suggestion Impact:The commit added browser: :firefox to the except guard, scoping the exception to Firefox as suggested.

code diff:

-             except: [{version: GlobalTestEnv.beta_browser_version(:firefox),
+             except: [{browser: :firefox,
                        reason: 'https://github.com/mozilla/geckodriver/issues/1842'},
                       {browser: %i[safari safari_preview]}] do

The except guard mixes a version exclusion with browser exclusions but omits the
browser key for the version condition. Add the browser: :firefox key to ensure
the version comparison only applies to Firefox and doesn't accidentally affect
other browsers.

rb/spec/integration/selenium/webdriver/manager_spec.rb [172-174]

-except: [{version: GlobalTestEnv.beta_browser_version(:firefox),
+except: [{browser: :firefox, version: GlobalTestEnv.beta_browser_version(:firefox),
            reason: 'https://github.com/mozilla/geckodriver/issues/1842'},
          {browser: %i[safari safari_preview]}]

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a bug introduced in the PR where removing the browser: :firefox key would cause the version exception to apply to all browsers, not just Firefox.

High
Bind version exclusion to Chrome

As with other guards, tie the version exclusion to the intended browser to avoid
unintended exclusion across all browsers. Include browser: :chrome alongside the
version to correctly target Chrome beta.

rb/spec/integration/selenium/webdriver/network_spec.rb [24-26]

-describe Network, exclude: {version: GlobalTestEnv.beta_browser_version(:chrome)},
+describe Network, exclude: {browser: :chrome, version: GlobalTestEnv.beta_browser_version(:chrome)},
                   exclusive: {bidi: true, reason: 'only executed when bidi is enabled'},
                   only: {browser: %i[chrome edge firefox]} do
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a bug where the version exclusion lacks a browser: :chrome key, which would incorrectly apply the exclusion to all browsers running the tests, not just Chrome.

High
Validate and fail fast on fetch/parse errors

Returning error strings or empty matches can silently break guards expecting a
version string. Raise on HTTP errors and nil/empty matches so callers don't
misinterpret failures as valid versions. Also validate the Firefox JSON key
exists and is non-empty.

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [196-216]

 def beta_browser_version(browser)
   case browser
   when :chrome
     uri = URI.parse('https://chromereleases.googleblog.com/search/label/Beta%20updates')
     response = Net::HTTP.get_response(uri)
-    return "Failed to fetch Chrome Beta page: #{response.code}" unless response.is_a?(Net::HTTPSuccess)
+    raise "Failed to fetch Chrome Beta page: #{response.code}" unless response.is_a?(Net::HTTPSuccess)
 
-    response.body.match(/\d+\.\d+\.\d+\.\d+/).to_s
-
+    version = response.body.match(/\d+\.\d+\.\d+\.\d+/)&.to_s
+    raise 'Failed to parse Chrome beta version' if version.nil? || version.empty?
+    version
   when :firefox
     uri = URI.parse('https://product-details.mozilla.org/1.0/firefox_versions.json')
     response = Net::HTTP.get_response(uri)
-    return "Failed to fetch Firefox version API: #{response.code}" unless response.is_a?(Net::HTTPSuccess)
+    raise "Failed to fetch Firefox version API: #{response.code}" unless response.is_a?(Net::HTTPSuccess)
 
     versions = JSON.parse(response.body)
-    versions['LATEST_FIREFOX_RELEASED_DEVEL_VERSION']
-
+    version = versions['LATEST_FIREFOX_RELEASED_DEVEL_VERSION']
+    raise 'Failed to parse Firefox beta version' if version.nil? || version.empty?
+    version
   else
     raise ArgumentError, "Unsupported browser: #{browser}"
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that returning error strings or nil can lead to silent failures in test guards, and improves robustness by raising explicit exceptions on fetch or parse failures.

Medium
  • Update

@aguspe aguspe changed the title Rb add guard for beta firefox [rb] Add guard for beta firefox Aug 10, 2025
@aguspe aguspe added the A-needs decision TLC needs to discuss and agree label Aug 10, 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.

I think rather than trying to figure out which Firefox/Chrome is beta, we can explicitly tell the test it's being run on beta versions - we already know that from Bazel:

"chrome-beta": {
"data": chrome_beta_data,
"deps": ["//rb/lib/selenium/webdriver:chrome"],
"tags": [],
"target_compatible_with": [],
"env": {
"WD_REMOTE_BROWSER": "chrome",
"WD_SPEC_DRIVER": "chrome",
"WD_BROWSER_VERSION": "beta",
} | select({

"firefox": {
"data": firefox_data,
"deps": ["//rb/lib/selenium/webdriver:firefox"],
"tags": [],
"target_compatible_with": [],
"env": {
"WD_REMOTE_BROWSER": "firefox",
"WD_SPEC_DRIVER": "firefox",
} | select({

You can see Chrome already has WD_BROWSER_VERSION=beta. We can add it to firefox-beta as well, then adapt the guard version to read from this environment variable:

describe Network, exclude: {version: 'beta'}

@aguspe aguspe removed the A-needs decision TLC needs to discuss and agree label Aug 11, 2025
@aguspe
Copy link
Contributor Author

aguspe commented Aug 11, 2025

@p0deje thank you for the idea! I like that solution way more

@aguspe aguspe merged commit 1c18799 into SeleniumHQ:trunk Aug 11, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants