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

SystemTesting::Driver can register capybara-webkit and poltergeist #29315

Merged
merged 1 commit into from Jun 2, 2017

Conversation

Projects
None yet
5 participants
@mariochavez
Contributor

mariochavez commented Jun 1, 2017

drivers.

When using driver_by with capybara-webkit or poltergeist,
SystemTesting::Driver will register the driver while passing
screen_size and options parameteres.

options could contain any option supported by the underlying driver.

This PR is related to issue #29000

@rails-bot

This comment has been minimized.

rails-bot commented Jun 1, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@eileencodes

Looking good so far! 👍 Left some comments for changes I'd like to see. Thanks for working on this.

actionpack/lib/action_dispatch/system_testing/driver.rb Outdated
when :poltergeist then register_poltergeist
when :webkit then register_webkit
end

This comment has been minimized.

@eileencodes

eileencodes Jun 1, 2017

Member

Can you make a new private register method and move this case statement inside. Then move the duplicate code from the namespaced register methods into the register method?

def use
  register unless rack_test?
  setup
end

private
  def register
   Capybara.register_driver @name do |app|
    case @name
    etc
  end
end

def register_selenium
...
end

etc
actionpack/lib/action_dispatch/system_testing/driver.rb Outdated
Capybara.register_driver @name do |app|
Capybara::Selenium::Driver.new(app, { browser: @browser }.merge(@options)).tap do |driver|
driver.browser.manage.window.size = Selenium::WebDriver::Dimension.new(*@screen_size)
end
end
end
def register_poltergeist
if defined?(Capybara::Poltergeist)

This comment has been minimized.

@eileencodes

eileencodes Jun 1, 2017

Member

What is the if defined?(Capybara::Poltergeist) buying us? This won't load without register getting called and if you don't add the gemfile it will blow up anyway. Otherwise this will silently fail I think.

actionpack/lib/action_dispatch/system_testing/driver.rb Outdated
Capybara::Webkit::Driver.new(app, Capybara::Webkit::Configuration.to_hash.merge(@options)).tap do |driver|
driver.resize_window(*@screen_size)
end

This comment has been minimized.

@eileencodes

eileencodes Jun 1, 2017

Member

✂️ this extra line.

@eileencodes eileencodes assigned eileencodes and unassigned pixeltrix Jun 1, 2017

@eileencodes eileencodes added this to the 5.1.2 milestone Jun 1, 2017

@mariochavez mariochavez force-pushed the mariochavez:driven_by_support_headless_drivers branch Jun 1, 2017

@eileencodes

This comment has been minimized.

Member

eileencodes commented Jun 2, 2017

Looks great, thanks!

Can you add an entry to the top of the actionpack CHANGELOG? And then squash your commits into one? I'll merge once those are done. Thanks for working on this!

SystemTesting::Driver can register capybara-webkit and poltergeist
drivers.

When using `driver_by` with capybara-webkit or poltergeist,
SystemTesting::Driver will register the driver while passing
`screen_size` and `options` parameteres.

`options` could contain any option supported by the underlying driver.

@mariochavez mariochavez force-pushed the mariochavez:driven_by_support_headless_drivers branch to 9063007 Jun 2, 2017

@mariochavez

This comment has been minimized.

Contributor

mariochavez commented Jun 2, 2017

@eileencodes done ! Thanks for your help to get this thru.

@eileencodes eileencodes merged commit e935a6c into rails:master Jun 2, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

eileencodes added a commit that referenced this pull request Jun 2, 2017

Merge pull request #29315 from mariochavez/driven_by_support_headless…
…_drivers

SystemTesting::Driver can register capybara-webkit and poltergeist
@eileencodes

This comment has been minimized.

Member

eileencodes commented Jun 2, 2017

Backported in c5dd451 because while a feature enhancement it's a very new feature and since this was missing I consider it a 🐛

@sebastianiorga

This comment has been minimized.

sebastianiorga commented Jul 5, 2017

Apologies if this is the wrong place to raise this issue, but it's directly caused by this pull request.

Ruby version 2.4.1
Rails version 5.1.2
Capybara version 2.14.4

The changes made in this commit to actionpack/lib/action_dispatch/system_testing/driver.rb break the ability to register Capybara drivers not named [:selenium, :poltergeist, :webkit].

Because ActionDispatch::SystemTesting::Driver#driver now gets called UNLESS name is :rack_test, it ends up overwriting custom Capybara drivers setup in RAILS_ROOT/test/application_system_test_case.rb with nil(since the case statement in register can only handle 3 keys.

For instance, the code below sets up headless_chrome as a system test driver:

# test/application_system_test_case.rb
require 'test_helper'
require 'capybara'
# require 'capybara/poltergeist'

ENV['RAILS_SYSTEM_TESTING_SCREENSHOT'] = 'simple'

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  # driven_by :selenium, using: :chrome, screen_size: [1400, 1400]
  Capybara.register_driver(:headless_chrome) do |app|
    capabilities = Selenium::WebDriver::Remote::Capabilities.chrome(
      chromeOptions: { args: %w[headless disable-gpu window-size=1920x1080] }
    )

    Capybara::Selenium::Driver.new(
      app,
      browser: :chrome,
      desired_capabilities: capabilities
    )
  end
  driven_by :headless_chrome
  # Rack::Test
  # driven_by :rack_test

  # capybara-webkit
  # Capybara::Webkit.configure do |config|
  #   config.raise_javascript_errors = false
  # end
  # driven_by :webkit

  # Poltergeist
  # Capybara.register_driver :poltergeist do |app|
  #   Capybara::Poltergeist::Driver.new(app, js_errors: true)
  # end
  # driven_by :poltergeist
end

This causes a nil error in Capybara::Session#driver since driver ends up being set to nil for the :headless_chrome mode.

Super hacky fix is to monkey patch register:

# test/application_system_test_case.rb
require 'test_helper'
require 'capybara'
# require 'capybara/poltergeist'

ENV['RAILS_SYSTEM_TESTING_SCREENSHOT'] = 'simple'
ActionDispatch::SystemTestCase.driver
class ActionDispatch::SystemTesting::Driver # :nodoc:
  def register
    return unless [:selenium, :poltergeist, :webkit].include?(@name)

    Capybara.register_driver @name do |app|
      case @name
      when :selenium then register_selenium(app)
      when :poltergeist then register_poltergeist(app)
      when :webkit then register_webkit(app)
      end
    end
  end
end

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
...

This is obviously not the way to go about things. I'm not sure modifying ActionDispatch::SystemTesting::Driver#register with that guard clause is the right solution either. After all: what makes those 3 options special? And if they are special there should be some single source of truth where they are declared with the case statement and other areas in the code referencing them.

Possibly a solution involves some sort of polymorphism, with a fallback that does nothing when the user manually registered the capybara driver they want to use?

Again, apologies if this is the wrong place for this comment.

@eileencodes

This comment has been minimized.

Member

eileencodes commented Jul 5, 2017

@sebastianiorga can you open a new issue with these details and reference this PR?

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Mar 18, 2018

Fix "Ruby on Rails 5.2 Release Notes" [ci skip]
- Remove
  ```
  *   Namespace error pages' CSS selectors to stop the styles from bleeding
      into other pages when using Turbolinks.
      ([Pull Request](rails#28814))
  ```
  since it was backported to
  `5-1-stable` by 50d5baf
  and
  `5-0-stable` by d1c4a39.

- Remove
  ```
  *   Allow irb options to be passed from `rails console` command.
      ([Pull Request](rails#29010))
  ```
  since it was backported to
  `5-1-stable` by e91b483.

- Remove
  ```
  *   Load environment file in `dbconsole` command.
      ([Pull Request](rails#29725))
  ```
  since it was backported to
  `5-1-stable` by 7f93428.

- Remove
  ```
  *   Gemfile for new apps: upgrade redis-rb from ~> 3.0 to 4.0.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Fix minitest rails plugin.
      The custom reporters are added only if needed.
      This will fix conflicts with others plugins.
      ([Commit](rails@ac99916))
  ```
  since it was backported to
  `5-1-stable` by caa7695.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.

- Remove
  ```
  *   Add `action_controller_api` and `action_controller_base` load hooks to be
      called in `ActiveSupport.on_load`.
      ([Pull Request](rails#28402))
  ```
  since it was backported to
  `5-1-stable` by b9a5fd7.

- Remove
  ```
  *   `driven_by` now registers poltergeist and capybara-webkit.
      ([Pull Request](rails#29315))
  ```
  since it was backported to
  `5-1-stable` by c5dd451.

- Remove
  ```
  *   Fallback `ActionController::Parameters#to_s` to `Hash#to_s`.
      ([Pull Request](rails#29630))
  ```
  since it was backported to
  `5-1-stable` by c1014e4
  and
  `5-0-stable` by 0e71fc3.

- Remove
  ```
  *   Make `take_failed_screenshot` work within engine.
      ([Pull Request](rails#30421))
  ```
  since it was backported to
  `5-1-stable` by 595a231.

- Remove
  ```
  *   Fix optimized url helpers when using relative url root.
      ([Pull Request](rails#31261))
  ```
  since it was backported to
  `5-1-stable` by e9b7795.

- Remove
  ```
  *   Update `distance_of_time_in_words` helper to display better error messages
      for bad input.
      ([Pull Request](rails#20701))
  ```
  since it was backported to
  `5-1-stable` by 2c97fbf.

- Remove
  ```
  *   Generate field ids in `collection_check_boxes` and
      `collection_radio_buttons`.
      ([Pull Request](rails#29412))
  ```
  since it was backported to
  `5-1-stable` by 2d8c10a.

- Remove
  ```
  *   Fix issues with scopes and engine on `current_page?` method.
      ([Pull Request](rails#29503))
  ```
  since it was backported to
  `5-1-stable` by 2135daf.

- Remove
  ```
  *   Bring back proc with arity of 1 in `ActionMailer::Base.default` proc
      since it was supported in Rails 5.0 but not deprecated.
      ([Pull Request](rails#30391))
  ```
  since it was backported to
  `5-1-stable` by b2bedb1.

- Remove
  ```
  *   Add type caster to `RuntimeReflection#alias_name`.
      ([Pull Request](rails#28961))
  ```
  since it was backported to
  `5-1-stable` by f644e7a.

- Remove
  ```
  *   Loading model schema from database is now thread-safe.
      ([Pull Request](rails#29216))
  ```
  since it was backported to
  `5-1-stable` by 02926cf.
  and
  `5-0-stable` by 84bcfe5

- Remove
  ```
  *   Fix destroying existing object does not work well when optimistic locking
      enabled and `locking_column` is null in the database.
      ([Pull Request](rails#28926))
  ```
  since it was backported to
  `5-1-stable` by e498052.

- Remove
  ```
  *   `ActiveRecord::Persistence#touch` does not work well
      when optimistic locking enabled and `locking_column`,
      without default value, is null in the database.
      ([Pull Request](rails#28914))
  ```
  since it was backported to
  `5-1-stable` by 1e2f63d.

- Remove
  ```
  *   Previously, when building records using a `has_many :through` association,
      if the child records were deleted before the parent was saved,
      they would still be persisted. Now, if child records are deleted
      before the parent is saved on a `has_many :through` association,
      the child records will not be persisted.
      ([Pull Request](rails#29593))
  ```
  since it was backported to
  `5-1-stable` by a22c39e.

- Remove
  ```
  *   Query cache was unavailable when entering the `ActiveRecord::Base.cache`
      block without being connected.
      ([Pull Request](rails#29609))
  ```
  since it was backported to
  `5-1-stable` by fd6c8cd
  and
  `5-0-stable` by 9f2532b.

- Remove
  ```
  *   `Relation#joins` is no longer affected by the target model's
      `current_scope`, with the exception of `unscoped`.
      ([Commit](rails@5c71000))
  ```
  since it was backported to
  `5-1-stable` by 3630d63.

- Remove
  ```
  *   Fix `unscoped(where: [columns])` removing the wrong bind values.
      ([Pull Request](rails#29780))
  ```
  since it was backported to
  `5-1-stable` by d378fcb.

- Remove
  ```
  *   When a `has_one` association is destroyed by `dependent: destroy`,
      `destroyed_by_association` will now be set to the reflection, matching the
      behaviour of `has_many` associations.
      ([Pull Request](rails#29855))
  ```
  since it was backported to
  `5-1-stable` by 8254a8b.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT`
      to keep the existing select list.
      ([Pull Request](rails#29848))
  ```
  since it was backported to
  `5-1-stable` by 0e8d4ed.

- Remove
  ```
  *   Ensure `sum` honors `distinct` on `has_many :through` associations.
      ([Commit](rails@566f1fd))
  ```
  since it was backported to
  `5-1-stable` by c0a1dc2.

- Remove
  ```
  *   Fix `COUNT(DISTINCT ...)` for `GROUP BY` with `ORDER BY` and `LIMIT`.
      ([Commit](rails@5668dc6))
  ```
  since it was backported to
  `5-1-stable` by 87ca68e.

- Remove
  ```
  *   MySQL: Don't lose `auto_increment: true` in the `db/schema.rb`.
      ([Commit](rails@9493d45))
  ```
  since it was backported to
  `5-1-stable` by 8b6e694.

- Remove
  ```
  *   Fix longer sequence name detection for serial columns.
      ([Pull Request](rails#28339))
  ```
  since it was backported to
  `5-1-stable` by af9c170
  and
  `5-0-stable` by 7025b1d.

- Remove
  ```
  *   Fix `bin/rails db:setup` and `bin/rails db:test:prepare` create wrong
      ar_internal_metadata's data for a test database.
      ([Pull Request](rails#30579))
  ```
  since it was backported to
  `5-1-stable` by bb67b5f
  and
  `5-0-stable` by 60437e6.

- Remove
  ```
  *   Fix conflicts `counter_cache` with `touch: true` by optimistic locking.
      ([Pull Request](rails#31405))
  ```
  since it was backported to
  `5-1-stable` by 5236dda.

- Remove
  ```
  *   Fix `count(:all)` to correctly work `distinct` with custom SELECT list.
      ([Commit](rails@c6cd9a5))
  ```
  since it was backported to
  `5-1-stable` by 6beb4de.

- Remove
  ```
  *   Fix to invoke callbacks when using `update_attribute`.
      ([Commit](rails@732aa34))
  ```
  since it was backported to
  `5-1-stable` by 6346683.

- Remove
  ```
  *   Use `count(:all)` in `HasManyAssociation#count_records` to prevent invalid
      SQL queries for association counting.
      ([Pull Request](rails#27561))
  ```
  since it was backported to
  `5-1-stable` by eef3c89.

- Remove
  ```
  *   Fix `count(:all)` with eager loading and having an order other than
      the driving table.
      ([Commit](rails@ebc09ed))
  ```
  since it was backported to
  `5-1-stable` by 6df9b69.

- Remove
  ```
  *   PostgreSQL: Allow pg-1.0 gem to be used with Active Record.
      ([Pull Request](rails#31671))
  ```
  since it was backported to
  `5-1-stable` by a9c06f6.

- Remove
  ```
  *   Fix that after commit callbacks on update does not triggered
      when optimistic locking is enabled.
      ([Commit](rails@7f9bd03))
  ```
  since it was backported to
  `5-1-stable` by aaee10e.

- Remove
  ```
  *   Fix regression in numericality validator when comparing Decimal and Float
      input values with more scale than the schema.
      ([Pull Request](rails#28584))
  ```
  since it was backported to
  `5-1-stable` by 5b1c3e5.
  Note that there was incorrect link to PR,
  original PR is rails#29249.

- Remove
  ```
  *   Fix to working before/after validation callbacks on multiple contexts.
      ([Pull Request](rails#31483))
  ```
  since it was backported to
  `5-1-stable` by 0f7046a.

- Remove
  ```
  *   Fix implicit coercion calculations with scalars and durations.
      ([Pull Request](rails#29163),
      [Pull Request](rails#29971))
  ```
  since it was backported to
  `5-1-stable` by 51ea27c,
                  4d82e2a.

- Remove
  ```
  *   Fix modulo operations involving durations.
      ([Commit](rails@a54e13b))
  ```
  since it was backported to
  `5-1-stable` by 233fa7e.

- Remove
  ```
  *   Return all mappings for a timezone identifier in `country_zones`.
      ([Commit](rails@cdce6a7))
  ```
  since it was backported to
  `5-1-stable` by 0222ebb.

- Remove
  ```
  *   Add support for compatibility with redis-rb gem for 4.0 version.
      ([Pull Request](rails#30748))
  ```
  since it was backported to
  `5-1-stable` by 3789531.
  Related to rails#32252.

Related to rails#32222, rails#32222 (comment).
Follow up a489cc8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment