Skip to content
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.

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

Add headless chrome driver to System Tests #30876

Merged
merged 1 commit into from Oct 17, 2017

Conversation

@y-yagi
Copy link
Member

@y-yagi y-yagi commented Oct 13, 2017

Currently, Headless Chrome seems to be stable, I think there is no problem in using it for testing.
Therefore, I think that it is good to add to the driver that System test supports by default.

r? @eileencodes

actionpack/lib/action_dispatch/system_testing/driver.rb Outdated
Capybara::Selenium::Driver.new(app, { browser: :chrome, options: browser_options }.merge(@options)).tap do |driver|
driver.browser.manage.window.size = Selenium::WebDriver::Dimension.new(*@screen_size)
end
end

This comment has been minimized.

@eileencodes

eileencodes Oct 15, 2017
Member

Let's follow the pattern of other drivers instead. Selenium is still the default driver, and then the option for the browser would be :headless_chrome. Which means instead of driven_by :selenium_headless_chrome this should be driven_by :selenium, using: :headless_chrome.

This comment has been minimized.

@y-yagi

y-yagi Oct 15, 2017
Author Member

Thanks for your review! It makes sense. I updated to use using option.

@eileencodes eileencodes added this to the 5.2.0 milestone Oct 15, 2017
@y-yagi y-yagi force-pushed the y-yagi:selenium_chrome_headless branch Oct 15, 2017
actionpack/lib/action_dispatch/system_testing/driver.rb Outdated
browser_options.args << "--headless"
browser_options.args << "--disable-gpu"

Capybara::Selenium::Driver.new(app, { browser: :chrome, options: browser_options }.merge(@options)).tap do |driver|

This comment has been minimized.

@eileencodes

eileencodes Oct 16, 2017
Member

This feels repetitive, with the only reason we need an extra method is because we need to merge options: browser_options.

What do you think about something like this:

def browser_options
  if @browser == :headless_chrome
    @options.merge(options: "--headless --disable-gpu")
  else
    @options
  end
end

def register_selenium(app)
  Capybara::Selenium::Driver.new(app, { browser: @browser }.merge(browser_options)).tap do |driver|
    driver.browser.manage.window.size = Selenium::WebDriver::Dimension.new(*@screen_size)
  end
end

This comment has been minimized.

@y-yagi

y-yagi Oct 16, 2017
Author Member

👍 Since that can not be passed by options, I just changed it and updated!

@y-yagi y-yagi force-pushed the y-yagi:selenium_chrome_headless branch to ada0585 Oct 16, 2017
@eileencodes eileencodes merged commit 683ec10 into rails:master Oct 17, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@y-yagi y-yagi deleted the y-yagi:selenium_chrome_headless branch Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants