[rb] Allow to set environment variables when staring a browser#17395
[rb] Allow to set environment variables when staring a browser#17395y-yagi wants to merge 6 commits into
Conversation
Review Summary by QodoAllow setting environment variables when starting a browser
WalkthroughsDescription• Add ability to set environment variables when starting browser • Allow explicit env variable configuration via Service class • Pass env variables through ChildProcess to Process.spawn • Add comprehensive unit tests for env variable handling File Changes1. rb/lib/selenium/webdriver/common/child_process.rb
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| allow(Process).to receive(:spawn).and_return(12_345) | ||
| allow(Process).to receive(:detach) | ||
|
|
||
| process.start | ||
|
|
||
| expect(Process).to have_received(:spawn).with({'FOO' => 'bar'}, 'echo', 'test', anything) | ||
| end |
There was a problem hiding this comment.
1. Mocks used in child_process_spec 📘 Rule violation ⚙ Maintainability
The newly added unit tests stub Process.spawn and Process.detach, which violates the requirement to avoid mocks in tests and can reduce fidelity to real behavior.
Agent Prompt
## Issue description
The new unit tests for `ChildProcess#start` use mocks/stubs (`allow(Process).to receive(...)`) and mock assertions (`have_received`), which violates the project's guidance to avoid mocks in tests.
## Issue Context
This PR adds support for passing environment variables to `Process.spawn`. The tests currently validate this by stubbing Ruby's `Process` module.
## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/child_process_spec.rb[25-47]
## Suggested direction
- Prefer an integration-like unit test that executes a very small real command and inspects its output/environment (e.g., spawn a Ruby one-liner that prints `ENV['FOO']`) rather than mocking `Process.spawn`.
- If platform differences are a concern, gate the test appropriately (e.g., skip on Windows if needed) while still avoiding mocks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Adds support in the Ruby bindings to pass an explicit environment hash into the driver service process launch, enabling callers to avoid inheriting (or to override) the Ruby process environment when starting a browser via its driver.
Changes:
- Add
envconfiguration toWebDriver::Serviceand propagate it throughServiceManagerintoChildProcess. - Update
ChildProcessto pass the env hash as the first argument toProcess.spawn. - Add unit tests covering env passing in
ChildProcess#startandChrome::Serviceinitialization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rb/lib/selenium/webdriver/common/service.rb | Introduces env attribute/keyword on the base Service configuration. |
| rb/lib/selenium/webdriver/common/service_manager.rb | Passes the configured env through to the spawned child process. |
| rb/lib/selenium/webdriver/common/child_process.rb | Uses Process.spawn(env, *command, options) to apply env overrides. |
| rb/spec/unit/selenium/webdriver/common/child_process_spec.rb | Adds specs asserting Process.spawn receives the env hash (and {} by default). |
| rb/spec/unit/selenium/webdriver/chrome/service_spec.rb | Adds a spec asserting Chrome::Service retains the provided env. |
| def initialize(path: nil, port: nil, log: nil, args: nil, env: nil) | ||
| port ||= self.class::DEFAULT_PORT |
There was a problem hiding this comment.
Service now accepts an env: keyword, but at least one concrete subclass (Safari::Service) overrides initialize without env:/** and will raise ArgumentError: unknown keyword: :env when users try to use this new option. Update the Safari service initializer to accept/pass through env: (or switch to **) so the feature is consistently available across drivers.
|
|
||
| WebDriver.logger.debug("Starting process: #{@command} with #{options}", id: :process) | ||
| @pid = Process.spawn(*@command, options) | ||
| @pid = Process.spawn(@env, *@command, options) |
There was a problem hiding this comment.
env is an attr_accessor, so it can be set to nil (or another non-Hash) after initialization; Process.spawn expects the optional first argument to be a Hash and will raise a TypeError if it isn't. Consider coercing to {} at the call site (or validating in env= and raising a more actionable WebDriver error) to make service startup resilient to nil/invalid env values.
y-yagi
force-pushed
the
allow_to_set_env
branch
from
3725918 to
51809ef
Compare
y-yagi
force-pushed
the
allow_to_set_env
branch
from
51809ef to
2e5da27
Compare
| it 'spawns process with environment variables' do | ||
| process = described_class.new('ruby', '-e', 'puts ENV["SELENIUM_TEST_VAR"]') | ||
| process.env = {'SELENIUM_TEST_VAR' => 'test_value'} |
There was a problem hiding this comment.
This spec spawns 'ruby' from PATH, which can be a different interpreter than the one running the test suite (and can fail in some CI setups). Use the current Ruby executable (e.g., RbConfig.ruby) for the child process command to make the test deterministic.
There was a problem hiding this comment.
The code is very simple and it would provide same behavior on all versions.
| def initialize(path: nil, port: nil, log: nil, args: nil, env: nil) | ||
| raise Error::WebDriverError, 'Safari Service does not support setting log output' if log |
There was a problem hiding this comment.
Safari::Service currently lists all keyword parameters explicitly. Other service subclasses (e.g., Chrome/Edge/Firefox) use def initialize(args: nil, **) and then super, which avoids needing to update this signature whenever the base WebDriver::Service adds a new config keyword (as happened here for env). Consider switching Safari::Service to accept ** (while still enforcing the log restriction) to make this more future-proof.
| def initialize(path: nil, port: nil, log: nil, args: nil, env: nil) | |
| raise Error::WebDriverError, 'Safari Service does not support setting log output' if log | |
| def initialize(args: nil, **opts) | |
| raise Error::WebDriverError, 'Safari Service does not support setting log output' if opts[:log] |
There was a problem hiding this comment.
I thinks the current implementation is better.
Currently, the environment variables used when launching a browser are the same as those of the Ruby process that performs the launch. However, there are times when want to change the environment variables passed to a browser. In our case, `LD_PRELOAD`. We want to set `LD_PRELOAD` to use jemalloc in Ruby, but we want to avoid setting it for Chrome because using jemalloc can cause crashes or unstable behavior. To address this issue, this PR fixes allow environment variables to be explicitly specified via the service.
y-yagi
force-pushed
the
allow_to_set_env
branch
from
2e5da27 to
4572e98
Compare
|
Nice @y-yagi! Before we merge, the new
Otherwise looks good to me! :) |
|
Persistent review updated to latest commit 2fbb347 |
Close Tempfile before spawning process to avoid EACCES on Windows. On Windows, an open file handle can prevent spawned processes from opening the same file for redirection, causing test failures.
|
Persistent review updated to latest commit 5ccc267 |
|
@aguspe |
|
Persistent review updated to latest commit c850774 |
JRuby environments may not have 'ruby' in PATH. Use RbConfig.ruby to get the path to the current Ruby interpreter instead.
|
Persistent review updated to latest commit adcba78 |
|
https://github.com/SeleniumHQ/selenium/actions/runs/25625296761/job/75219325680 has failed. I added the commit for fix it. |
🔗 Related Issues
None
💥 What does this PR do?
Currently, the environment variables used when launching a browser are the same as those of the Ruby process that performs the launch.
However, there are times when want to change the environment variables passed to a browser. In our case,
LD_PRELOAD. We want to setLD_PRELOADto use jemalloc in Ruby, but we want to avoid setting it for Chrome because using jemalloc can cause crashes or unstable behavior.To address this issue, this PR fixes allow environment variables to be explicitly specified via the service.
🔧 Implementation Notes
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes