Use the default Capybara registered puma server configuration

[twalpole]

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]

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]

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]

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]

@twalpole can you add a changelog note for this?

[twalpole]

@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]

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

[guilleiguaran]

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]

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

[guilleiguaran]

@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]

@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]

@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]

@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]

@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]

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]

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?.