Rails transactional database cleanup

[BrianHawley]

🤔 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:

• Don't require the files in lib/cucumber/rails/database unless and until their strategies are chosen. Maybe autoload?
• Delay calling 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.
• Move all of the 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.
• Move the 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.
• Make Cucumber::Rails::Database before_js, before_non_js, and after all take a scenario parameter.
• In the Before and After hooks in lib/cucumber/rails/hooks/active_record.rb, pass in self as a parameter to the calls to those before_js, before_non_js, and after methods.

Now at this point we have two alternatives.

In the first:

• In the Cucumber::Rails::Database before_js and before_non_js methods, call scenario.setup_fixtures before calling the @strategy before_js or before_non_js methods, and call scenario.teardown_fixtures before the @strategy after method.
• If the Rails version is 5.1+, just default to telling Rails to use transactional fixtures, and use NullStrategy because it doesn't need to do anything more.
• If the Rails version is < 5.1, you can use SharedConnectionStrategy if someone picks :transactional, using the old somewhat but not really thread-safe integration code.
• This method is simpler and more thorough, but might be considered a breaking change. For one thing, it probably won't work with the below patch workaround installed because it will call the lifecycle methods more than once in the same lifecycle (at the moment, this only affects me). For another thing, I haven't determined whether there are occasions when you wouldn't want to call those lifecycle methods, so I'd have to study the code again; I think that they're basically noops if you don't have Rails transactional fixtures configured, but I need to check what else hooks into this.

In the second:

• In the Cucumber::Rails::Database before_js, before_non_js, and after 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.
• Have a RailsStrategy that calls scenario.setup_fixtures in before_js and before_non_js, and calls scenario.teardown_fixtures in after.
• If someone picks :transactional, use RailsStrategy for Rails 5.1+, and SharedConnectionStrategy for earlier versions.
• This method has the advantage of being compatible with the below patch, and not a breaking change. The disadvantage is that the lifecycle methods aren't called in other scenarios.

⛏Have you considered any alternatives or workarounds?

This works (I put it in features/support/database.rb):

# frozen_string_literal: true

module Cucumber
  module Rails
    module Database
      # Manage persistent state during the Cucumber run.
      class OurStrategy < Strategy
        # Cucumber creates a Cucumber::Runtime but doesn't expose a reference
        # to it. That runtime has a support_code, which has a registry, which
        # is the only thing with a reference to the World that is used as a
        # context for the tests. However, even though that test context is
        # based on ActionDispatch::IntegrationTest, Cucumber doesn't call any
        # of the lifecycle methods for that context object; unfortunately this
        # includes the setup and teardown of thread-safe database connections.
        # Cucumber's own shared connections aren't properly thread-safe, so we
        # would prefer to use the ones managed by Rails.
        #
        # However, even though Cucumber makes the test context available to
        # hooks, including the hooks that call the database cleanup code, it
        # doesn't pass a reference to that context to the database cleaner,
        # so we can't call the lifecycle methods from the database cleaner.
        # Because of this, we have to track the current_world with patches.

        # Thread-local copy of the current_world (test context):
        thread_cattr_accessor :current_world, instance_writer: false

        # Patch for the registry to update our copy of its current_world:
        module RegistryAndMorePatch
          def begin_scenario(test_case)
            super
            Cucumber::Rails::Database::OurStrategy.current_world = current_world
          end

          def end_scenario
            super
            Cucumber::Rails::Database::OurStrategy.current_world = current_world
          end
        end
        Cucumber::Glue::RegistryAndMore.prepend(RegistryAndMorePatch)

        # Our database cleaner lifecycle methods:

        def before_js
          # Rails prepares the ActiveRecord databases in setup_fixtures.
          current_world.setup_fixtures
        end

        def before_non_js
          # Rails prepares the ActiveRecord databases in setup_fixtures.
          current_world.setup_fixtures
        end

        def after
          # Rails cleans up the ActiveRecord databases in teardown_fixtures.
          current_world.teardown_fixtures

          # Clean up other resources
          ::Rails.cache.clear
          # ... other cleanups that I have been doing here, which are specific to our code.
        end
      end
    end
  end
end

# We're doing our own cleanup instead of disabling cleanup because Rails only cleans up the ActiveRecord databases.
Cucumber::Rails::Database.javascript_strategy = Cucumber::Rails::Database::OurStrategy

# cucumber-rails requires DatabaseCleaner even if it isn't being used, and hooks into it.
# So, we disable DatabaseCleaner with its NullStrategy.
DatabaseCleaner.strategy = DatabaseCleaner::NullStrategy.new

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.

[BrianHawley]

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

[luke-hill]

Lot to digest here. I'll try isolate my thoughts per the bullet points in time.

[luke-hill]

Re-reading parts of your thread I can comment on the following

• There is nothing stopping us making the next version 3.0 - We don't really have too much breaking logic to do, so it could be a "small" 3.0
• I definitely currently have a lot of geo-political stuff in work atm, so it would be easier if this came through 3-5 small PR's or issues where I can isolate the small change/s (If possible). I can / should get to your original issue, it just might take time. I've hardly contributed here in the last 3-4 months other than the release of 2.5 I committed to.
• We have amended our test / support matrix a bit recently, so I'd prefer not to amend it again soon. But again if we did do a 3.x release we could cut out a little more chaff (Next logical piece to remove would be ruby 2.5 support, which is hardly tested now anyways).

[BrianHawley]

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.