Rails 5.1 compatibility

[samstickland]

Hi,

Has anyone tried this with Rails 5.1, where the "system" tests automatically share the database connection as the backend?

I've started getting some hangs on my capybara tests. I'm yet to dig into them properly, but I'm curious to know if other people have had success in this front.

[rosenfeld]

Hi, this is not related to Rails 5.1, but with Capybara and how most of its drivers work. They will usually spawn a new process to serve the application which would then be accessed by the Capybara drivers.

The problem is that two processes can't share the same database connection, thus the transaction rollback strategy doesn't work.

Usually we tag such tests with :features, for example, and use another approach for those examples, such as DatabaseCleaner or directly truncating tables affected by the tests and use the transaction rollback faster approach for the other examples.

The reason why it hangs is due to some database deadlocks because the example started a transaction, acquired some lock, then the other process serving the app will wait forever for that lock to be released.

[samstickland]

Hi,

Thanks the reply. This behaviour has been changed in Rails 5.1 - database connections are now shared and DatabaseCleaner and other database cleaner strategies are no longer required.

Here's the PR: https://github.com/rails/rails/pull/28083

Ensure test threads share a DB connection

This ensures multiple threads inside a transactional test to see consistent database state.

When a system test starts Puma spins up one thread and Capybara spins up another thread. Because of this when tests are run the database cannot see what was inserted into the database on teardown. This is because there are two threads using two different connections.

This change uses the statement cache to lock the threads to using a single connection ID instead of each not being able to see each other. This code only runs in the fixture setup and teardown so it does not affect real production databases.

When a transaction is opened we set lock_thread to Thread.current so we can keep track of which connection the thread is using. When we rollback the transaction we unlock the thread and then there will be no left-over data in the database because the transaction will roll back the correct connections.

This thread https://github.com/rspec/rspec-rails/issues/1808 on rspec-rails describes how people have been able to make it work with RSpec simply by removing DatabaseCleaner and setting config.use_transactional_fixtures = true

This also works for me. However if I run with the following code:

    # New nested transaction for every single example or group
    config.nested_transaction do |example_or_group, run|
      ActiveRecord::Base.transaction(requires_new: true) do
        run[]
        raise ActiveRecord::Rollback
      end
    end

then my capybara test hang indefinitely.

This configuration works, but means, of course, that I can't use the nested transactions in my Capybara tests.. Which honestly, isn't the great loss ever, since we weren't using them before, but it would be nice to have.

    # New nested transaction for every single example or group
    config.nested_transaction do |example_or_group, run|
      (run[]; next) if example_or_group.metadata[:type] == :feature

      ActiveRecord::Base.transaction(requires_new: true) do
        run[]
        raise ActiveRecord::Rollback
      end
    end

[rosenfeld]

I agree it would be nice and if it is possible I'll certainly want this feature, I just don't know how to implement it yet. I mean, I'm no longer using Rails, but Roda instead, but if Rails is able to serve Puma in the same process as the tests then it should be certainly possible to replace DatabaseCleaner with transactions and I'm certainly interested on this subject. A while back I almost created a ticket in Puma to ask them to allow that but then I thought they wouldn't accept and ended up not filling that ticket. But if Rails was able to do that, then it should be possible. Different threads is not a problem since you can share the same connection among multiple threads, my concern is because I thought Puma would start in a new process instead.

That's the reason why I use rack-test in my project instead of rack_toolkit. I'd much prefer rack_toolkit but I'll only use it if I can make Puma requests share the same connection.

Unfortunately, I don't have time today to look into this (I don't have time to do what I'm supposed to do actually). As soon as I find some free time I'll certainly investigate this and see if it's indeed possible to make Puma share the same process as the tests. If you want to be sure you can print Process.pid from your test and from the controller in your feature tests just to make sure they are indeed the same.

I'll keep this open for discussion purposes, but there's nothing that could be done on this gem to make it support Rails 5.1 as far as I can tell, but the discussion is interesting as long as it's indeed a possibility.

[samstickland]

So, I have my test suite running without DatabaseCleaner now, on Rails 5.1. But if I try to wrap the capybara feature tests in a nested transaction the tests just hang.. My guess would be that something in the nested transactions is interfering with the connection sharing code. I'll let you know if I find anything.

S

2017-05-24 14:35 GMT+01:00 Rodrigo Rosenfeld Rosas <notifications@github.com

:

Reopened #2 https://github.com/rosenfeld/rspec_nested_transactions/issues/2.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rosenfeld/rspec_nested_transactions/issues/2#event-1095586024, or mute the thread https://github.com/notifications/unsubscribe-auth/AJfZU-RXiwDh6JKiDdUglEV4d1a3xkhAks5r9DINgaJpZM4NkYeu .

[rosenfeld]

Please try to print the process id both from the tests and from your controller before it hangs just to make sure they are both the same process.

[rosenfeld]

I just found some time to play with this until I'm requested for a meeting. I could just confirm in an interactive session that it's possible to run Puma in the same process, which means it should be possible to share the database connection, thus using transactions rollbacks to clean-up data modified by tests. I'll give it a try with Sequel first, since I'm more used to it and once I confirm it works, I'll see what's the current situation with ActiveRecord...

[rosenfeld]

Actually only today I got a few hours to try this and I could make it work by providing a custom connection pool implementation to Sequel that would allow a connection to be shared among separate threads, which might be suitable when running tests, but it's not something I would use in production :)

I tested it standalone so far but I bet it works with rspec_nested_transactions which I'm going to confirm in a few minutes. After I confirm it works, I'll try to understand how this works with the current ActiveRecord release.

[samstickland]

OK - thanks for having the time to look into this! :)

2017-05-25 18:31 GMT+01:00 Rodrigo Rosenfeld Rosas <notifications@github.com

:

Actually only today I got a few hours to try this and I could make it work by providing a custom connection pool implementation to Sequel that would allow a connection to be shared among separate threads, which might be suitable when running tests, but it's not something I would use in production :)

I tested it standalone so far but I bet it works with rspec_nested_transactions which I'm going to confirm in a few minutes. After I confirm it works, I'll try to understand how this works with the current ActiveRecord release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rosenfeld/rspec_nested_transactions/issues/2#issuecomment-304071594, or mute the thread https://github.com/notifications/unsubscribe-auth/AJfZU_K5_boi7mK6OLwh1vRqMJSEBkshks5r9brmgaJpZM4NkYeu .

[rosenfeld]

Ok, after experimenting a bit with it, I noticed that it works fine with rspec_nested_transactions, however if your application has code to connect to the database in a separate thread, I think it could lead to hard-to-debug issues if two threads try to use the same connection at the same time, which would be required for the transaction rollback strategy to work. That means there are many cases where this could go bad. In my application, I run several queries in parallel for an expensive requests, so I would have to switch this behavior off for examples testing that action. It wouldn't also work with tools to parallelize test execution for example. Or if you have some background job running in a separate thread rather than through some queue like Sidekiq. So, this is not something I would count on in a real application, but it's an interesting experiment. I'll try to look at ActiveRecord implementation to see whether or not it allows one to share the same transaction among different threads and how it handles concurrent access to the same connection...

[rosenfeld]

I don't really know how ActiveRecord is supposed to work, but it's easy to reproduce with rails console:

ActiveRecord::Base.connection_pool.lock_thread= true

running = true
def select_one
  ActiveRecord::Base.connection.execute 'select 1'
end

Thread.start{ (select_one; sleep 2) while running }
select_one # returns as expected
running = false
sleep 3
running = true
# now with a transaction:
Thread.start{ ActiveRecord::Base.transaction { (select_one; sleep 2) while running } }
select_one # now it hangs

So, I'm not sure how this is supposed to work with ActiveRecord when transactions are involved...

[xtagon]

Sorry to creep in on a closed issue, but just to clarify, does this mean the gem isn't compatible with 5.1 and probably never will be? I'm evaluating using it in a Rails 5.1 project. Thanks.

[rosenfeld]

This has nothing to do with Rails 5.1 actually. This project has never worked with Capybara whatever Rails version you were using. The problem is that when you use something like Capybara using some driver other than :rack_test, Capybara will spawn a new process/thread which prevents the connection from being shared between your tests and the Capybara server process/thread. I was able to use an special connection pool that would allow such configuration to work with Sequel, but I don't know how to make it work with ActiveRecord. It seems ActiveRecord::Base.connection_pool.lock_thread = true was introduced in Rails 5.1 to allow some database transaction to work across separate threads by sharing the same connection, but as I demonstrate in the snippet above, it doesn't seem to work.

So, unless you find a way to make it work with ActiveRecord, it won't be possible to use this gem with integration tests that require spawning a separate server in another thread, like the approach used by default in Rails 5.1 controller and integration tests. I don't think this approach is a good idea anyway, even for Sequel, as you'd be testing a different scenario than what you would have seen in production.

So, while your tests don't spawn a separate server, like model tests and other tests not exercising the controllers, you should be able to use rspec_nested_transactions to use the faster rollback strategy. But as long as you are going through the Capybara server, like testing controllers, than you should use a separate strategy.

I'll usually tag my tests and choose which strategy to adopt depending on the test tag. For integration tests I use a table truncate strategy rather than wrapping inside a transaction and rolling it back.

[xtagon]

Hi @rosenfeld, thank you so much for the detailed explaination. I appreciate you taking the time to clarify it for me 👍

[rosenfeld]

No problem :)