[rb] fix using environment variables to set drivers#17571
Open
titusfortner wants to merge 2 commits into
Open
Conversation
Contributor
Review Summary by QodoFix environment variables in driver path resolution
WalkthroughsDescription• Move environment variable check to DriverFinder for consistent resolution • Remove redundant env_path calls from Service initialization • Ensure environment variables apply in typical driver usage patterns • Refactor path resolution order: service path, env vars, then class path Diagramflowchart LR
A["Service.initialize"] -->|removed env_path| B["Service.launch"]
B -->|calls DriverFinder| C["DriverFinder.paths"]
C -->|checks in order| D["service.executable_path"]
D -->|or| E["env_path"]
E -->|or| F["class_path"]
F -->|returns| G["driver executable"]
File Changes1. rb/lib/selenium/webdriver/common/driver_finder.rb
|
Contributor
Code Review by Qodo
1. Missing Safari::Options#browser_name=
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| def initialize(path: nil, port: nil, log: nil, args: nil) | ||
| port ||= self.class::DEFAULT_PORT | ||
| args ||= [] | ||
| path ||= env_path |
Contributor
There was a problem hiding this comment.
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
| self.class::SHUTDOWN_SUPPORTED | ||
| end | ||
|
|
||
| def env_path |
Contributor
There was a problem hiding this comment.
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
Contributor
|
Persistent review updated to latest commit afac4b5 |
Comment on lines
+39
to
+40
| def browser_name | ||
| @options[:browser_name] = Safari.technology_preview? ? TECHNOLOGY_PREVIEW : BROWSER |
Contributor
There was a problem hiding this comment.
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
| raise Error::WebDriverError, 'Safari is only supported on Mac' unless Platform.os.mac? | ||
|
|
||
| raise Error::WebDriverError, 'Unable to find Safari' | ||
| @path ||= nil |
Contributor
There was a problem hiding this comment.
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
Comment on lines
+39
to
+40
| def browser_name | ||
| @options[:browser_name] = Safari.technology_preview? ? TECHNOLOGY_PREVIEW : BROWSER |
Contributor
There was a problem hiding this comment.
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
Contributor
CI Feedback 🧐(Feedback updated until commit afac4b5)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
the current implementation doesn't apply to the typical use case
Unless a user is attempting to start the driver service before initializing the driver, the environment variable won't be used
💥 What does this PR do?
Builds on #17564 to put the environment variable check where the service class check is in the DriverFinder
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes