Skip to content

[rb] fix using environment variables to set drivers #17571

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.

Sign up for GitHub

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

Open
titusfortner wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

[rb] fix using environment variables to set drivers #17571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab591c4
[rb] fix using environment variables to set drivers
titusfortner May 25, 2026
afac4b5
let selenium manager handle safari paths instead of hard coding
titusfortner May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions rb/lib/selenium/webdriver/common/driver_finder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def browser_path?

def paths
@paths ||= begin
path = @service.executable_path || resolve_class_path
path = @service.executable_path || env_path || class_path
path ? paths_from_service(path) : paths_from_manager
rescue StandardError => e
WebDriver.logger.error("Exception occurred: #{e.message}")
Expand All @@ -58,7 +58,11 @@ def paths
end
end

def resolve_class_path
def env_path
ENV.fetch(@service.class::DRIVER_PATH_ENV_KEY, nil)
end

def class_path
path = @service.class.driver_path
path.is_a?(Proc) ? path.call : path
end
Expand Down
7 changes: 1 addition & 6 deletions rb/lib/selenium/webdriver/common/service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ def driver_path=(path)
def initialize(path: nil, port: nil, log: nil, args: nil)
port ||= self.class::DEFAULT_PORT
args ||= []
path ||= env_path
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 25, 2026

Choose a reason for hiding this comment

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

Action required

2. Env var service init regression 🐞 Bug ≡ Correctness

Service#initialize no longer applies the DRIVER_PATH_ENV_KEY environment variable, but existing unit
specs expect Service.new.executable_path to reflect that env var immediately. This will fail
multiple unit tests and is a behavioral break for any caller reading executable_path before
DriverFinder/launch runs.
Agent Prompt 
## Issue description
`Service#initialize` no longer reads the driver-path environment variable (e.g., `SE_CHROMEDRIVER`) to populate `@executable_path`. The repo’s unit specs currently assert that `Service.new.executable_path` is derived from the env var, so the change will cause test failures and breaks the previously-tested behavior.

## Issue Context
The env-var lookup was removed from `Service#initialize` and moved into `DriverFinder`. That makes env vars apply later (when DriverFinder runs), but it also means `service.executable_path` stays `nil` after construction.

## Fix Focus Areas
Choose one consistent behavior and align code + tests:

- **Preserve existing behavior (least breaking):** make `Service#executable_path` (reader) fall back to ENV when `@executable_path` is nil (or re-introduce env assignment in `initialize`).
- **If behavior change is intended:** update the affected unit specs to assert env usage via `DriverFinder` or `Service#launch`, not via `Service#executable_path` immediately after `new`.

Relevant locations:
- rb/lib/selenium/webdriver/common/service.rb[69-92]
- rb/spec/unit/selenium/webdriver/chrome/service_spec.rb[153-173]
- rb/spec/unit/selenium/webdriver/safari/service_spec.rb[118-138]
- rb/spec/unit/selenium/webdriver/edge/service_spec.rb[163-183]
- rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[215-236]
- rb/spec/unit/selenium/webdriver/ie/service_spec.rb[153-173]
- rb/spec/unit/selenium/webdriver/common/driver_finder_spec.rb[22-99]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


@executable_path = path
@host = Platform.localhost
Expand All @@ -88,18 +87,14 @@ def initialize(path: nil, port: nil, log: nil, args: nil)
end

def launch
@executable_path ||= env_path || DriverFinder.new(nil, self).driver_path
@executable_path ||= DriverFinder.new(nil, self).driver_path
ServiceManager.new(self).tap(&:start)
end

def shutdown_supported
self.class::SHUTDOWN_SUPPORTED
end

def env_path
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 25, 2026

Choose a reason for hiding this comment

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

Action required

1. service#env_path removed 📘 Rule violation ≡ Correctness

Selenium::WebDriver::Service#env_path was removed without a deprecation path, which is a breaking
change for callers and leaves the RBI/RBS contract inconsistent with the runtime implementation.
This can trigger runtime NoMethodError for existing consumers and also break RBS/Steep validation
during upgrades.
Agent Prompt 
## Issue description
`Selenium::WebDriver::Service#env_path` was removed from the Ruby implementation without a deprecation path, but the corresponding RBS signature still declares it, creating an API break for callers and leaving the type/signature contract out of sync with runtime behavior.

## Issue Context
- The Ruby signature file still exposes `env_path` as a public method (`def env_path`).
- The `Service` implementation no longer defines `env_path`, so existing callers will hit `NoMethodError` at runtime.
- The mismatch will also fail RBS/Steep (or other signature validation) because the method is declared but not implemented.
- Compliance requires maintaining API compatibility and deprecating public functionality before removal.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/service.rb[89-105]
- rb/sig/lib/selenium/webdriver/common/service.rbs[48-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ENV.fetch(self.class::DRIVER_PATH_ENV_KEY, nil)
end

private

def warn_driver_log_override
Expand Down
7 changes: 1 addition & 6 deletions rb/lib/selenium/webdriver/safari.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ def technology_preview
end

def technology_preview!
Service.driver_path = technology_preview
@use_technology_preview = true
end

Expand All @@ -47,11 +46,7 @@ def path=(path)
end

def path
@path ||= '/Applications/Safari.app/Contents/MacOS/Safari'
return @path if File.file?(@path) && File.executable?(@path)
raise Error::WebDriverError, 'Safari is only supported on Mac' unless Platform.os.mac?

raise Error::WebDriverError, 'Unable to find Safari'
@path ||= nil
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 25, 2026

Choose a reason for hiding this comment

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

Action required

2. safari.path now nil 📘 Rule violation ≡ Correctness

Selenium::WebDriver::Safari.path now defaults to nil, removing the previous behavior of
providing/validating a default Safari binary path and raising actionable errors. This is a
user-visible behavior change that can break callers relying on a non-nil default or on the earlier
deterministic exceptions.
Agent Prompt 
## Issue description
`Safari.path` now memoizes `nil`, changing the public method’s behavior so it no longer provides a default Safari path nor performs early validation/OS checks. This is a backward-incompatible, user-visible behavior change.

## Issue Context
`Safari.path` is a public singleton method. With the new implementation, any caller expecting a default path (or early, actionable errors) will instead receive `nil` and may fail later with less clear errors.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari.rb[48-50]

## Suggested approach
- Reintroduce the previous default path and validation/error behavior (e.g., default to the Safari binary on macOS and raise a clear `WebDriverError` when unsupported/unavailable).
- If the new `nil` default is intentional, add a deprecation path and update callers/documentation accordingly so upgrades do not silently change behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

end
end
end # Safari
Expand Down
8 changes: 4 additions & 4 deletions rb/lib/selenium/webdriver/safari/options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ class Options < WebDriver::Options
# @see https://developer.apple.com/documentation/webkit/about_webdriver_for_safari
CAPABILITIES = {automatic_inspection: 'safari:automaticInspection',
automatic_profiling: 'safari:automaticProfiling'}.freeze
BROWSER = Selenium::WebDriver::Safari.technology_preview? ? 'Safari Technology Preview' : 'safari'
BROWSER = 'safari'
TECHNOLOGY_PREVIEW = 'Safari Technology Preview'

def add_option(name, value = nil)
key = name.is_a?(Hash) ? name.keys.first : name
Expand All @@ -35,9 +36,8 @@ def add_option(name, value = nil)
super
end

def as_json(*)
@options[:browser_name] = Safari.technology_preview? ? 'Safari Technology Preview' : 'safari'
super
def browser_name
@options[:browser_name] = Safari.technology_preview? ? TECHNOLOGY_PREVIEW : BROWSER
Comment on lines +39 to +40
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 25, 2026

Choose a reason for hiding this comment

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

Action required

1. Missing safari::options#browser_name= 📘 Rule violation ≡ Correctness

Defining Safari::Options#browser_name prevents the base Options.set_capabilities from generating
the standard browser_name= writer, breaking API compatibility for consumers who set W3C
capabilities via the normal setter. It also forces a computed value on every read, making overrides
impossible and potentially changing downstream capability payloads.
Agent Prompt 
## Issue description
`Safari::Options` now defines a custom `browser_name` method, which prevents `Selenium::WebDriver::Options.set_capabilities` from generating the normal `browser_name=` writer. This is a backward-incompatible API change and also makes it impossible for callers to override `browser_name`.

## Issue Context
`Options.set_capabilities` only defines both getter and setter when the getter method is not already defined. Because `Safari::Options` defines `browser_name`, the pair is skipped.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[39-41]
- rb/lib/selenium/webdriver/common/options.rb[54-66]

## Suggested approach
- Prefer restoring the prior pattern: remove the `browser_name` override and instead override `as_json` (or another serialization hook) to set `@options[:browser_name]` based on `Safari.technology_preview?` right before serialization; this keeps the standard accessor pair (`browser_name` and `browser_name=`) available.
- If keeping `browser_name` is required, explicitly add a compatible `browser_name=` writer and ensure `browser_name` does not overwrite user-provided values unintentionally.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +39 to +40
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 25, 2026

Choose a reason for hiding this comment

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

Action required

3. Safari tp browsername not set 🐞 Bug ≡ Correctness

Safari::Options#browser_name now mutates @options[:browser_name], but the driver creation path
serializes capabilities via options.as_json without ever calling browser_name, so browserName
remains the initialize default 'safari' even when Safari.technology_preview! is enabled. This
breaks the Safari Technology Preview flow that expects browser_name to be `'Safari Technology
Preview'` when the flag is set before or after options are created.
Agent Prompt 
## Issue description
`Safari::Options#browser_name` updates `@options[:browser_name]`, but the code path that builds capabilities for a local driver uses `options.as_json` and does not call `browser_name`. As a result, enabling `Safari.technology_preview!` does not change the serialized W3C `browserName` from `'safari'` to `'Safari Technology Preview'`.

## Issue Context
`WebDriver::Options#as_json` serializes from a dup of `@options` and does not invoke getters, so any logic placed only in `Safari::Options#browser_name` is skipped unless called explicitly.

## Fix Focus Areas
- rb/lib/selenium/webdriver/safari/options.rb[29-41]

## Suggested fix
Add/restore a Safari::Options `as_json` override that ensures the `browser_name` logic runs before calling `super`, e.g.:

```ruby
def as_json(*)
  browser_name # ensures @options[:browser_name] reflects technology_preview?
  super
end
```

(Alternatively, set `@options[:browser_name]` in a hook that is guaranteed to run before serialization, but `as_json` is the minimal, spec-aligned place given existing driver initialization flow.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

end
end # Options
end # Safari
Expand Down
4 changes: 3 additions & 1 deletion rb/sig/lib/selenium/webdriver/common/driver_finder.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ module Selenium

def paths: -> untyped

def resolve_class_path: -> String?
def class_path: -> String?

def env_path: -> String?

def paths_from_service: (String path) -> Hash[Symbol, String]

Expand Down
6 changes: 4 additions & 2 deletions rb/sig/lib/selenium/webdriver/safari/options.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ module Selenium

CAPABILITIES: Hash[Symbol, String]

BROWSER: untyped
BROWSER: String

TECHNOLOGY_PREVIEW: String

def add_option: (untyped name, ?untyped? value) -> untyped

def as_json: (*untyped) -> untyped
def browser_name: () -> String
end
end
end
Expand Down
Loading