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

Integrate with ActionDispatch::SystemTest #1813

Merged
merged 22 commits into from Aug 26, 2017

Conversation

Projects
None yet
@samphippen
Member

samphippen commented May 5, 2017

TODO:

  • Implementation cleanup
  • ensure it works with infer_spec_type_from_file_location
  • docs
  • blog post with example
  • get some user testing in real rails apps
@elsurudo

This comment has been minimized.

Show comment
Hide comment
@elsurudo

elsurudo May 11, 2017

Great work! Any estimate on when this will be merged?

elsurudo commented May 11, 2017

Great work! Any estimate on when this will be merged?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe May 11, 2017

Member

@elsurudo we'd gladly accept help on this, in the mean time our type: :feature specs work well with the new transactional setup and you can capybara on

Member

JonRowe commented May 11, 2017

@elsurudo we'd gladly accept help on this, in the mean time our type: :feature specs work well with the new transactional setup and you can capybara on

citizen428 added a commit to lockstep/rails_new that referenced this pull request May 24, 2017

Remove DatabaseCleaner
This should JustWork™ in Rails 5.1+. I’m tracking
rspec/rspec-rails#1813 so
We can eventually integrate system specs (the now
“official” Rails way) instead of feature specs.
@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jun 16, 2017

Member

This is now ready for user testing. Please let me know if you'd like to use this and give me some feedback

Member

samphippen commented Jun 16, 2017

This is now ready for user testing. Please let me know if you'd like to use this and give me some feedback

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jun 17, 2017

Member

@JonRowe would you be able to provide review here?

Member

samphippen commented Jun 17, 2017

@JonRowe would you be able to provide review here?

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jun 18, 2017

I tried this out on a Rails 5.1 project and got things mostly working. Here was my general process:

  1. Update my rspec-rails dependency to point at this branch, and point other rspec-* deps at their master branch.
  2. Remove database_cleaner gem and associated configuration! 🎊
  3. Turned on config.use_transactional_fixtures because no more DatabaseCleaner! 🎆 🍾
  4. Move all tests from spec/features to spec/system because we use infer_spec_type_from_file_location! (sorry (not sorry), Sam).
  5. Add the following global configuration to maintain our previous use of Rack::Test for tests that don't require JS.
RSpec.configure do |config|
  config.before(:each, type: :system) do
    driven_by :rack_test
  end

  config.before(:each, type: :system, js: true) do
    driven_by :headless_chrome # a driver I define elsewhere
  end
end

Results

It kinda worked... I had a couple of problems.

  1. We use an outermost feature keyword in our "feature" (now "system") specs with nested scenarios rather than describe and it. The use of feature seems to override both the location-inferred spec type and an explicitly set spec type and set the example group to feature. Doing a replace of feature with describe in spec/system fixed this issue.
  2. I was not able to set the driver in a global before as specified above. Capybara would always use the Rails system test default. I was able to set the driver in line in the test.
  3. System tests are configured to take a screenshot when a test fails. The screenshot is always written to tmp/screenshots/failures_.png which means you only get to review the last failure's screenshot. I've never used the Rails system tests, but judging by the dangling _ in the filename, I'm guessing there's supposed to be some sort of unique identifier there.

The first problem stems from the use of feature syntax. This is an old habit that maybe should just go away. I'm not sure how many people use it, but we use it extensively and exclusively at thoughtbot. I can't think of a good reason to keep it, but it's rather puzzling when one can't figure out why their global type: :system before blocks aren't running.

I also encountered what I consider to be an unfortunate, if minor, annoyance. I like my test output to be as clean as possible -- just the test status indicators with no output. System tests seem to muck this up quite a bit. See:

terminal_ _tmux_ _tat_ _130x40

The first bit of output is from puma starting up, presumably when RSpec hits its first system test. I'm glad to see it doesn't log all requests, but it'd sure be awesome if we could suppress this output somehow.

Then you see my test failures. The screenshot writes a log in-line. The "RSpec-friendly" way to do this would be to include that output with the detailed failure message.

I'm guessing that these annoyances are mostly out of RSpec's control and have to do with how system tests are implemented in Rails. Is there something we could contribute back to Rails to make the output play nicer? It seems to be that if this outputs during minitest runs, it'd be equally annoying for folks like me? Maybe I'm just too much of a neat freak?

derekprior commented Jun 18, 2017

I tried this out on a Rails 5.1 project and got things mostly working. Here was my general process:

  1. Update my rspec-rails dependency to point at this branch, and point other rspec-* deps at their master branch.
  2. Remove database_cleaner gem and associated configuration! 🎊
  3. Turned on config.use_transactional_fixtures because no more DatabaseCleaner! 🎆 🍾
  4. Move all tests from spec/features to spec/system because we use infer_spec_type_from_file_location! (sorry (not sorry), Sam).
  5. Add the following global configuration to maintain our previous use of Rack::Test for tests that don't require JS.
RSpec.configure do |config|
  config.before(:each, type: :system) do
    driven_by :rack_test
  end

  config.before(:each, type: :system, js: true) do
    driven_by :headless_chrome # a driver I define elsewhere
  end
end

Results

It kinda worked... I had a couple of problems.

  1. We use an outermost feature keyword in our "feature" (now "system") specs with nested scenarios rather than describe and it. The use of feature seems to override both the location-inferred spec type and an explicitly set spec type and set the example group to feature. Doing a replace of feature with describe in spec/system fixed this issue.
  2. I was not able to set the driver in a global before as specified above. Capybara would always use the Rails system test default. I was able to set the driver in line in the test.
  3. System tests are configured to take a screenshot when a test fails. The screenshot is always written to tmp/screenshots/failures_.png which means you only get to review the last failure's screenshot. I've never used the Rails system tests, but judging by the dangling _ in the filename, I'm guessing there's supposed to be some sort of unique identifier there.

The first problem stems from the use of feature syntax. This is an old habit that maybe should just go away. I'm not sure how many people use it, but we use it extensively and exclusively at thoughtbot. I can't think of a good reason to keep it, but it's rather puzzling when one can't figure out why their global type: :system before blocks aren't running.

I also encountered what I consider to be an unfortunate, if minor, annoyance. I like my test output to be as clean as possible -- just the test status indicators with no output. System tests seem to muck this up quite a bit. See:

terminal_ _tmux_ _tat_ _130x40

The first bit of output is from puma starting up, presumably when RSpec hits its first system test. I'm glad to see it doesn't log all requests, but it'd sure be awesome if we could suppress this output somehow.

Then you see my test failures. The screenshot writes a log in-line. The "RSpec-friendly" way to do this would be to include that output with the detailed failure message.

I'm guessing that these annoyances are mostly out of RSpec's control and have to do with how system tests are implemented in Rails. Is there something we could contribute back to Rails to make the output play nicer? It seems to be that if this outputs during minitest runs, it'd be equally annoying for folks like me? Maybe I'm just too much of a neat freak?

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jun 20, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jun 20, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jun 20, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jun 20, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813
@clupprich

This comment has been minimized.

Show comment
Hide comment
@clupprich

clupprich Jun 22, 2017

I've followed the steps @derekprior describes in his comment above. Things work smoothly, besides the custom driven_by block:

RSpec.configure do |config|
  config.before(:each, type: :system) do
    driven_by :rack_test
  end

  config.before(:each, type: :system, js: true) do
    driven_by :chrome
  end
end

The driver isn't actually switched to rack_test or chrome for me, but the default selenium is used. Maybe that's because it's overwritten in this before block?

clupprich commented Jun 22, 2017

I've followed the steps @derekprior describes in his comment above. Things work smoothly, besides the custom driven_by block:

RSpec.configure do |config|
  config.before(:each, type: :system) do
    driven_by :rack_test
  end

  config.before(:each, type: :system, js: true) do
    driven_by :chrome
  end
end

The driver isn't actually switched to rack_test or chrome for me, but the default selenium is used. Maybe that's because it's overwritten in this before block?

@JonRowe

Nice to see how simple this is, few minor comments.

if /5(\.|-)1/ === RAILS_VERSION || "master" == RAILS_VERSION
gem 'capybara', '~> 2.13', :require => false
else
gem 'capybara', '~> 2.2.0', :require => false

This comment has been minimized.

@JonRowe

JonRowe Jun 23, 2017

Member

Why doesn't bundler automatically work this out?

@JonRowe

JonRowe Jun 23, 2017

Member

Why doesn't bundler automatically work this out?

This comment has been minimized.

@samphippen

samphippen Jun 24, 2017

Member

The pin to 2.2.0 prevents us from bumping further than the highest 2.2.x rails needs at least 2.13. However, we pin to the earliest version that we support, so this is why this is conditionaled like this.

@samphippen

samphippen Jun 24, 2017

Member

The pin to 2.2.0 prevents us from bumping further than the highest 2.2.x rails needs at least 2.13. However, we pin to the earliest version that we support, so this is why this is conditionaled like this.

This comment has been minimized.

@JonRowe

JonRowe Jun 24, 2017

Member

👍

@JonRowe
when /stable$/
gem_list = %w[rails railties actionmailer actionpack activerecord activesupport]
gem_list << 'activejob' if version > '4-1-stable'
gem_list << 'actionview' if version > '4-0-stable'
if RUBY_VERSION >= "1.9.3"
gem_list << 'puma' if version > '5-0-stable'

This comment has been minimized.

@JonRowe

JonRowe Jun 23, 2017

Member

5-0 or 5-1

@JonRowe

JonRowe Jun 23, 2017

Member

5-0 or 5-1

Show outdated Hide outdated example_app_generator/generate_app.rb
@@ -20,6 +20,7 @@
gsub_file 'Gemfile', /^.*\bgem 'rails.*$/, ''
gsub_file "Gemfile", /.*web-console.*/, ''
gsub_file "Gemfile", /.*debugger.*/, ''
gsub_file "Gemfile", /.*puma.*/, ""

This comment has been minimized.

@JonRowe

JonRowe Jun 23, 2017

Member

Will every generated app now need puma, shouldn't this be in a guard statement?

@JonRowe

JonRowe Jun 23, 2017

Member

Will every generated app now need puma, shouldn't this be in a guard statement?

This comment has been minimized.

@samphippen

samphippen Jun 24, 2017

Member

this deletes the generated puma requirement from the default rails generator, replacing it with ours.

@samphippen

samphippen Jun 24, 2017

Member

this deletes the generated puma requirement from the default rails generator, replacing it with ours.

This comment has been minimized.

@JonRowe

JonRowe Jun 24, 2017

Member

Copy

@JonRowe
the default `driven_by(:selenium)` from Rails. If you want to override this
behaviour you can call `driven_by` manually in a test.

This comment has been minimized.

@JonRowe

JonRowe Jun 23, 2017

Member

Excess white space, my old nemesis, we meet again.

@JonRowe

JonRowe Jun 23, 2017

Member

Excess white space, my old nemesis, we meet again.

We encourage you to familiarse yourself with their documentation.
RSpec **does not** use your `ApplicationSystemTestCase` helper. Instead it uses
the default `driven_by(:selenium)` from Rails. If you want to override this

This comment has been minimized.

@JonRowe

JonRowe Jun 23, 2017

Member

Might it be worth us overriding this and defaulting to rack test?

@JonRowe

JonRowe Jun 23, 2017

Member

Might it be worth us overriding this and defaulting to rack test?

This comment has been minimized.

@samphippen

samphippen Jun 24, 2017

Member

Strongly disagree, selenium is the default in Rails and so I think it's better to mirror that default.

@samphippen

samphippen Jun 24, 2017

Member

Strongly disagree, selenium is the default in Rails and so I think it's better to mirror that default.

This comment has been minimized.

@JonRowe

JonRowe Jun 24, 2017

Member

Your call, just a suggestion

@JonRowe

JonRowe Jun 24, 2017

Member

Your call, just a suggestion

sgrif pushed a commit to rails/rails that referenced this pull request Jun 24, 2017

Add an option to silence puma in system tests.
This is motivated by our usage of system test in RSpec. Puma lazily
boots the first time a system test is used, but this causes some
unfortunate output to appear in the middle of the user's green dots. An
example of this can be seen in @derekprior's comment
[here](rspec/rspec-rails#1813 (comment)).

There are alternatives in RSpec where we attempt to intercept the puma
boot and prevent the output from being made there, but that would
involve some monkey patching. This seems like a cleaner solution.
@JonRowe

I'm happy if you're happy @samphippen merge on 📗

System specs are RSpec's wrapper around Rails' own
[system tests](http://guides.rubyonrails.org/testing.html#system-testing).
We encourage you to familiarse yourself with their documentation.

This comment has been minimized.

@abevoelker

abevoelker Jun 25, 2017

s/familiarse/familiarise/

@abevoelker

abevoelker Jun 25, 2017

s/familiarse/familiarise/

jgarber623 added a commit to jgarber623/sixtwothree.org that referenced this pull request Jul 5, 2017

Initial commit of application's next version
A fresh Rails 5.1 app was generated with:

```
rails new . -d postgresql --skip-action-mailer --skip-spring
  --skip-listen --skip-coffee --skip-turbolinks --skip-test
  --skip-bundle --skip-keeps
```

Updated configuration files were added for the following:

- Code Climate
- EditorConfig
- Rspec
- Rubocop
- Travis CI

For now, the app will use Rspec feature specs instead of the new
Rails 5.1 system tests pending resolution of this rspec/rspec-rails PR:
rspec/rspec-rails#1813

jgarber623 added a commit to jgarber623/sixtwothree.org that referenced this pull request Jul 5, 2017

Initial commit of application's next version
A fresh Rails 5.1 app was generated with:

```
rails new . -d postgresql --skip-action-mailer --skip-spring
  --skip-listen --skip-coffee --skip-turbolinks --skip-test
  --skip-bundle --skip-keeps
```

Updated configuration files were added for the following:

- Code Climate
- EditorConfig
- Rspec
- Rubocop
- Travis CI

For now, the app will use Rspec feature specs instead of the new
Rails 5.1 system tests pending resolution of this rspec/rspec-rails PR:
rspec/rspec-rails#1813

jgarber623 added a commit to jgarber623/sixtwothree.org that referenced this pull request Jul 5, 2017

Initial commit of application's next version
A fresh Rails 5.1 app was generated with:

```
rails new . -d postgresql --skip-action-mailer --skip-spring
  --skip-listen --skip-coffee --skip-turbolinks --skip-test
  --skip-bundle --skip-keeps
```

Updated configuration files were added for the following:

- Code Climate
- EditorConfig
- Rspec
- Rubocop
- Travis CI

For now, the app will use Rspec feature specs instead of the new
Rails 5.1 system tests pending resolution of this rspec/rspec-rails PR:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Jul 7, 2017

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

@samphippen samphippen changed the title from WIP: Integrate with ActionDispatch::SystemTest to Integrate with ActionDispatch::SystemTest Jul 7, 2017

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jul 7, 2017

Member

This is now ready for release pending rails/rails@eba3618 being released in a stable version of rails (presumably 5.1.3)

Member

samphippen commented Jul 7, 2017

This is now ready for release pending rails/rails@eba3618 being released in a stable version of rails (presumably 5.1.3)

@maschwenk

This comment has been minimized.

Show comment
Hide comment
@maschwenk

maschwenk Aug 3, 2017

@samphippen Excited for this, and stable Rails 5.1.3 shipped today. Let's 👊 it?

maschwenk commented Aug 3, 2017

@samphippen Excited for this, and stable Rails 5.1.3 shipped today. Let's 👊 it?

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Aug 3, 2017

@maschwenk rails/rails@eba3618 didn't make it into 5.1.3 - will be in 5.1.4

twalpole commented Aug 3, 2017

@maschwenk rails/rails@eba3618 didn't make it into 5.1.3 - will be in 5.1.4

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Sep 10, 2017

@mhenrixon Most likely because Rails overwrites your registration of :selenium when driven_by is called - https://github.com/rails/rails/blob/5-1-stable/actionpack/lib/action_dispatch/system_testing/driver.rb#L23 . Either change the name of the driver you're registering to something other than :selenium (:poltergeist and :webkit would get overwritten too) or pass the requisite options through to driven_by

twalpole commented Sep 10, 2017

@mhenrixon Most likely because Rails overwrites your registration of :selenium when driven_by is called - https://github.com/rails/rails/blob/5-1-stable/actionpack/lib/action_dispatch/system_testing/driver.rb#L23 . Either change the name of the driver you're registering to something other than :selenium (:poltergeist and :webkit would get overwritten too) or pass the requisite options through to driven_by

@mhenrixon

This comment has been minimized.

Show comment
Hide comment
@mhenrixon

mhenrixon Sep 10, 2017

Ah! That makes sense, I kind of like that approach. It avoids confusion with naming. Thanks a bunch @twalpole

mhenrixon commented Sep 10, 2017

Ah! That makes sense, I kind of like that approach. It avoids confusion with naming. Thanks a bunch @twalpole

@dgmora

This comment has been minimized.

Show comment
Hide comment
@dgmora

dgmora Sep 22, 2017

Hi guys, I've already read the comments but it'd be really nice to be able to use this from a released version, even a beta one. What's missing to have a new release out?

dgmora commented Sep 22, 2017

Hi guys, I've already read the comments but it'd be really nice to be able to use this from a released version, even a beta one. What's missing to have a new release out?

@Petercopter

This comment has been minimized.

Show comment
Hide comment
@Petercopter

Petercopter Oct 2, 2017

How does this affect the feature/scenario syntax?
https://relishapp.com/rspec/rspec-rails/docs/feature-specs/feature-spec

I've got things working in our test suite, but we have some specs that use the feature/scenario syntax, and those are having trouble loading our auth helpers.

Petercopter commented Oct 2, 2017

How does this affect the feature/scenario syntax?
https://relishapp.com/rspec/rspec-rails/docs/feature-specs/feature-spec

I've got things working in our test suite, but we have some specs that use the feature/scenario syntax, and those are having trouble loading our auth helpers.

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 2, 2017

@Petercopter Using RSpec.feature to specify your tests sets the test type to :feature, so if you've changed your auth helpers to only load in system tests, that would explain it. I'm wondering what you are gaining by swapping them all from feature tests to system tests?

twalpole commented Oct 2, 2017

@Petercopter Using RSpec.feature to specify your tests sets the test type to :feature, so if you've changed your auth helpers to only load in system tests, that would explain it. I'm wondering what you are gaining by swapping them all from feature tests to system tests?

@Petercopter

This comment has been minimized.

Show comment
Hide comment
@Petercopter

Petercopter Oct 2, 2017

@twalpole You must be the one of the busiest guys in the Rails community. Thanks so much for your quick response to my random questions in several different repos.

I guess the problem we have would be that we have inconsistent syntax in our test suite. It seems like the best course of action would be to convert to the same syntax throughout the suite.

I'm curious as well, about what is to be gained. It's new and shiny, so it must be better! 😸

Petercopter commented Oct 2, 2017

@twalpole You must be the one of the busiest guys in the Rails community. Thanks so much for your quick response to my random questions in several different repos.

I guess the problem we have would be that we have inconsistent syntax in our test suite. It seems like the best course of action would be to convert to the same syntax throughout the suite.

I'm curious as well, about what is to be gained. It's new and shiny, so it must be better! 😸

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 2, 2017

@Petercopter Really the only difference between system tests and feature tests when using RSpec with Capybara is a little being of configuration that can be saved (if you are ok with the default config chosen by Rails)- both get the biggest new feature which is shared DB connections in Rails 5.1 (obviating the need for database_cleaner in most cases)

twalpole commented Oct 2, 2017

@Petercopter Really the only difference between system tests and feature tests when using RSpec with Capybara is a little being of configuration that can be saved (if you are ok with the default config chosen by Rails)- both get the biggest new feature which is shared DB connections in Rails 5.1 (obviating the need for database_cleaner in most cases)

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Oct 2, 2017

Ah, rspec feature tests get shared DB connections in Rails 5.1, meaning database cleaner config can be changed? I did not realize this, I assumed it would need to be a 'system' test to take advantage of this new rails 5.1 feature.

jrochkind commented Oct 2, 2017

Ah, rspec feature tests get shared DB connections in Rails 5.1, meaning database cleaner config can be changed? I did not realize this, I assumed it would need to be a 'system' test to take advantage of this new rails 5.1 feature.

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 2, 2017

@jrochkind Yes, it's enabled when RAILS_ENV == test and rails transactional testing is enabled - IIRC

twalpole commented Oct 2, 2017

@jrochkind Yes, it's enabled when RAILS_ENV == test and rails transactional testing is enabled - IIRC

@bluz71

This comment has been minimized.

Show comment
Hide comment
@bluz71

bluz71 Oct 3, 2017

@twalpole, I just got rid of Database Cleaner from my feature specs and everything is working great (very nice). So would you recommend I stop there or should I convert these feature specs over to system specs?

Reading between the lines it seems your suggestion would be to leave feature specs as is (why change something that does not need changing)? Hence, I assume RSpec will respect feature specs for the long-term? Not deprecate as an old and not-modern way.

Many thanks.

bluz71 commented Oct 3, 2017

@twalpole, I just got rid of Database Cleaner from my feature specs and everything is working great (very nice). So would you recommend I stop there or should I convert these feature specs over to system specs?

Reading between the lines it seems your suggestion would be to leave feature specs as is (why change something that does not need changing)? Hence, I assume RSpec will respect feature specs for the long-term? Not deprecate as an old and not-modern way.

Many thanks.

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 3, 2017

@bluz71 Correct, I don't really see any gains worth the effort of moving existing feature tests over to system tests, especially if tests are written using things like the feature syntactic sugar. I haven't read about any plans to deprecate feature specs, but I can't really say what the future plans around that are.

twalpole commented Oct 3, 2017

@bluz71 Correct, I don't really see any gains worth the effort of moving existing feature tests over to system tests, especially if tests are written using things like the feature syntactic sugar. I haven't read about any plans to deprecate feature specs, but I can't really say what the future plans around that are.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Oct 3, 2017

That is, it's a Rails decision, not an rspec decision. Rspec will support em as long as Rails does. If I understand @twalpole's answer correctly.

jrochkind commented Oct 3, 2017

That is, it's a Rails decision, not an rspec decision. Rspec will support em as long as Rails does. If I understand @twalpole's answer correctly.

@sunnyrjuneja

This comment has been minimized.

Show comment
Hide comment
@sunnyrjuneja

sunnyrjuneja Oct 10, 2017

Hey everyone,

Just wanted to say, THANKS for all the awesome work! Super excited to be system specs inside of RSpec.

In case you were looking for any fodder for that blog post, I would love an explanation of the implementation difference between a feature and system spec. I'm weighing on if I should bother converting feature specs to system specs.

sunnyrjuneja commented Oct 10, 2017

Hey everyone,

Just wanted to say, THANKS for all the awesome work! Super excited to be system specs inside of RSpec.

In case you were looking for any fodder for that blog post, I would love an explanation of the implementation difference between a feature and system spec. I'm weighing on if I should bother converting feature specs to system specs.

@moveson

This comment has been minimized.

Show comment
Hide comment
@moveson

moveson Oct 19, 2017

I'm having trouble with selenium-driven tests not seeing records in the database. Consider the following:

RSpec.describe 'User logs in' do
  let!(:user) { create(:user, email: email, password: password, password_confirmation: password) }
  let(:email) { 'jane@example.com' }
  let(:password) { '12345678' }

  scenario 'with valid email and password' do
    visit new_user_session_path
    fill_in 'Email', with: email
    fill_in 'Password', with: password
    click_button 'Sign in'
    expect(page).to have_content('You are signed in.')
  end
end

This test passes when RSpec.configure is set to driven_by :rack_test, but it results in "Invalid email or password" and test failure when driven_by is set to :selenium or any other selenium-based driver.

Using Rails 5.1.4, RSpec 3.7.0, selenium-webdriver 3.6 and capybara 2.15.4, and config.use_transactional_fixtures = true.

moveson commented Oct 19, 2017

I'm having trouble with selenium-driven tests not seeing records in the database. Consider the following:

RSpec.describe 'User logs in' do
  let!(:user) { create(:user, email: email, password: password, password_confirmation: password) }
  let(:email) { 'jane@example.com' }
  let(:password) { '12345678' }

  scenario 'with valid email and password' do
    visit new_user_session_path
    fill_in 'Email', with: email
    fill_in 'Password', with: password
    click_button 'Sign in'
    expect(page).to have_content('You are signed in.')
  end
end

This test passes when RSpec.configure is set to driven_by :rack_test, but it results in "Invalid email or password" and test failure when driven_by is set to :selenium or any other selenium-based driver.

Using Rails 5.1.4, RSpec 3.7.0, selenium-webdriver 3.6 and capybara 2.15.4, and config.use_transactional_fixtures = true.

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 19, 2017

@moveson Prior to Rails 5.1 that would have been caused by running transactional testing, and not having database_cleaner installed. In Rails 5.1 however the DB connection is shared between threads in testing mode, thereby allowing transaction mode (and obviating the need for database_cleaner in most cases). Since you're using 5.1 your issue would tend to imply whatever server you're using with Capybara isn't running in process (so the DB connection can be shared). What server are you using? If puma, check the output produced for something like "clustered mode" and fix it to run in single mode.

twalpole commented Oct 19, 2017

@moveson Prior to Rails 5.1 that would have been caused by running transactional testing, and not having database_cleaner installed. In Rails 5.1 however the DB connection is shared between threads in testing mode, thereby allowing transaction mode (and obviating the need for database_cleaner in most cases). Since you're using 5.1 your issue would tend to imply whatever server you're using with Capybara isn't running in process (so the DB connection can be shared). What server are you using? If puma, check the output produced for something like "clustered mode" and fix it to run in single mode.

@moveson

This comment has been minimized.

Show comment
Hide comment
@moveson

moveson Oct 19, 2017

@twalpole I'm using puma in dev and production. I don't see the familiar puma output on startup in my RSpec log. How can I tell what server is running in the test environment? In any case, to be safe, I changed my config/puma.rb to workers 1 and threads 1,1, but it did not fix the problem.

moveson commented Oct 19, 2017

@twalpole I'm using puma in dev and production. I don't see the familiar puma output on startup in my RSpec log. How can I tell what server is running in the test environment? In any case, to be safe, I changed my config/puma.rb to workers 1 and threads 1,1, but it did not fix the problem.

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 19, 2017

@moveson You actually need workers 0 to ensure in process mode - there's a PR that was accepted into the next Rails 5.1 release to try and ensure it gets set that way - rails/rails@bc02a01 . You should be able to get the Puma output back by setting ActionDispatch::SystemTesting::Server.silence_puma = false - Unfortunately it was chosen to make silence the default in rspec-rails (unlike Rails), rather than letting experienced users opt into silence, which hides the issue with puma.

twalpole commented Oct 19, 2017

@moveson You actually need workers 0 to ensure in process mode - there's a PR that was accepted into the next Rails 5.1 release to try and ensure it gets set that way - rails/rails@bc02a01 . You should be able to get the Puma output back by setting ActionDispatch::SystemTesting::Server.silence_puma = false - Unfortunately it was chosen to make silence the default in rspec-rails (unlike Rails), rather than letting experienced users opt into silence, which hides the issue with puma.

@moveson

This comment has been minimized.

Show comment
Hide comment
@moveson

moveson Oct 19, 2017

@twalpole Setting workers to 0 and threads to 0,1 did the trick. Thank you! Do you happen to know an elegant way to set these values in the test environment without affecting my settings in dev/prod?

moveson commented Oct 19, 2017

@twalpole Setting workers to 0 and threads to 0,1 did the trick. Thank you! Do you happen to know an elegant way to set these values in the test environment without affecting my settings in dev/prod?

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 19, 2017

@moveson - You could monkey patch in the rails change I linked above (or run from the 5-1-stable branch), or configure a different config file for puma --- Puma reads it's config from a number of possible places, try config/puma/test.rb IIRC

twalpole commented Oct 19, 2017

@moveson - You could monkey patch in the rails change I linked above (or run from the 5-1-stable branch), or configure a different config file for puma --- Puma reads it's config from a number of possible places, try config/puma/test.rb IIRC

@moveson

This comment has been minimized.

Show comment
Hide comment
@moveson

moveson Oct 19, 2017

Got it. Again, thanks!

moveson commented Oct 19, 2017

Got it. Again, thanks!

@aptituz

This comment has been minimized.

Show comment
Hide comment
@aptituz

aptituz Oct 21, 2017

I'm currently having some trouble get this running with a dockerized application (while the same application works fine without docker involved).

In the docker case the tests are driven_by a selenium remote driver, with a chrome standalone docker container. So far the interaction between the two containers (one with the web application, one with the browser) works (as I can tell from the created screenshots), except for stubbing of authentication (the session is just not logged in via login_as).

I already tried adjusting puma settings in the docker container (worked fine for the non-docker-case), but the symptoms of the problem are quiet different (with more then one threads the authentication problem occurs in a number of cases only, in this case it fails on every try).

Anybody an idea? Could it be that the in other places suggested disabling of self.use_transactional_fixturesis still necessary in my use case and that I would need database cleaner?

aptituz commented Oct 21, 2017

I'm currently having some trouble get this running with a dockerized application (while the same application works fine without docker involved).

In the docker case the tests are driven_by a selenium remote driver, with a chrome standalone docker container. So far the interaction between the two containers (one with the web application, one with the browser) works (as I can tell from the created screenshots), except for stubbing of authentication (the session is just not logged in via login_as).

I already tried adjusting puma settings in the docker container (worked fine for the non-docker-case), but the symptoms of the problem are quiet different (with more then one threads the authentication problem occurs in a number of cases only, in this case it fails on every try).

Anybody an idea? Could it be that the in other places suggested disabling of self.use_transactional_fixturesis still necessary in my use case and that I would need database cleaner?

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Oct 21, 2017

@aptituz You don't fully define your use case, nor what login_as you're referring to. The Warden provided login_as (Devise), Rails 5.1 DB connection sharing (transactional testing, no need for database_cleaner), and Capybara's request completion detection all depend on the tests and the application under test running in the same process (multiple threads, but 1 process).

If you're referring to that login_as and are trying to run your AUT in a docker instance but run the tests on your local host - then you lose all that, will have to log in manually by filling in the fields everytime (or write your own login shortcut that depends only on parameters passed in the request), will need to use database_cleaner, and will need to be very careful about any extra requests from your app which your tests don't specifically wait to finish.

If, instead, you are running the tests on the docker instance and letting Capybara start the AUT on the docker instance all in one process (but the browser is in a different docker instance) then you should be fine, and probably just have a configuration issue.

twalpole commented Oct 21, 2017

@aptituz You don't fully define your use case, nor what login_as you're referring to. The Warden provided login_as (Devise), Rails 5.1 DB connection sharing (transactional testing, no need for database_cleaner), and Capybara's request completion detection all depend on the tests and the application under test running in the same process (multiple threads, but 1 process).

If you're referring to that login_as and are trying to run your AUT in a docker instance but run the tests on your local host - then you lose all that, will have to log in manually by filling in the fields everytime (or write your own login shortcut that depends only on parameters passed in the request), will need to use database_cleaner, and will need to be very careful about any extra requests from your app which your tests don't specifically wait to finish.

If, instead, you are running the tests on the docker instance and letting Capybara start the AUT on the docker instance all in one process (but the browser is in a different docker instance) then you should be fine, and probably just have a configuration issue.

@aptituz

This comment has been minimized.

Show comment
Hide comment
@aptituz

aptituz Oct 21, 2017

@twalpole You are right. Thanks that you ask for clarification.

I'm referring to the Warden provided login_as method and only the browser is in a different docker instance. The application itself and the tests are in the same container, but ... well ... it could be that the tests ran against a previously started instance, I'll have to check that. According to your explanations this could be the culprit, so thanks: these hints are helpful to me.

aptituz commented Oct 21, 2017

@twalpole You are right. Thanks that you ask for clarification.

I'm referring to the Warden provided login_as method and only the browser is in a different docker instance. The application itself and the tests are in the same container, but ... well ... it could be that the tests ran against a previously started instance, I'll have to check that. According to your explanations this could be the culprit, so thanks: these hints are helpful to me.

@aptituz

This comment has been minimized.

Show comment
Hide comment
@aptituz

aptituz Oct 23, 2017

@twalpole

I just wanted to drop a note that I got it running eventually. So, thanks again for your help.

For some reasons I don't remember I had a Capybara. run_server = false line around. So yeah, the tests were indeed run again a different, accidentally running container. After removing that setting it was basically just a host! "http://#{ip}:#{Capybara.server_port}" in a before(:each) to let capybara actually use the AUT containers external address for the tests instead of 127.0.0.1 as per default.

aptituz commented Oct 23, 2017

@twalpole

I just wanted to drop a note that I got it running eventually. So, thanks again for your help.

For some reasons I don't remember I had a Capybara. run_server = false line around. So yeah, the tests were indeed run again a different, accidentally running container. After removing that setting it was basically just a host! "http://#{ip}:#{Capybara.server_port}" in a before(:each) to let capybara actually use the AUT containers external address for the tests instead of 127.0.0.1 as per default.

@Xosmond

This comment has been minimized.

Show comment
Hide comment
@Xosmond

Xosmond Nov 23, 2017

I have a problem with system tests:

RSpec.describe "Authentification", type: :system, js: true do
  let(:valid_attributes) { attributes_for(:user) }
  before :each do
    @user = User.create!(valid_attributes)
    visit '/users/login'
  end
    it "Enable second factor auth" do
      sign_in_with(valid_attributes[:email], valid_attributes[:password])
      @user.update! direct_otp: "111111" # on js: true, the user is not updated on the browser database
      activate_second_factor(@user.role.name, @user.direct_otp)
      expect(page).to have_content 'Se ha activado correctamente el segundo factor de autentificación.'
    end
end

This example only works when javascript is disabled, when I add js: true to the test, I get an error because the direct_otp field its not correct because user is not updated on the browser database.

Any suggestion on why this is happening?
Because user is created via code and I can login but late, on the middle of the test, when I update the user, the changes are not shared.

Rails 5.1.4
Rspec-rails 3.7.2

Xosmond commented Nov 23, 2017

I have a problem with system tests:

RSpec.describe "Authentification", type: :system, js: true do
  let(:valid_attributes) { attributes_for(:user) }
  before :each do
    @user = User.create!(valid_attributes)
    visit '/users/login'
  end
    it "Enable second factor auth" do
      sign_in_with(valid_attributes[:email], valid_attributes[:password])
      @user.update! direct_otp: "111111" # on js: true, the user is not updated on the browser database
      activate_second_factor(@user.role.name, @user.direct_otp)
      expect(page).to have_content 'Se ha activado correctamente el segundo factor de autentificación.'
    end
end

This example only works when javascript is disabled, when I add js: true to the test, I get an error because the direct_otp field its not correct because user is not updated on the browser database.

Any suggestion on why this is happening?
Because user is created via code and I can login but late, on the middle of the test, when I update the user, the changes are not shared.

Rails 5.1.4
Rspec-rails 3.7.2

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Nov 23, 2017

@Xosmond Assuming you're using Rails 5.1+, if the app code isn't seeing DB changes made in the test code it's usually that you have the app running in a separate process rather than separate thread. Turn off the silencing of puma that rspec-rails does by default (it's a bad default option for beginners trying to troubleshoot) and see if the output puma then generates tells you it's running in 'single mode' (as opposed to cluster mode). If it doesn't you need to check where/how your puma is getting it's config from and make sure it's set to run with 0 processes and some number >= 1 threads.

twalpole commented Nov 23, 2017

@Xosmond Assuming you're using Rails 5.1+, if the app code isn't seeing DB changes made in the test code it's usually that you have the app running in a separate process rather than separate thread. Turn off the silencing of puma that rspec-rails does by default (it's a bad default option for beginners trying to troubleshoot) and see if the output puma then generates tells you it's running in 'single mode' (as opposed to cluster mode). If it doesn't you need to check where/how your puma is getting it's config from and make sure it's set to run with 0 processes and some number >= 1 threads.

@Xosmond

This comment has been minimized.

Show comment
Hide comment
@Xosmond

Xosmond Nov 23, 2017

@twalpole I have turn off the silencing and I got:

Puma starting in single mode...
* Version 3.11.0 (ruby 2.3.5-p376), codename: Love Song
* Min threads: 0, max threads: 1
* Environment: test
* Listening on tcp://0.0.0.0:9887
Use Ctrl-C to stop

Thats why I can login with the user created via code, but the app is not seeing the later change (user.update!) done by the test code on the it macro.

But if I do that change on the before macro , it works:

RSpec.describe "Authentification", type: :system, js: true do
  let(:valid_attributes) { attributes_for(:user) }
  before :each do
    @user = User.create!(valid_attributes)
    @user.update! direct_otp: "111111"
    visit '/users/login'
  end
    it "Enable second factor auth" do
      sign_in_with(valid_attributes[:email], valid_attributes[:password])
      activate_second_factor(@user.role.name, @user.direct_otp)
      expect(page).to have_content 'Se ha activado correctamente el segundo factor de autentificación.'
    end
end

Something happens after the before macro.

Xosmond commented Nov 23, 2017

@twalpole I have turn off the silencing and I got:

Puma starting in single mode...
* Version 3.11.0 (ruby 2.3.5-p376), codename: Love Song
* Min threads: 0, max threads: 1
* Environment: test
* Listening on tcp://0.0.0.0:9887
Use Ctrl-C to stop

Thats why I can login with the user created via code, but the app is not seeing the later change (user.update!) done by the test code on the it macro.

But if I do that change on the before macro , it works:

RSpec.describe "Authentification", type: :system, js: true do
  let(:valid_attributes) { attributes_for(:user) }
  before :each do
    @user = User.create!(valid_attributes)
    @user.update! direct_otp: "111111"
    visit '/users/login'
  end
    it "Enable second factor auth" do
      sign_in_with(valid_attributes[:email], valid_attributes[:password])
      activate_second_factor(@user.role.name, @user.direct_otp)
      expect(page).to have_content 'Se ha activado correctamente el segundo factor de autentificación.'
    end
end

Something happens after the before macro.

@twalpole

This comment has been minimized.

Show comment
Hide comment
@twalpole

twalpole Nov 23, 2017

@Xosmond Ok, since it is running in single mode it's probably a timing issue with the way your tests are written (or possible you aren't actually running in transaction mode). This really isn't the right place for that discussion. You should post over on stack overflow, add a capybara tag to the question and provide details of exactly what your sign_in_with and activate_second_factor methods do.

twalpole commented Nov 23, 2017

@Xosmond Ok, since it is running in single mode it's probably a timing issue with the way your tests are written (or possible you aren't actually running in transaction mode). This really isn't the right place for that discussion. You should post over on stack overflow, add a capybara tag to the question and provide details of exactly what your sign_in_with and activate_second_factor methods do.

@Xosmond

This comment has been minimized.

Show comment
Hide comment
@Xosmond

Xosmond Nov 24, 2017

@twalpole I'm using config.use_transactional_fixtures = true and I have tried to sleep 5 it and nothing works. Oh I will. Same, thanks.

Xosmond commented Nov 24, 2017

@twalpole I'm using config.use_transactional_fixtures = true and I have tried to sleep 5 it and nothing works. Oh I will. Same, thanks.

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Mar 27, 2018

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Mar 27, 2018

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813

jcoyne added a commit to samvera-labs/hyku that referenced this pull request Mar 27, 2018

Switch from Poltergeist to headless Chrome
Poltergeist is no longer maintained
Rails 5.1 introduces ActionDispatch::SystemTestCase which should allow
our feature tests to be speed up when this is done:
rspec/rspec-rails#1813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment