Home Original page

[rb] Update Chrome/Edge args for test environment by cgoldberg · Pull Request #16376 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Avoid mutable default arguments

Do not use a mutable default array for args; default to nil and initialize
inside the method to avoid unintended state sharing between calls.

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [273-280]

-def chrome_options(args: [], **opts)
+def chrome_options(args: nil, **opts)
+  args = (args || []).dup
   opts[:browser_version] = 'stable' if WebDriver::Platform.windows?
   opts[:web_socket_url] = true if ENV['WEBDRIVER_BIDI'] && !opts.key?(:web_socket_url)
   opts[:binary] ||= ENV['CHROME_BINARY'] if ENV.key?('CHROME_BINARY')
   args << '--headless' if ENV['HEADLESS']
   args << '--no-sandbox' unless Platform.windows?
   WebDriver::Options.chrome(args: args, **opts)
 end

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early to prevent logic errors; avoid mutating shared default arguments across calls.

Low
Avoid mutable default arguments

Replace the mutable default array for args with nil and initialize within the
method to prevent cross-invocation state leakage.

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [282-289]

-def edge_options(args: [], **opts)
+def edge_options(args: nil, **opts)
+  args = (args || []).dup
   opts[:browser_version] = 'stable' if WebDriver::Platform.windows?
   opts[:web_socket_url] = true if ENV['WEBDRIVER_BIDI'] && !opts.key?(:web_socket_url)
   opts[:binary] ||= ENV['EDGE_BINARY'] if ENV.key?('EDGE_BINARY')
   args << '--headless' if ENV['HEADLESS']
   args << '--no-sandbox' unless Platform.windows?
   WebDriver::Options.edge(args: args, **opts)
 end
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Validate inputs and states early to prevent logic errors; avoid mutating shared default arguments across calls.

Low
Possible issue
Restore a flag for headless stability

Conditionally re-add the --disable-gpu flag when running Edge in headless mode
to improve stability and prevent potential rendering issues in CI/CD
environments.

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [282-289]

 def edge_options(args: [], **opts)
   opts[:browser_version] = 'stable' if WebDriver::Platform.windows?
   opts[:web_socket_url] = true if ENV['WEBDRIVER_BIDI'] && !opts.key?(:web_socket_url)
   opts[:binary] ||= ENV['EDGE_BINARY'] if ENV.key?('EDGE_BINARY')
-  args << '--headless' if ENV['HEADLESS']
+  if ENV['HEADLESS']
+    args << '--headless'
+    args << '--disable-gpu'
+  end
   args << '--no-sandbox' unless Platform.windows?
   WebDriver::Options.edge(args: args, **opts)
 end
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid point about potential test flakiness by re-adding the --disable-gpu flag for headless runs, but this contradicts the PR's explicit removal of a flag that is officially considered unnecessary with the new headless mode.

Low
  • More