Selenium: [rb] Options overwrite custom desired capabilities

[artplan1]

Meta

Selenium-rb gem version:
selenium-webdriver (3.13.0)

Browser:
Chrome 67.0.3396.87 (Official Build) (64-bit)

Driver setup

Capybara.register_driver(:selenium_chrome_headless) do |app|
  capabilities = Selenium::WebDriver::Remote::Capabilities.chrome(
    'goog:chromeOptions': { args: %w[no-sandbox headless disable-gpu window-size=1024,768] }
  )

  Capybara::Selenium::Driver.new(app, browser: :chrome, desired_capabilities: capabilities)
end

I know I can setup args using args (deprecated) or options

Expected Behavior

Create driver with given capabilities

Actual Behavior

Options overwrite custom capabilities because of https://github.com/SeleniumHQ/selenium/blob/master/rb/lib/selenium/webdriver/chrome/driver.rb#L106

caps.merge!(options) unless options.empty?

options here is not empty. It equals {"goog:chromeOptions" => {} }

Steps to reproduce

Added spec and possible fix in:
https://github.com/artplan1/selenium/commit/9b14903b44b8136f418ce520aa564150603244d5

Possible fix:

caps.merge!(options) unless options[Options::KEY].empty?

or as_json in Options.rb can return {} instead of {"goog:chromeOptions" => {} }
not sure if it's required with recent changes

def as_json(*)
...
  opts ? {KEY => opts} : {}
end

Spec:

it 'does not merge empty options' do
  custom_caps = Remote::Capabilities.new('goog:chromeOptions' => {args: %w[foo bar]})

  expect(http).to receive(:call) do |_, _, payload|
    expect(payload[:desiredCapabilities]['goog:chromeOptions'][:args]).to eq(%w[foo bar])
    resp
  end

  Driver.new(http_client: http, desired_capabilities: custom_caps)
end

Thanks

[connorshea]

As I mentioned in #6077, I think this is the source of that issue. It's causing my project issues when upgrading to 3.13.0. Thanks for the thorough bug report @artplan1, I would have never figured this out! :D

[connorshea]

I tested your patch and it fixed the problem! 👏

[connorshea]

Using this in the Gemfile should do it if someone wants to test this solution (Note that github in Bundler 1.x isn't secure unless you override the source like git_source(:github) { |repo| "https://github.com/#{repo}.git" }):

gem 'selenium-webdriver', github: 'artplan1/selenium', ref: '9b14903b44b8136f418ce520aa564150603244d5', glob: 'rb/*.gemspec'

I wouldn't recommend it as a long-term solution though.

@artplan1 do you intend to open a PR with this fix?

[artplan1]

@connorshea thanks for testing

yea, I wanted to open PR, but i don't feel good about this code actually.

options[Options::KEY].empty?

checking for key here feels dirty to me 🤔

I like changing return of as_json to opts ? {KEY => opts} : {} more, but not sure if even empty goog:chromeOptions is required

I wanted collaborators opinion on this 🙂

[connorshea]

@artplan1 I'd say the best way to get feedback is to open a PR and ask for some ;)

[luke-hill]

I'm an open user and partial contributor to selenium. and modifying the as_json method feels a bit suspect to me.

It would be better to have that method still operate as intended. And then clean up / sanitize the method as you've proposed in your initial code stub.

Plus from a large scale project perspective, having an extra guard/modification line into a chunky method is easy to triage. Rewriting a method which is used across the suite feels more suspect to issues down the line (Especially when the whole W3C/OSS argument comes into play)

TL;DR - Use the first methodology with a spec. It's better - My 0.02c