Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the default Capybara registered puma server configuration #30638

Merged
merged 1 commit into from Sep 18, 2017

Conversation

@twalpole
Copy link
Contributor

twalpole commented Sep 17, 2017

The puma_rails "server" registered by Rails for system tests doesn't force Puma into "in process" mode. This means if the user happens to have a config/puma.rb configuration file which is configuring puma in cluster mode for production it will also run Puma in cluster mode for tests. Since cluster mode runs Puma in new processes it defeats the connection sharing needed for transactional testing, and prevents Capybara from being able to monitor open connections at the end of tests.

This PR changes the behavior to just use the default Capybara registered puma "server" which does pass the needed options, and I don't really see the benefit of Rails registering its own "server" config.

@rails-bot
Copy link

rails-bot commented Sep 17, 2017

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

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

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

Please see the contribution instructions for more information.

@maschwenk
Copy link
Contributor

maschwenk commented Sep 18, 2017

This issue directly impacted me, and will probably impact other Rails users as more people migrate to SystemTests. Like most people running Puma in the dev/prod stack, I had some configuration in config/puma.rb that was overriding the SystemTests-configured Puma. It took a long time to figure out what the cause was until I directly messaged @twalpole over Gitter.

@eileencodes in the evolution of the PR I saw you placed a big emphasis on sensible defaults and out-of-the-box functionality. To that point, it makes sense to me that SystemTests should strongly enforce a Puma configuration that takes advantage of the connection sharing (but maybe leave the port to be configurable? not sure if that will make a huge difference)

@eileencodes
Copy link
Member

eileencodes commented Sep 18, 2017

I think this is fine because I originally meant for the port to be settable but now that I think about it that eventually got removed with a few refactorings. If we need the port to be configurable beyond the config/puma.rb it makes sense to add that feature to system tests - but we don't need to delay this PR for that.

@eileencodes
Copy link
Member

eileencodes commented Sep 18, 2017

@twalpole can you add a changelog note for this?

@eileencodes eileencodes added this to the 5.1.5 milestone Sep 18, 2017
@twalpole twalpole force-pushed the twalpole:capybara_server branch to ba04b58 Sep 18, 2017
@twalpole
Copy link
Contributor Author

twalpole commented Sep 18, 2017

@eileencodes Done - Also, I notice you tagged this 5.1.5 -- It does require an update to Capybara 2.15+ for the passing of the 'Silent' option to Capybaras registered puma server to work - so not sure whether backporting is acceptable or not.

@eileencodes eileencodes merged commit 42d09f6 into rails:master Sep 18, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eileencodes eileencodes removed this from the 5.1.5 milestone Sep 18, 2017
@eileencodes
Copy link
Member

eileencodes commented Sep 18, 2017

@twalpole thanks for pointing that out, I've removed it from the 5.1.5 milestone.

@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 3, 2017

Isn't possible to backport this to 5.1.x? This allow using another web servers without changing puma as the default, e.g in my app I can do something like:

config.before(:suite)   { Capybara.server = :passenger }
@maschwenk
Copy link
Contributor

maschwenk commented Nov 3, 2017

@guilleiguaran if you look directly above in stream, we have already back-ported it in #30712

@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 3, 2017

@maschwenk even when #30712 makes registered :rails_puma match the behavior of Capybara registered puma it doesn't allow the use of alternatives servers since Puma Rack Handler is required and initialized in the code.

@twalpole
Copy link
Contributor Author

twalpole commented Nov 4, 2017

@guilleiguaran The fact that Rails registers a server shouldn't matter - you just need to make sure your setting of Capybara.server is executed after the Rails setting of it.

@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 4, 2017

@twalpole: but Rails fails if puma isn't present in the Gemfile, so you need to add it as a dependency even if you want to use another server. See rspec/rspec-rails#1882

The current solution in RSpec 3.7 is to disable System Tests entirely if puma can't be loaded: https://github.com/rspec/rspec-rails/pull/1884/files

@twalpole
Copy link
Contributor Author

twalpole commented Nov 4, 2017

@guilleiguaran So include the puma gem in the test environment, registering the server doesn't actually initialize puma until that server is used, so as long as you set an alternative before Capybara attempts to start the server puma will never be used - Note: this PR didn't remove the require "rack/handler/puma" line from the file so it would still require puma to be in the project.

@twalpole
Copy link
Contributor Author

twalpole commented Nov 4, 2017

@guilleiguaran Or just stick to writing feature specs which don't have a requirement of puma and still get the connection sharing/transactional testing/remove database_cleaner benefits under Rails 5.1

@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 4, 2017

Note: this PR didn't remove the require "rack/handler/puma" line from the file so it would still require puma to be in the project.

Yup, I already removed it in Rails master: 50f6976

still get the connection sharing/transactional testing/remove database_cleaner benefits under Rails 5.1

Didn't know about this, I think that's good enough for me.

@obromios
Copy link

obromios commented Apr 16, 2018

Is a possible temporary workaround for this to put a guard clause in config/puma.rb that skips the relevant puma cluster configuration code unless Rails.env.production?.

@betelgeuse

This comment has been minimized.

@twalpole in Capybara the default when this change was done was 0:4 ( teamcapybara/capybara@91dcc7f ) and still is. To me it seems that 0:4 is fundamentally incompatible with the system spec connection sharing between threads so was the increase in the default thread count caused by this change intentional? If not, should the default be changed via Rails or Capybara code?

This comment has been minimized.

Copy link
Contributor Author

twalpole replied Jul 8, 2020

What makes you think this is “fundamentally incompatible”? It’s been working correctly for thousands of people for 3 years

This comment has been minimized.

Copy link

betelgeuse replied Jul 8, 2020

If you do the following things will break:

  1. Assume the documented behaviour that during a request the connection is checked out to you
    # This will be the default behavior for Active Record when used in conjunction with
  2. Use PostgreSQL SET to change settings for the current connection

Debugging such a test suite failure is what eventually led me to find this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.