-
-
Notifications
You must be signed in to change notification settings - Fork 327
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
Rails transactional database cleanup #531
Comments
Technically, the thread-safe Rails transactional cleanup has been there since Rails 5.1, but there was a bug in the mysql connection adapter code that wasn't thread-safe, which was fixed in 6.0. However, I made a patch for that mysql issue, based on the 6.0 fix, so I've been using that workaround code since Rails 5.1; I removed the mysql patch after updating Rails to 6.0. The above workaround still works, even in Rails 7.0 using the cucumber-rails code from git now. In case anyone was curious, here's that mysql patch: # frozen_string_literal: true
# Backport fix for https://github.com/rails/rails/issues/34798 from Rails 6.
if Rails::VERSION::MAJOR < 6
# This file is safe to require ahead of time for Rails 5, but not for 6+.
require 'active_record/connection_adapters/mysql/database_statements'
module ActiveRecord
module ConnectionAdapters
module MySQL
module DatabaseStatements
def exec_delete(sql, name = nil, binds = [])
if without_prepared_statement?(binds)
@lock.synchronize do
execute_and_free(sql, name) { @connection.affected_rows }
end
else
exec_stmt_and_free(sql, name, binds) { |stmt| stmt.affected_rows }
end
end
alias :exec_update :exec_delete
end
end
end
end
end |
Lot to digest here. I'll try isolate my thoughts per the bullet points in time. |
Re-reading parts of your thread I can comment on the following
|
I think I've figured out how to write the tests with the existing matrix, and a slight tweak of the existing test setup code, just adding a couple tests to the existing suite. The tricky part has been setting aside the time, as I'm dealing with my own outside issues. I'll be able to do more soon, like in a couple weeks. Still not requesting that anyone else do this. Just looking for advice, particularly about the stuff that was in question or alternatives above. |
🤔 What's the problem you're trying to solve?
Rails 5.1+ added code that its test framework hooks into that makes transactional database cleanup thread-safe (there were bugs in the mysql adapter that were fixed in 6.0, but those are patchable). DatabaseCleaner style transactional cleanup is not thread-safe, even with the shared_connection hook that cucumber-rails uses already. However, ActionDispatch::IntegrationTest has fixture support and lifecycle hooks that Rails hooks into, which does let you do transactional database cleanup in a thread-safe way, and World inherits from ActionDispatch::IntegrationTest, so those lifecycle hooks are available as long as the World runtime instance is accessible.
Another advantage of using this solution would be to make it possible to not refer to the database_cleaner gem at all when you aren't using one of the database strategies that doesn't require it, including the null strategy and the rails transactional strategy.
The big problem is that the World instance is not made available to the Strategy instance. I came up with a workaround for this, which I will show below. However, the World instance is available in the Before and After hook blocks, so it's possible to make it available to the Strategy instance.
✨ What's your proposed solution?
First of all, it's worth emphasizing that I am not requesting that other people do the work. I am just making the request here so I have a place to ask for advice.
This has multiple changes, but we should be able to make them while supporting backwards-compatible behavior:
default_strategy!
until you need to use the strategy for the first time, if it hasn't been defined yet. This will prevent the:truncation
strategy code, along with DatabaseCleaner, from being loaded if it's not needed.ActiveRecord::Base
shared_connection
patch code from lib/cucumber/rails/hooks/active_record.rb to lib/cucumber/rails/database/shared_connection_strategy.rb, so it doesn't get loaded when it doesn't need to be.require 'cucumber/rails/hooks/database_cleaner'
from lib/cucumber/rails/hooks.rb to the just the strategy files that need to use DatabaseCleaner at all. If NullStrategy or Rails transactional cleanup is used, we shouldn't even attempt to require DatabaseCleaner and shouldn't even log an error if the gem isn't installed.Cucumber::Rails::Database
before_js
,before_non_js
, andafter
all take ascenario
parameter.Before
andAfter
hooks in lib/cucumber/rails/hooks/active_record.rb, pass inself
as a parameter to the calls to thosebefore_js
,before_non_js
, andafter
methods.Now at this point we have two alternatives.
In the first:
Cucumber::Rails::Database
before_js
andbefore_non_js
methods, callscenario.setup_fixtures
before calling the@strategy
before_js
orbefore_non_js
methods, and callscenario.teardown_fixtures
before the@strategy
after
method.:transactional
, using the old somewhat but not really thread-safe integration code.In the second:
Cucumber::Rails::Database
before_js
,before_non_js
, andafter
methods, check the arity of the corresponding methods of the@strategy
value and pass in the scenario value as a parameter if the arity isn't 0.RailsStrategy
that callsscenario.setup_fixtures
inbefore_js
andbefore_non_js
, and callsscenario.teardown_fixtures
inafter
.:transactional
, useRailsStrategy
for Rails 5.1+, andSharedConnectionStrategy
for earlier versions.⛏Have you considered any alternatives or workarounds?
This works (I put it in features/support/database.rb):
I've been using this solution for 2 years. It works, and is as thread-safe as Rails transactional cleanup in rspec, test::unit, minitest, etc.
Additional context
This means that World would need to continue to inherit from ActionDispatch::IntegrationTest, which means that #505 would need to either be significantly changed or canceled altogether.
The biggest challenge I've run into on this is figuring out how to phrase a test in your test suite that would verify that DatabaseCleaner has not been loaded or even required, since that's one of the main goals of these changes, without breaking any other tests of the strategies that use DatabaseCleaner. That's going to need some thought, which I haven't had the time for until now. I'm hoping that your feature tests can execute an external instance of cucumber with different settings, so I can verify the effect of those settings. I would have done this ages ago if it wasn't for that issue.
I'd like to try to make this a less-breaking change, so it can get in as soon as possible, preferably in version 2.5. It would be a disservice to put this off until a cucumber-rails 3.x version, unless you have such a version planned in the near future.
The text was updated successfully, but these errors were encountered: