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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Capybara Integration with Rails (AKA System Tests) #26703

Merged
merged 37 commits into from Feb 20, 2017

Conversation

@eileencodes
Member

eileencodes commented Oct 4, 2016

Update: This PR changed a lot over the course of being open. Please refer to the documentation or code in master / 5-1-stable to use system tests

I'm really excited to open this initial PR for bringing system/acceptance test support to Rails through Capybara.

The goal of this PR is for Rails to take on all the setup that is needed in applications to allow Capybara and make system testing default in Rails applications.

Why is the name Rails::SystemTestCase?
I chose this name because this was the name DHH indicated he preferred in the Rails' Basecamp long ago. I'm totally open to changing the name but ultimately decided to put it in ActionPack with the Rails namespace so I could get onto writing code and stop worrying about the name 馃槈

The test framework has been moved it it's own gem under the Rails name and is now ActionSystemTest
Because the gem expects a Module and our test frameworks expect a Class for inheritance I've made the gem and module name ActionSystemTest and the class for inheriting test code from ActionSystemTestCase. Open to other names as well but the Module and Class names can't be the same else Ruby gets confused.

How do I add System tests to my application?
By default new applications include a base system test much like integration tests. Gemfiles generated for new applications will include Capybara and the Selenium driver.

Capybara's selenium driver requires some setup by the programmer, so I've added a layer between Capybara and Rails so that Rails' takes on that work called RailsSeleniumDriver. The default settings are as follows:

  • The server is puma (because this is Rails' default)
  • the browser is chrome (because FF is broken w/ Capybara and Selenium right now),
  • the default port is 28100, and
  • the default screen size is [1400, 1400].

The reason I have chosen Chrome as the default browser is because Firefox doesn't work out of the box. Selenium and the current version of Firefox don't play nicely together so I've set up the driver to default to Chrome, but it can easily be switched to using Firefox. I'd also like to provide support for Safari.

Instead of requiring the programmer to set up the port, browser, server, etc Rails handles that setup and frees up the programmer to work on writing their tests.

The Rails drivers allow the default configuration to be changed by initializing a new driver:

# config/environments/test.rb
config.system_testing.driver = SystemTesting::DriverAdapters::RailsSeleniumDriver.new(
  browser: :firefox
)

What if I don't want to use Selenium?
If the programmer wants to use one of Capybara's default drivers rather than the provided defaults in Rails Selenium configuration they can easily change that in the test environment. I've provided a shim in Rails so setting the Capybara driver is simple. I've named the class CapybaraDriver and it gives access to the 4 Capybara drivers: poltergeist, webrick, selenium (with no setup), and rack_test:

# config/environments/test.rb
config.system_testing.driver = :poltergeist

Each of the Capybara default drivers that requires a server defaults to Puma. Any of the settings can be changed by initializing a new CapbyaraDriver object:

# config/environments/test.rb
config.system_testing.driver = SystemTesting::DriverAdapters::CapybaraDriver.new(
  name: :poltergeist,
  server: :webrick
)

What if I'm making a new application and don't want system testing at all?
It's easy to skip system testing files in the app generator or the scaffold generator by running with the --skip-system-test flag.

What if I already use Capybara and don't want to use Rails?
Because Rails provides a specific test case name to inherit from you can easily just completely skip the Rails version of system testing and use Capybara directly.

I want to try it out!
I made a test app where you can try out system tests in Rails w/ Selenium! https://github.com/eileencodes/system_testing_app


Cool! What's next?

Below is a list of what is done and what's left to do. Once we sort out naming and other issues I'll finish up the items below.

What's done?

  • Generators: Scaffold, individual, and application
  • Basic Documentation
  • Testing adapter settings (this just tests that Rails provides options included, not that Capybara and friends work. The assumption is that actual behavior of Capybara and friends is tested by themselves, Rails tests it's framework for initializing Capybara)
  • Configurable driver adapters w/ RailsSeleniumDriver being the default.
  • Railtie for configuring system test settings
  • Support for screenshots
  • Changelog
  • Fix the railties tests 馃榿
  • Move to it's own gem under the rails name.
  • Rails Guides
  • Other custom but generic helpers we support at Basecamp that we'd like to port over, especially ActiveJob and ActionCable support
  • Testing the custom helpers and assertions
  • Because Capybara and the test suite use 2 different threads, transactions aren't correctly rolled back at the end of the test, leaving data behind. For years DatabaseCleaner has been used to resolve this problem, but it's fixable in Rails. See PR #28083

Future work

  • Support for Safari (last I checked this wasn't quite ready for prime time)

cc/ @dhh @georgeclaghorn

@eileencodes eileencodes added this to the 5.1.0 milestone Oct 4, 2016

@eileencodes eileencodes self-assigned this Oct 4, 2016

@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Oct 4, 2016

Member

Transactions aren't handled in Capybara the same as they are in Rails' other test so we need to handle that. Capybara suggests using the DatabaseCleaner gem but we only need a small portion of that code.

@eileencodes We should also look for https://github.com/amatsuda/database_rewinder as an alternative to DatabaseCleaner.

Member

prathamesh-sonpatki commented Oct 4, 2016

Transactions aren't handled in Capybara the same as they are in Rails' other test so we need to handle that. Capybara suggests using the DatabaseCleaner gem but we only need a small portion of that code.

@eileencodes We should also look for https://github.com/amatsuda/database_rewinder as an alternative to DatabaseCleaner.

@nynhex

This comment has been minimized.

Show comment
Hide comment
@nynhex

nynhex Oct 4, 2016

This is excellent!

nynhex commented Oct 4, 2016

This is excellent!

@alexcameron89

Hey @eileencodes, I ran through the documentation and commented on some grammar issues.

This PR looks great, and I'm excited for it to be a part of the default Test Suite!

Show outdated Hide outdated actionpack/lib/system_test_case.rb
module Rails
# System tests are similar to Integration tests in that they incorporate multiple
# controllers and actions, but can be used to similate a real user experience.

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

similate -> simulate

@alexcameron89

alexcameron89 Oct 4, 2016

Member

similate -> simulate

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters.rb
#
# By default Rails supports Capybara with the Selenium Driver. Rails provides
# configuration setup for using the selenium driver with Capybara.
# Additionally Rails can be used as a layer between Capybara and it's other

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

it's -> its

@alexcameron89

alexcameron89 Oct 4, 2016

Member

it's -> its

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
# The drivers Capybara supports are: +:rack_test+, +:selenium+, +:webkit+,
# and +:poltergeist+.
#
# Rails provides it's own defaults for Capybara with the Selenium driver

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

it's -> its

@alexcameron89

alexcameron89 Oct 4, 2016

Member

it's -> its

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
# directly.
#
# To set your system tests to use one of Capybara's default drivers add
# the following to yur Rails' configuration test environment:

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

"to use one of Capybara's default drivers add" -> "to use one of Capybara's default drivers, add"

line 18: yur -> your

@alexcameron89

alexcameron89 Oct 4, 2016

Member

"to use one of Capybara's default drivers add" -> "to use one of Capybara's default drivers, add"

line 18: yur -> your

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
# JavaScript testing and doesn't require a server.
#
# The +:poltergeist+ and +:webkit+ drivers are headless, but require some
# extra environment setup. Because the defalt server for Rails is Puma, each

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

defalt -> default

@alexcameron89

alexcameron89 Oct 4, 2016

Member

defalt -> default

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
# of the Capybara drivers will default to using Puma. Changing the configuration
# to use Webrick is possible by initalizing a new driver object.
#
# The default settings for the <tt>CapybaraDrvier</tt> are:

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

CapybaraDrvier -> CapybaraDriver

@alexcameron89

alexcameron89 Oct 4, 2016

Member

CapybaraDrvier -> CapybaraDriver

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/rails_selenium_driver.rb
# selenium-webdriver gem is required by this driver.
#
# The <tt>RailsSeleniumDriver</tt> is useful for real browser testing and
# support Chrome and Firefox.

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

support -> supports

@alexcameron89

alexcameron89 Oct 4, 2016

Member

support -> supports

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/rails_selenium_driver.rb
# @screen_size=[ 1400, 1400 ]
# >
#
# The settings for the <tt>RailsSeleniumDriver</tt> can be changed in the

This comment has been minimized.

@alexcameron89

alexcameron89 Oct 4, 2016

Member

"in the Rails'" -> "in the Rails" OR "in Rail's"

@alexcameron89

alexcameron89 Oct 4, 2016

Member

"in the Rails'" -> "in the Rails" OR "in Rail's"

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Oct 4, 2016

Member

I assume that this follow's Capybara's opinionated policy of making it hard to access request/response objects and that we should use integration tests for those?

Member

pixeltrix commented Oct 4, 2016

I assume that this follow's Capybara's opinionated policy of making it hard to access request/response objects and that we should use integration tests for those?

Show outdated Hide outdated actionpack/lib/system_test_case.rb
require "system_testing/test_helper"
require "system_testing/driver_adapter"
module Rails

This comment has been minimized.

@maclover7

maclover7 Oct 4, 2016

Member

If we're shipping this under the actionpack gem, should we use the ActionPack base module? 馃槵

@maclover7

maclover7 Oct 4, 2016

Member

If we're shipping this under the actionpack gem, should we use the ActionPack base module? 馃槵

This comment has been minimized.

@eileencodes

eileencodes Oct 4, 2016

Member

I explained why I chose this name in the message above and I don't want to move it around 100 times before we have a consensus on name.

@eileencodes

eileencodes Oct 4, 2016

Member

I explained why I chose this name in the message above and I don't want to move it around 100 times before we have a consensus on name.

This comment has been minimized.

@dhh

dhh Oct 4, 2016

Member

I actually don't see any dependencies for ActionPack in this patch, so I think we could consider making it a complete stand-alone gem.

@dhh

dhh Oct 4, 2016

Member

I actually don't see any dependencies for ActionPack in this patch, so I think we could consider making it a complete stand-alone gem.

This comment has been minimized.

@eileencodes

eileencodes Oct 4, 2016

Member

@dhh Rails::SystemTestCase inherits from ActionDispatch::IntegrationTest so we don't have to rewrite all the routing and url helpers handling.

@eileencodes

eileencodes Oct 4, 2016

Member

@dhh Rails::SystemTestCase inherits from ActionDispatch::IntegrationTest so we don't have to rewrite all the routing and url helpers handling.

This comment has been minimized.

@dhh

dhh Oct 4, 2016

Member

Ah, I missed that. Then going with ActionDispatch::SystemTest probably makes sense. But agree no sense in making any changes until we really know if we love that name!

@dhh

dhh Oct 4, 2016

Member

Ah, I missed that. Then going with ActionDispatch::SystemTest probably makes sense. But agree no sense in making any changes until we really know if we love that name!

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

I like ActionDispatch::SystemTest

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

I like ActionDispatch::SystemTest

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapter.rb
# Rails' configuration file.
def driver=(driver)
@driver = DriverAdapters.lookup(driver)
@driver.call

This comment has been minimized.

@maclover7

maclover7 Oct 4, 2016

Member

Should we be more explicit about what call does, in regard to setting up / registering the adapter? 馃槵

@maclover7

maclover7 Oct 4, 2016

Member

Should we be more explicit about what call does, in regard to setting up / registering the adapter? 馃槵

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Calling your personal @driver gets you a smooth getaway 馃槑

@kaspth

kaspth Oct 6, 2016

Member

Calling your personal @driver gets you a smooth getaway 馃槑

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters.rb
# | Selenium | Firefox | Yes |
# | Webkit | Headless w/ QtWebKit | Yes |
# | Poltergeist | Headless w/ PhantomJS | Yes |
module DriverAdapters

This comment has been minimized.

@maclover7

maclover7 Oct 4, 2016

Member

Does it make sense to provide a "base" class to inherit from, so people know which methods they need to implement when creating adapters? 馃槵

@maclover7

maclover7 Oct 4, 2016

Member

Does it make sense to provide a "base" class to inherit from, so people know which methods they need to implement when creating adapters? 馃槵

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
end
def supports_screenshots?
if @name == :rack_test

This comment has been minimized.

@fnando

fnando Oct 4, 2016

Contributor

Don't use a conditional to return a boolean.

def supports_screenshots?
  @name == :rack_test
end
@fnando

fnando Oct 4, 2016

Contributor

Don't use a conditional to return a boolean.

def supports_screenshots?
  @name == :rack_test
end

This comment has been minimized.

@jonathanhefner

jonathanhefner Oct 5, 2016

I agree, but it would be @name != :rack_test. (However, it may have been written as is with the intention that more cases / logic would be added.)

@jonathanhefner

jonathanhefner Oct 5, 2016

I agree, but it would be @name != :rack_test. (However, it may have been written as is with the intention that more cases / logic would be added.)

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Oct 4, 2016

Member

Is there a specific reason for defaulting to selenium instead of a headless driver such as poltergeist or capybara-webkit?

Member

sgrif commented Oct 4, 2016

Is there a specific reason for defaulting to selenium instead of a headless driver such as poltergeist or capybara-webkit?

@dixpac

This comment has been minimized.

Show comment
Hide comment
@dixpac

dixpac Oct 5, 2016

Contributor

I agree with @sgrif, IMHO in real world apps people will switch to headless driver right away, so maybe capybara-webkit/poltergeist is better default here 馃槃

Contributor

dixpac commented Oct 5, 2016

I agree with @sgrif, IMHO in real world apps people will switch to headless driver right away, so maybe capybara-webkit/poltergeist is better default here 馃槃

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Oct 5, 2016

Member

@sgrif and @dixpac I chose the Selenium driver as default because the purpose of adding system testing to Rails is for the initial setup to be absolutely zero. Capybara does that through their default, Rack Test, but it's not really a useful demonstration of the merits of system testing since it doesn't support JavaScript testing.

The selenium driver is one that Rails can take on all of the setup without any extra requirements from the programmer. capybara-webkit requires environmental setup that Rails can't take on - installing the Qt libraries. poltergeist requires PhantomJS which does have a gem but I don't know much about whether it's stable. We use Selenium at Basecamp and for now I'm going to leave the default as-is.

If there's a compelling reason to change the default it's as simple as swapping out the pre-set default and ensuring that Rails takes on the setup for that.


@pixeltrix Since this PR doesn't change how Capybara works, but rather provides a layer between Capybara and Rails.Currently, no, the request object isn't defined, but Rails::SystemTestCase does inherit from ActionDispatch::IntegrationTest so perhaps that's something we can easily expose in the future 馃槃

Member

eileencodes commented Oct 5, 2016

@sgrif and @dixpac I chose the Selenium driver as default because the purpose of adding system testing to Rails is for the initial setup to be absolutely zero. Capybara does that through their default, Rack Test, but it's not really a useful demonstration of the merits of system testing since it doesn't support JavaScript testing.

The selenium driver is one that Rails can take on all of the setup without any extra requirements from the programmer. capybara-webkit requires environmental setup that Rails can't take on - installing the Qt libraries. poltergeist requires PhantomJS which does have a gem but I don't know much about whether it's stable. We use Selenium at Basecamp and for now I'm going to leave the default as-is.

If there's a compelling reason to change the default it's as simple as swapping out the pre-set default and ensuring that Rails takes on the setup for that.


@pixeltrix Since this PR doesn't change how Capybara works, but rather provides a layer between Capybara and Rails.Currently, no, the request object isn't defined, but Rails::SystemTestCase does inherit from ActionDispatch::IntegrationTest so perhaps that's something we can easily expose in the future 馃槃

@kaspth

Looks sweet! Especially looking forward to the DatabaseCleaner work around 馃榿

Show outdated Hide outdated actionpack/lib/system_test_case.rb
# fill_in 'Name', with: 'Arya'
# click_on 'Create User'
#
# assert_text 'Arya'

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

woof! 馃悤

@kaspth

kaspth Oct 6, 2016

Member

woof! 馃悤

Show outdated Hide outdated actionpack/lib/system_test_case.rb
# click_on 'New User'
#
# fill_in 'Name', with: 'Arya'
# click_on 'Create User'

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Should we do anything to make the Capybara helpers work better with Rails' integration helpers?

I haven't used Capybara much, but from what I can remember you can't call visit and then use assert_response :success for instance.

Pointing this out because the system test inherits from ActionDispatch::IntegrationTest so users might think our built in assertions would work.

@kaspth

kaspth Oct 6, 2016

Member

Should we do anything to make the Capybara helpers work better with Rails' integration helpers?

I haven't used Capybara much, but from what I can remember you can't call visit and then use assert_response :success for instance.

Pointing this out because the system test inherits from ActionDispatch::IntegrationTest so users might think our built in assertions would work.

This comment has been minimized.

@eileencodes

eileencodes Oct 14, 2016

Member

I think that's a great idea for the future, but right now there's still a ton left to do on this PR and I'm not exactly sure how to expose it just yet. I agree it would be great to add that in once we have this PR in master.

@eileencodes

eileencodes Oct 14, 2016

Member

I think that's a great idea for the future, but right now there's still a ton left to do on this PR and I'm not exactly sure how to expose it just yet. I agree it would be great to add that in once we have this PR in master.

This comment has been minimized.

@kaspth

kaspth Oct 28, 2016

Member

Great, let's revisit some other time 馃憤

@kaspth

kaspth Oct 28, 2016

Member

Great, let's revisit some other time 馃憤

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapter.rb
# Rails' configuration file.
def driver=(driver)
@driver = DriverAdapters.lookup(driver)
@driver.call

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Calling your personal @driver gets you a smooth getaway 馃槑

@kaspth

kaspth Oct 6, 2016

Member

Calling your personal @driver gets you a smooth getaway 馃槑

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters.rb
# | Rails' Selenium | Chrome | Yes |
# | Rack Test | No JS Support | No |
# | Selenium | Firefox | Yes |
# | Webkit | Headless w/ QtWebKit | Yes |

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

I believe it's spelled WebKit.

@kaspth

kaspth Oct 6, 2016

Member

I believe it's spelled WebKit.

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
#
# config.system_testing.driver = SystemTesting::DriverAdapters::CapybaraDriver.new(
# name: :webkit,
# puma: :webrick

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

server: :webrick

@kaspth

kaspth Oct 6, 2016

Member

server: :webrick

Show outdated Hide outdated actionpack/lib/system_testing/test_helpers/assertions.rb
private
def type_for_selector(*items)
if items.first.is_a?(Symbol)
items.shift

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Why do we want to shift this arg? What kind of arguments can we see in this method?

@kaspth

kaspth Oct 6, 2016

Member

Why do we want to shift this arg? What kind of arguments can we see in this method?

Show outdated Hide outdated actionpack/lib/system_testing/test_helpers/assertions.rb
#
# assert_none_of_selectors('ul', 'ol')
def assert_none_of_selectors(*items)
options = items.extract_options!

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

I think we can use a double splat here:

def assert_none_of_selectors(*items, **options)
  # ...
end
@kaspth

kaspth Oct 6, 2016

Member

I think we can use a double splat here:

def assert_none_of_selectors(*items, **options)
  # ...
end
Show outdated Hide outdated actionpack/lib/system_testing/test_helpers/assertions.rb
# Asserts that all of the provided selectors are present on the given page.
#
# assert_all_of_selectors('p', 'td')
def assert_all_of_selectors(*items)

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Ditto re double splat.

@kaspth

kaspth Oct 6, 2016

Member

Ditto re double splat.

Show outdated Hide outdated actionpack/lib/system_testing/test_helpers/screenshot_helper.rb
def image_path
path = "tmp/screenshots/failures_#{method_name}.png"
page.save_screenshot(Rails.root.join(path))
path

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Find it a little odd that image_path both returns a path and saves an image. I mean, wouldn't the two calls to image_path in find_image save two more images?

@kaspth

kaspth Oct 6, 2016

Member

Find it a little odd that image_path both returns a path and saves an image. I mean, wouldn't the two calls to image_path in find_image save two more images?

Show outdated Hide outdated actionpack/test/system_testing/screenshot_helper_test.rb
assert_equal true, Rails::SystemTestCase.driver.supports_screenshots?
Rails::SystemTestCase.driver = :poltergeist
assert_equal true, Rails::SystemTestCase.driver.supports_screenshots?

This comment has been minimized.

@kaspth

kaspth Oct 6, 2016

Member

Why do we want this to return booleans over our generally preferred falsey values?

@kaspth

kaspth Oct 6, 2016

Member

Why do we want this to return booleans over our generally preferred falsey values?

This comment has been minimized.

@eileencodes

eileencodes Oct 15, 2016

Member

This was an oversight, I've fixed this now.

@eileencodes

eileencodes Oct 15, 2016

Member

This was an oversight, I've fixed this now.

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
# name: :webkit,
# server: :webrick
# )
class CapybaraDriver

This comment has been minimized.

@maclover7

maclover7 Oct 15, 2016

Member

[sorry, just reposting this comment since it got accidentally marked as "outdated" thanks to the great work being done here :)]

Does it make sense to provide a "base" class to inherit from, so people know which methods they need to implement when creating adapters? 馃槵

@maclover7

maclover7 Oct 15, 2016

Member

[sorry, just reposting this comment since it got accidentally marked as "outdated" thanks to the great work being done here :)]

Does it make sense to provide a "base" class to inherit from, so people know which methods they need to implement when creating adapters? 馃槵

This comment has been minimized.

@eileencodes

eileencodes Oct 15, 2016

Member

Hey @maclover7 thanks. I know there are a lot of comments to address and getting around to them as I can. In the future, no need to re-comment.

I don't think adding a base class makes sense becasue the Rails provided adapter takes different arguments than the capybara provided adapters. It might make sense as this project evolves but I'll tackle that when that happens.

@eileencodes

eileencodes Oct 15, 2016

Member

Hey @maclover7 thanks. I know there are a lot of comments to address and getting around to them as I can. In the future, no need to re-comment.

I don't think adding a base class makes sense becasue the Rails provided adapter takes different arguments than the capybara provided adapters. It might make sense as this project evolves but I'll tackle that when that happens.

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapter.rb
@@ -0,0 +1,31 @@
require "system_testing/driver_adapters"
module SystemTesting

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Should not this class be in one namespacing?

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Should not this class be in one namespacing?

@rafaelfranca

It feels to me that this should be a new gem. It have its own namespace, its own railtie it can be skipped and I don't think we should add capybara as dependency of actionpack. I'd create a new gem, inside the rails repository and add it as dependency of Rails.

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/capybara_driver.rb
@port = port
end
def call

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Maybe call it start?

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Maybe call it start?

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/rails_selenium_driver.rb
# config.system_testing.driver = SystemTesting::DriverAdapters::RailsSeleniumDriver.new(
# server: :webrick,
# port: 28100,
# screen_size: [ 800, 800 ]

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Our style guide doesn't put spaces inside array literals

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Our style guide doesn't put spaces inside array literals

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Actually it is the other way around. I'll make rubocop point that.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

Actually it is the other way around. I'll make rubocop point that.

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

There is no support for that in rubocop 馃槩 .

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

There is no support for that in rubocop 馃槩 .

Show outdated Hide outdated actionpack/lib/system_testing/driver_adapters/rails_selenium_driver.rb
attr_reader :browser, :server, :port, :screen_size
def initialize(browser: :chrome, server: :puma, port: 28100, screen_size: [ 1400,1400 ]) # :nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

missing space after the , and remove the space inside the array literal

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

missing space after the , and remove the space inside the array literal

Show outdated Hide outdated actionpack/lib/system_testing/railtie.rb
options.driver ||= Rails::SystemTestCase.default_driver
ActiveSupport.on_load(:system_testing) do
options.each { |k,v| send("#{k}=", v) }

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

space after the ,

@rafaelfranca

rafaelfranca Oct 29, 2016

Member

space after the ,

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Oct 29, 2016

Member

@rafaelfranca David and I talked about the naming etc this week and decided on creating a new gem inside Rails, so I'll be doing that soon. Thanks for the feedback!

Member

eileencodes commented Oct 29, 2016

@rafaelfranca David and I talked about the naming etc this week and decided on creating a new gem inside Rails, so I'll be doing that soon. Thanks for the feedback!

@kinopyo kinopyo referenced this pull request Nov 7, 2016

Merged

Add rspec style guides #14

Show outdated Hide outdated actionsystemtest/actionsystemtest.gemspec
s.license = "MIT"
s.author = ["Eileen Uchitell", "David Heinemeier Hansson"]

This comment has been minimized.

@kaspth

kaspth Nov 11, 2016

Member

Personally I'd throw an e on there, but I'll let you be the judge of how to spell your name 馃槃

@kaspth

kaspth Nov 11, 2016

Member

Personally I'd throw an e on there, but I'll let you be the judge of how to spell your name 馃槃

This comment has been minimized.

@eileencodes

eileencodes Nov 11, 2016

Member

I noticed that, still working on a bunch of stuff. I'll let you know when I'm ready for more review.

@eileencodes

eileencodes Nov 11, 2016

Member

I noticed that, still working on a bunch of stuff. I'll let you know when I'm ready for more review.

This comment has been minimized.

@kaspth

kaspth Nov 11, 2016

Member

Sounds good 馃憤

@kaspth

kaspth Nov 11, 2016

Member

Sounds good 馃憤

Show outdated Hide outdated actionsystemtest/lib/action_system_test/driver_adapters.rb
# === Driver Features
#
# | | Default Browser | Supports Screenshots? |
# |-----------------|-----------------------|-----------------------|

This comment has been minimized.

@kaspth

kaspth Nov 11, 2016

Member

Nitpick: saw another table where we put spaces between the rows and pipes, e.g.: ---- | ----. Not sure if we have a preferred style.

@kaspth

kaspth Nov 11, 2016

Member

Nitpick: saw another table where we put spaces between the rows and pipes, e.g.: ---- | ----. Not sure if we have a preferred style.

Show outdated Hide outdated actionsystemtest/lib/action_system_test/gem_version.rb
@@ -0,0 +1,15 @@
module ActionSystemTest
# Returns the version of the currently loaded Action Cable as a <tt>Gem::Version</tt>.

This comment has been minimized.

@kaspth

kaspth Nov 11, 2016

Member

Let's be cable cutters: Action System Test, I presume 馃榿

@kaspth

kaspth Nov 11, 2016

Member

Let's be cable cutters: Action System Test, I presume 馃榿

Show outdated Hide outdated actionsystemtest/lib/action_system_test/version.rb
require_relative "gem_version"
module ActionSystemTest
# Returns the version of the currently loaded Action Cable as a <tt>Gem::Version</tt>

This comment has been minimized.

@kaspth

kaspth Nov 11, 2016

Member

Cable :)

@kaspth

kaspth Nov 11, 2016

Member

Cable :)

Show outdated Hide outdated railties/test/application/test_test.rb
puts "before: " + has_user_table.to_s
end
task :after_hook do
has_user_table = ActiveRecord::Base.connection.table_exists?('users')
has_user_table = ActiveRecord::Base.connection.data_source_exists?('users')

This comment has been minimized.

@kaspth

kaspth Nov 11, 2016

Member

How is this change related to system testing?

@kaspth

kaspth Nov 11, 2016

Member

How is this change related to system testing?

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helpers/form_helper.rb
# click_checkbox_label 'Admin', checked: true
def click_checkbox_label(name, checked: false)
field = find_checkbox(name, checked)
label = find_label_wrapper(field)

This comment has been minimized.

@twalpole

twalpole Nov 12, 2016

Contributor

Capybara provides a selector - https://github.com/jnicklas/capybara/blob/master/lib/capybara/selector.rb#L432 - for the label that wraps an input or where the id/for attributes match. so this could be find(:label, for: field)

@twalpole

twalpole Nov 12, 2016

Contributor

Capybara provides a selector - https://github.com/jnicklas/capybara/blob/master/lib/capybara/selector.rb#L432 - for the label that wraps an input or where the id/for attributes match. so this could be find(:label, for: field)

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helpers/form_helper.rb
end
private
def find_checkbox(name, checked)

This comment has been minimized.

@twalpole

twalpole Nov 12, 2016

Contributor

Capybara provides a :checkbox selector - https://github.com/jnicklas/capybara/blob/master/lib/capybara/selector.rb#L298 - which would limit this to only checkboxes if thats whats desired

@twalpole

twalpole Nov 12, 2016

Contributor

Capybara provides a :checkbox selector - https://github.com/jnicklas/capybara/blob/master/lib/capybara/selector.rb#L298 - which would limit this to only checkboxes if thats whats desired

@kaspth kaspth referenced this pull request Nov 22, 2016

Closed

ActionCable testing #23211

Show outdated Hide outdated ...es/lib/rails/generators/test_unit/system/templates/system_test_helper.rb
take_failed_screenshot
Capybara.reset_sessions!
end
end

This comment has been minimized.

@eileencodes

eileencodes Dec 15, 2016

Member

This file is generated when a new app is generated or a scaffold is generated and there is no system_test_helper file.

The reason for this file is there is a ton of other setup that Capybara can do and programmers need a place to put that code. Additionally some may not want to take_failed_screenshots or do other such work. This gives them a clear place to put anything and everything related to system tests or required for setup/teardown on every test.

@eileencodes

eileencodes Dec 15, 2016

Member

This file is generated when a new app is generated or a scaffold is generated and there is no system_test_helper file.

The reason for this file is there is a ton of other setup that Capybara can do and programmers need a place to put that code. Additionally some may not want to take_failed_screenshots or do other such work. This gives them a clear place to put anything and everything related to system tests or required for setup/teardown on every test.

This comment has been minimized.

@kaspth

kaspth Dec 15, 2016

Member

Great!

Should this call the Active Job helpers like setup :set_queue_adapter_to_async?

@kaspth

kaspth Dec 15, 2016

Member

Great!

Should this call the Active Job helpers like setup :set_queue_adapter_to_async?

This comment has been minimized.

@eileencodes

eileencodes Dec 15, 2016

Member

I was torn about that. I'm thinking - since AJ is included by default and created by default maybe it should be included by default in the generated file. At first I was thinking that you may not want to use AJ, but if it's auto generated then it should probably be included.

@eileencodes

eileencodes Dec 15, 2016

Member

I was torn about that. I'm thinking - since AJ is included by default and created by default maybe it should be included by default in the generated file. At first I was thinking that you may not want to use AJ, but if it's auto generated then it should probably be included.

This comment has been minimized.

@kaspth

kaspth Dec 20, 2016

Member

It does sound like baking it in is a good default, then people can just remove it themselves if it doesn't suit their needs.

@kaspth

kaspth Dec 20, 2016

Member

It does sound like baking it in is a good default, then people can just remove it themselves if it doesn't suit their needs.

Show outdated Hide outdated actionsystemtest/actionsystemtest.gemspec
s.license = "MIT"
s.author = ["Eileen Uchitelle", "David Heinemeier Hansson"]

This comment has been minimized.

@kaspth

kaspth Dec 15, 2016

Member

鉂わ笍

@kaspth

kaspth Dec 15, 2016

Member

鉂わ笍

Show outdated Hide outdated ...es/lib/rails/generators/test_unit/system/templates/system_test_helper.rb
take_failed_screenshot
Capybara.reset_sessions!
end
end

This comment has been minimized.

@kaspth

kaspth Dec 15, 2016

Member

Great!

Should this call the Active Job helpers like setup :set_queue_adapter_to_async?

@kaspth

kaspth Dec 15, 2016

Member

Great!

Should this call the Active Job helpers like setup :set_queue_adapter_to_async?

Fixed everything here!

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Dec 27, 2016

Member

This is ready for review from @dhh. I've finished all the above work: added guides, got tests in Railties passing, rebased, cleaned up some commits, and moved everything to a new gem called ActionSystemTest.

ActionSystemTest has a Base class so that the test class can be ActionSystemTestCase which system tests will inherit from. The reason for this is because the gem name must be a module, but test's inherit from a class. ActionSystemTestCase, which all system tests inherit from is defined in the system_test_helper.rb generated with the application, scaffold or test.

This file is where anyone writing system tests would add additional Capybara configuration if desired, and where the setup/teardown requirements live. Specifically out of the box tests handle resetting Capybara sessions, taking screenshots of failed tests, and setting the ActiveJob queue adapter to async.

One note: I did not add DatabaseCleaner type code to Active Record. After discussing this with Aaron we found that Active Record was behaving incorrectly and not closing connections after opening them.

Fixing that makes DatabaseCleaner unnecessary because then Active Record would know how to rollback. I'm still working on finishing those changes up because there are some failures in AR, but I don't consider this a blocker for merging system tests because 1) this doesn't affect models that use fixtures and 2) database cleaner still works

Member

eileencodes commented Dec 27, 2016

This is ready for review from @dhh. I've finished all the above work: added guides, got tests in Railties passing, rebased, cleaned up some commits, and moved everything to a new gem called ActionSystemTest.

ActionSystemTest has a Base class so that the test class can be ActionSystemTestCase which system tests will inherit from. The reason for this is because the gem name must be a module, but test's inherit from a class. ActionSystemTestCase, which all system tests inherit from is defined in the system_test_helper.rb generated with the application, scaffold or test.

This file is where anyone writing system tests would add additional Capybara configuration if desired, and where the setup/teardown requirements live. Specifically out of the box tests handle resetting Capybara sessions, taking screenshots of failed tests, and setting the ActiveJob queue adapter to async.

One note: I did not add DatabaseCleaner type code to Active Record. After discussing this with Aaron we found that Active Record was behaving incorrectly and not closing connections after opening them.

Fixing that makes DatabaseCleaner unnecessary because then Active Record would know how to rollback. I'm still working on finishing those changes up because there are some failures in AR, but I don't consider this a blocker for merging system tests because 1) this doesn't affect models that use fixtures and 2) database cleaner still works

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helper.rb
include TestHelpers::ScreenshotHelper
include Capybara::DSL
Capybara.app = Rack::Builder.new do

This comment has been minimized.

@jnicklas

jnicklas Jan 2, 2017

Contributor

It's a bit confusing that this configuration, which is global configuration is set inside a module. It makes it look as though this config only applies to this module, when it in fact applies to everthing.

@jnicklas

jnicklas Jan 2, 2017

Contributor

It's a bit confusing that this configuration, which is global configuration is set inside a module. It makes it look as though this config only applies to this module, when it in fact applies to everthing.

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

Agree, but also, I think we should encapsulate this in a bootstrap method somewhere. It feels oddly free-floating in a helper to do this kind of global configuration. (I'm thinking something like register_browser_driver).

@dhh

dhh Jan 10, 2017

Member

Agree, but also, I think we should encapsulate this in a bootstrap method somewhere. It feels oddly free-floating in a helper to do this kind of global configuration. (I'm thinking something like register_browser_driver).

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helpers.rb
module TestHelpers
extend ActiveSupport::Autoload
autoload :ActiveJobSetup

This comment has been minimized.

@jnicklas

jnicklas Jan 2, 2017

Contributor

Isn't autoload generally discouraged? Maybe this was fixed in newer Ruby versions?

@jnicklas

jnicklas Jan 2, 2017

Contributor

Isn't autoload generally discouraged? Maybe this was fixed in newer Ruby versions?

This comment has been minimized.

@bf4

bf4 Apr 30, 2017

Contributor

@jnicklas (I didn't see this answered elsewhere. My apologies if it was)

  1. this autoload is actually not Module::autoload, but one added by extending ActiveSupport::Autoload and is somewhat incompatible. The Rails version uses dependency tracking to ensure what only what is needed is booted, and that what that needs is also booted :)
  2. The Ruby Module.autoload has been threadsafe since sometime in 2.0 across all Ruby impls. For convenience, pasting some discussion from headius/thread_safe#15 (comment)

Why does the thread_safe gem use autoload which is known not to be thread-safe? (Or so I understand)

Currently autoload is not safe to use in a multi-threaded application. To put it more bluntly, it's broken.

The current logic for autoload is as follows:

  1. A special object is inserted into the target constant table, used as a marker for autoloading
  2. When that constant is looked up, the marker is found and triggers autoloading
  3. The marker is first removed, so the constant now appears to be undefined if retrieved concurrently
  4. The associated autoload resource is required, and presumably redefines the constant in question
  5. The constant lookup, upon completion of autoload, looks up the constant again and either returns its new value or proceeds with normal constant resolution

The problem arises when two or more threads try to access the constant. Because autoload is stateful and unsynchronized, the second thread may encounter the constant table in any number of states:

  1. It may see the autoload has not yet fired, if the first thread has encountered the marker but not yet removed it. It would then proceed along the same autoload path, requiring the same file a second time.
  2. It may not find an autoload marker, and assume the constant does not exist.
  3. It may see the eventual constant the autoload was intended to define.

Of these combinations, (3) is obviously the desired behavior. (1) can only happen on native-threaded implementations that do not have a global interpreter lock, since it requires concurrency during autoload's internal logic. (2) can happen on any implementation, since while the required file is processing the original autoload constant appears to be undefined.

'autoload will be dead, I strongly discourage the use of autoload in any standard libraries'

The @thedarkone wrote that autoload has been thread-safe since about 2012. So, I looked it up:

@thedarkone So, the internet is wrong inasmuch as autoload is thread-safe in

@bf4

bf4 Apr 30, 2017

Contributor

@jnicklas (I didn't see this answered elsewhere. My apologies if it was)

  1. this autoload is actually not Module::autoload, but one added by extending ActiveSupport::Autoload and is somewhat incompatible. The Rails version uses dependency tracking to ensure what only what is needed is booted, and that what that needs is also booted :)
  2. The Ruby Module.autoload has been threadsafe since sometime in 2.0 across all Ruby impls. For convenience, pasting some discussion from headius/thread_safe#15 (comment)

Why does the thread_safe gem use autoload which is known not to be thread-safe? (Or so I understand)

Currently autoload is not safe to use in a multi-threaded application. To put it more bluntly, it's broken.

The current logic for autoload is as follows:

  1. A special object is inserted into the target constant table, used as a marker for autoloading
  2. When that constant is looked up, the marker is found and triggers autoloading
  3. The marker is first removed, so the constant now appears to be undefined if retrieved concurrently
  4. The associated autoload resource is required, and presumably redefines the constant in question
  5. The constant lookup, upon completion of autoload, looks up the constant again and either returns its new value or proceeds with normal constant resolution

The problem arises when two or more threads try to access the constant. Because autoload is stateful and unsynchronized, the second thread may encounter the constant table in any number of states:

  1. It may see the autoload has not yet fired, if the first thread has encountered the marker but not yet removed it. It would then proceed along the same autoload path, requiring the same file a second time.
  2. It may not find an autoload marker, and assume the constant does not exist.
  3. It may see the eventual constant the autoload was intended to define.

Of these combinations, (3) is obviously the desired behavior. (1) can only happen on native-threaded implementations that do not have a global interpreter lock, since it requires concurrency during autoload's internal logic. (2) can happen on any implementation, since while the required file is processing the original autoload constant appears to be undefined.

'autoload will be dead, I strongly discourage the use of autoload in any standard libraries'

The @thedarkone wrote that autoload has been thread-safe since about 2012. So, I looked it up:

@thedarkone So, the internet is wrong inasmuch as autoload is thread-safe in

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helpers/assertions.rb
#
# assert_all_of_selectors(:avatar, 'Eileen', 'Jeremy')
# assert_all_of_selectors(:avatar, 'Eileen', 'Jeremy', visible: all)
def assert_all_of_selectors(selector, *items, **options)

This comment has been minimized.

@jnicklas

jnicklas Jan 2, 2017

Contributor

I think these helpers are quite useful, but I'd rather see them moved upstream to Capybara itself, otherwise we're creating a "Rails-dialect" of Capybara, which I think would be much harder for newer users to grasp.

@jnicklas

jnicklas Jan 2, 2017

Contributor

I think these helpers are quite useful, but I'd rather see them moved upstream to Capybara itself, otherwise we're creating a "Rails-dialect" of Capybara, which I think would be much harder for newer users to grasp.

This comment has been minimized.

@twalpole

twalpole Jan 2, 2017

Contributor

I've created a PR for adding these methods to Capybara - teamcapybara/capybara#1820 - the only real difference is that the :wait option will apply to the group as a whole rather than each individual selector check

@twalpole

twalpole Jan 2, 2017

Contributor

I've created a PR for adding these methods to Capybara - teamcapybara/capybara#1820 - the only real difference is that the :wait option will apply to the group as a whole rather than each individual selector check

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helpers/screenshot_helper.rb
module ActionSystemTest
module TestHelpers
# Screenshot helper for system testing
module ScreenshotHelper

This comment has been minimized.

@jnicklas

jnicklas Jan 2, 2017

Contributor

I'm not really sure if the inclusion of this is warranted.

@jnicklas

jnicklas Jan 2, 2017

Contributor

I'm not really sure if the inclusion of this is warranted.

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

I like it. I think it's exactly the kind of process improvement that makes the out-of-the-box configuration feels extra nice. We use those screenshots on every failure for BC3 to help diagnose.

@dhh

dhh Jan 10, 2017

Member

I like it. I think it's exactly the kind of process improvement that makes the out-of-the-box configuration feels extra nice. We use those screenshots on every failure for BC3 to help diagnose.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Jan 2, 2017

Contributor

Pinging @twalpole, the maintainer of Capybara, for his input on this as well.

I've had a look through this PR, and I have some comments:

I think it's not optimal to add a layer around Capybara drivers like this is doing. I understand that there is some desire from Rails' perspective to take ownership of the experience here, but this has a significant downside that we should be aware of: it requires every Capybara driver to also have a Rails-Capybara driver. It also just seems plain unnecessary to me, since Capybara already has a system for registering and managing drivers. If the desire is to make this look more Rails-native then simply building a thin shim around this system would be preferable, I think.

For example, supposing that Rails wants to change some defaults for the selenium driver, then Rails could just to something like this:

Capybara.register_driver :rails_selenium do |app|
  Capybara::Selenium::Driver.new(app, browser: :chrome)
end

Capybara.default_driver = :rails_selenium

And Rails could build a shim around Capybara.default_driver and Capybara.current_driver like this:

module ActionSystemTest
  def self.current_driver
    Capybara.current_driver
  end

  def self.current_driver=(name)
    Capybara.current_driver = name
  end
end

I think that maybe the purpose of this configuration is to be able to switch to something other than Capybara for these tests, but if that is the case, I'd like you to consider the following two issues: 1) Actually building an alternative driver would be a huge undertaking 2) This is precisely what Capybara already does and was in fact exactly what Capybara was created for, abstracting away the differences between multiple drivers transparently.

I think it'd be best to avoid adding a "Rails-dialect" of Capybara, where some methods come from Capybara, some from Rails, etc... This would make it much more difficult for users to discover where methods are coming from, and create confusion for people switching between ActionSystemTest and plain Capybara projects.

Contributor

jnicklas commented Jan 2, 2017

Pinging @twalpole, the maintainer of Capybara, for his input on this as well.

I've had a look through this PR, and I have some comments:

I think it's not optimal to add a layer around Capybara drivers like this is doing. I understand that there is some desire from Rails' perspective to take ownership of the experience here, but this has a significant downside that we should be aware of: it requires every Capybara driver to also have a Rails-Capybara driver. It also just seems plain unnecessary to me, since Capybara already has a system for registering and managing drivers. If the desire is to make this look more Rails-native then simply building a thin shim around this system would be preferable, I think.

For example, supposing that Rails wants to change some defaults for the selenium driver, then Rails could just to something like this:

Capybara.register_driver :rails_selenium do |app|
  Capybara::Selenium::Driver.new(app, browser: :chrome)
end

Capybara.default_driver = :rails_selenium

And Rails could build a shim around Capybara.default_driver and Capybara.current_driver like this:

module ActionSystemTest
  def self.current_driver
    Capybara.current_driver
  end

  def self.current_driver=(name)
    Capybara.current_driver = name
  end
end

I think that maybe the purpose of this configuration is to be able to switch to something other than Capybara for these tests, but if that is the case, I'd like you to consider the following two issues: 1) Actually building an alternative driver would be a huge undertaking 2) This is precisely what Capybara already does and was in fact exactly what Capybara was created for, abstracting away the differences between multiple drivers transparently.

I think it'd be best to avoid adding a "Rails-dialect" of Capybara, where some methods come from Capybara, some from Rails, etc... This would make it much more difficult for users to discover where methods are coming from, and create confusion for people switching between ActionSystemTest and plain Capybara projects.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Jan 2, 2017

Contributor

Also, since my post was a bit critical I want to say thank you to @eileencodes for pushing forward on this and doing so much work to make this happen. Making it easier and more discoverable for Rails users to use Capybara/system/integration tests is really great.

Contributor

jnicklas commented Jan 2, 2017

Also, since my post was a bit critical I want to say thank you to @eileencodes for pushing forward on this and doing so much work to make this happen. Making it easier and more discoverable for Rails users to use Capybara/system/integration tests is really great.

@eileencodes

This comment has been minimized.

Show comment
Hide comment
@eileencodes

eileencodes Jan 2, 2017

Member
Member

eileencodes commented Jan 2, 2017

@dhh

First pass of a review.

Show outdated Hide outdated actionsystemtest/lib/action_system_test.rb
# require 'system_test_helper'
#
# class Users::CreateTest < ActionSystemTestCase
# def adding_a_new_user

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

Should use the string-based definition form, so test "adding a new user".

@dhh

dhh Jan 10, 2017

Member

Should use the string-based definition form, so test "adding a new user".

Show outdated Hide outdated actionsystemtest/README.md
```ruby
class ActionSystemTestCase < ActionSystemTest::Base
ActionSystemTest.driver = ActionSystemTest::DriverAdapters::CapybaraDriver.new(

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

There's something odd about this way of configuring the driver. Since ActionSystemTest is a global, it's weird to set properties on it within the scope of a ActionSystemTestCase class. Like, is there some hook that automatically rolls this driver setting back when this case is done running? Or are we just generally setting a global that'll be in place until we change it? If the latter, it should be set outside the scope of the class definition.

But I think even better would be to extract these settings to a higher level, so it's something like:

class ActionSystemTestCase < ActionSystemTest::Base
  driven_by :poltergeist, server: :webkit, port: 3000
end
@dhh

dhh Jan 10, 2017

Member

There's something odd about this way of configuring the driver. Since ActionSystemTest is a global, it's weird to set properties on it within the scope of a ActionSystemTestCase class. Like, is there some hook that automatically rolls this driver setting back when this case is done running? Or are we just generally setting a global that'll be in place until we change it? If the latter, it should be set outside the scope of the class definition.

But I think even better would be to extract these settings to a higher level, so it's something like:

class ActionSystemTestCase < ActionSystemTest::Base
  driven_by :poltergeist, server: :webkit, port: 3000
end

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

As I reflected further on the whole poltergeist vs selenium vs whatever, I realized that this configuration point is still too low level. We shouldn't be caring about whether its poltergeist or whatever. We should be caring about which browser we're simulating, whether it's headless or not, and then perhaps the port we're hitting against. Need more work there.

@dhh

dhh Jan 10, 2017

Member

As I reflected further on the whole poltergeist vs selenium vs whatever, I realized that this configuration point is still too low level. We shouldn't be caring about whether its poltergeist or whatever. We should be caring about which browser we're simulating, whether it's headless or not, and then perhaps the port we're hitting against. Need more work there.

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

Further reflection: I think we should split these configuration points into two separate things: 1) What server are we providing, like puma on 3000. 2) What driver type are we using, so that's which browser we're simulating and whether we're headless or not.

@dhh

dhh Jan 10, 2017

Member

Further reflection: I think we should split these configuration points into two separate things: 1) What server are we providing, like puma on 3000. 2) What driver type are we using, so that's which browser we're simulating and whether we're headless or not.

Show outdated Hide outdated actionsystemtest/lib/action_system_test.rb
# tests.
#
# class ActionSystemTestCase < ActionSystemTest::Base
# ActionSystemTest.driver = :rack_test

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

I don't think we have a compelling argument to ship with a Rack driver or highlight its availability. Since the only reason you'd suffer the slowdown of running through a browser for testing is to get the JavaScript tested too, I don't think it makes sense to promote a rack way that doesn't allow this.

I'd vote to nix this option. If there's a backdoor somewhere to do it by setting things by hand, fine. But it shouldn't be promoted and we should make no special accommodations for it.

@dhh

dhh Jan 10, 2017

Member

I don't think we have a compelling argument to ship with a Rack driver or highlight its availability. Since the only reason you'd suffer the slowdown of running through a browser for testing is to get the JavaScript tested too, I don't think it makes sense to promote a rack way that doesn't allow this.

I'd vote to nix this option. If there's a backdoor somewhere to do it by setting things by hand, fine. But it shouldn't be promoted and we should make no special accommodations for it.

Show outdated Hide outdated actionsystemtest/README.md
end
```
Capybara itself provides 4 drivers: RackTest, Selenium, Webkit, and Poltergeist.

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

I don't think the name/type of driver is actually important to dwell on for these system tests. What we care about is what these drivers provide: Firefox, Chrome, Safari?, and headless/not driving. Let's bring the documentation up to talk about these areas configuration points and downplay the fact of whatever underlying driver is supplying what.

@dhh

dhh Jan 10, 2017

Member

I don't think the name/type of driver is actually important to dwell on for these system tests. What we care about is what these drivers provide: Firefox, Chrome, Safari?, and headless/not driving. Let's bring the documentation up to talk about these areas configuration points and downplay the fact of whatever underlying driver is supplying what.

Show outdated Hide outdated actionsystemtest/README.md
end
```
First we visit the +users_path+. From there we are going to use Action System

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

We should be using _url's here, rather than _path's, since we're simulating full browser requests.

@dhh

dhh Jan 10, 2017

Member

We should be using _url's here, rather than _path's, since we're simulating full browser requests.

Show outdated Hide outdated actionsystemtest/lib/action_system_test/test_helpers/screenshot_helper.rb
module ActionSystemTest
module TestHelpers
# Screenshot helper for system testing
module ScreenshotHelper

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

I like it. I think it's exactly the kind of process improvement that makes the out-of-the-box configuration feels extra nice. We use those screenshots on every failure for BC3 to help diagnose.

@dhh

dhh Jan 10, 2017

Member

I like it. I think it's exactly the kind of process improvement that makes the out-of-the-box configuration feels extra nice. We use those screenshots on every failure for BC3 to help diagnose.

Show outdated Hide outdated guides/source/testing.md
```
The `helpers`, `mailers`, and `models` directories are meant to hold tests for view helpers, mailers, and models, respectively. The `controllers` directory is meant to hold tests for controllers, routes, and views. The `integration` directory is meant to hold tests for interactions between controllers.
The system test directory holds system tests, also known as acceptance tests.
System tests inherit from Capybara and perform in browser tests for your
application.

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

Let's nail the value proposition clearer here. You do system tests when you want to test with JavaScript and through a real browser. The other stuff is not important in comparison.

@dhh

dhh Jan 10, 2017

Member

Let's nail the value proposition clearer here. You do system tests when you want to test with JavaScript and through a real browser. The other stuff is not important in comparison.

Show outdated Hide outdated guides/source/testing.md
```
Here the test is inheriting from `ActionSystemTestCase`. This allows for no-setup
Capybara integration with Selenium Webdriver.

This comment has been minimized.

@dhh

dhh Jan 10, 2017

Member

What other options are there for inheritance?

@dhh