Compatibility with database_cleaner v2 adapter gems #467

botandrose · 2020-06-06T22:07:10Z

Summary

Hi folks! This PR aims to resolve some incompatibilities with cucumber-rails and the upcoming major v2 release of database_cleaner.

Details

In order to get access to the database_cleaner configuration in a ORM-agnostic way, we first try to require database_cleaner/core (which is only available in the database_cleaner v2 family of gems), then fall back to requiring database_cleaner for folks still on v1, finally falling back to silence if neither is available, since database_cleaner is an optional dependency.

Motivation and Context

Last week, we cut beta versions of the next major version of database_cleaner. The main change here is that all of the ORM adapters have been split off into their own gems, e.g. database_cleaner-active_record, database_cleaner-redis, etc. Understanding that the majority of database_cleaner's current users are using it with Rails/ActiveRecord (and this gem, too), we chose to make the database_cleaner v2.0.0 gem a metagem that depends on database_cleaner-active_record, and simply loads that when require "database_cleaner" is run. This means that the vast majority of our users won't have to touch a thing to upgrade from database_cleaner v1 to v2.

However, not everyone is only using ActiveRecord. There are those who can't get away with just leaving gem "database_cleaner" as-is in their Gemfile, and they have a bit more work to do. For example, let's say you're using Rails with Sequel and Redis, and want to upgrade to database_cleaner v2.0.0. No big deal, you just replace gem "database_cleaner" with gem "database_cleaner-sequel" and gem "database_cleaner-redis" and you're good to go. However, in this situation, the current implementation of cucumber-rails' database_cleaner integration will silently fail.

The underlying issue is that none of the ORM adapters have a lib/database_cleaner.rb file (only the database_cleaner metagem has it, for backwards compatibility with the common case), so the require "database_cleaner" call in the "lib/cucumber/rails/hooks/database_cleaner.rb" will silently fail since LoadError is being caught and discarded, the only visible clue being that the database state is now persisting between test cases. Personally, I consider this issue to be a blocker for releasing final v2.0.0 versions of the database_cleaner family of gems.

How Has This Been Tested?

The first commit in this PR adds a new cucumber feature that fails, exposing this issue. It does this by simply adding gem "database_cleaner-active_record" to the Gemfile, instead of gem "database_cleaner". This was the simplest way to expose the issue without adding a dev dependency on another ORM. This is also a valid and supported way to use database_cleaner with ActiveRecord, BTW.

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly.

This change should be transparent to consumers of the library.

botandrose · 2020-06-06T22:47:52Z

Okay, tests have all completed for the failing test commit. I've checked the output for all the runners, and they are all failing as expected, with the following:

Using the default profile...
Feature: Create widgets
  Background: 2 initial widgets # features/widgets.feature:2
    Given I have 2 widgets      # features/step_definitions/widget_steps.rb:1
  Scenario: Add 3 widgets        # features/widgets.feature:5
    When I create 3 more widgets # features/step_definitions/widget_steps.rb:7
    Then I should have 5 widgets # features/step_definitions/widget_steps.rb:13
  Scenario: Add 7 widgets        # features/widgets.feature:9
    When I create 7 more widgets # features/step_definitions/widget_steps.rb:7
    Then I should have 9 widgets # features/step_definitions/widget_steps.rb:13
      expected: 9
           got: 14
      (compared using ==)
       (RSpec::Expectations::ExpectationNotMetError)
      ./features/step_definitions/widget_steps.rb:14:in `"I should have {int} widgets"'
      features/widgets.feature:11:in `I should have 9 widgets'
Failing Scenarios:
cucumber features/widgets.feature:9 # Scenario: Add 7 widgets
2 scenarios (1 failed, 1 passed)
6 steps (1 failed, 5 passed)
0m0.155s
..................................................................
(::) failed steps (::)
expected that command "bundle exec rake cucumber" has exit status of "0", but has "1". (RSpec::Expectations::ExpectationNotMetError)
./features/step_definitions/cucumber_rails_steps.rb:72:in `"the feature run should pass with:"'
features/database_cleaner.feature:83:in `the feature run should pass with:'
Failing Scenarios:
cucumber features/database_cleaner.feature:46 # Scenario: Create records in background with database_cleaner-active_record

This demonstrates the issue, namely, that the database_cleaner integration will simply stop working without an error message.

botandrose · 2020-06-06T23:22:58Z

Okay, we're green! Ready for review. What do y'all think?

olleolleolle

This change makes sense, is well-described, specified, and improves the implementation by moving the LoadError handling closer to the require it protects.

Thanks @botandrose!

luke-hill · 2020-06-08T07:20:20Z

Hi there. So provisionally I'm ok with this. I'm trying to find a commit, but maybe you can help.

Someone mentioned that with a new-er version of Rails (I can't remember if it was 5.2 or 6.0), database cleaner was no longer required and sort of auto included. But I can't remember the details as it was a long time ago. I remember the thought process here was we were going to remove database cleaner at some point.

If I'm wrong that's fine. You may be able to piece things together better for me. I'll try have a dig around. For now can we just hold off. But if everything's ok I'm happy with the approach here, just need to confirm my original thought process was wrong.

luke-hill · 2020-06-08T07:22:44Z

#406 &
#459

BrianHawley · 2020-06-08T22:25:39Z

It was in Rails 5.1, and it's not strictly that database_cleaner isn't required. It was more that they made it possible to run tests in a mode where all threads share a database connection, and this mode makes it possible to have transaction-mode cleanup actually work in multi-threading environments like capybara.

However, that mode does change the behavior of the app: all threads are sharing one database connection. Rails doesn't have built-in support for any of the other database_cleaner cleanup modes, which you'd still need to use when testing certain kinds of concurrency issues.

Fortunately the new test concurrency mode works with database_cleaner's transactional cleanup mode, so even if you have it installed, it can still be used.

luke-hill · 2020-06-09T08:00:50Z

Ok so in essence, adding this new additional permutation won't alter the use case for people who no longer want to use database cleaner in rails 5.1+ ?

luke-hill · 2020-06-09T08:02:01Z

lib/cucumber/rails/hooks/database_cleaner.rb

+  begin
+    require 'database_cleaner'
+  rescue LoadError
+    Cucumber.logger.debug('database_cleaner not present.')


Probably worth hinting that this has checked both gems right?

Sure! Are you imagining something like "neither database_cleaner v1 or v2 present"?

Something like that yeh

luke-hill · 2020-06-16T08:00:08Z

Also when you come back to this, could you rebase with latest master, and add a changelog entry at the top in unreleased

botandrose · 2020-06-17T17:51:20Z

@luke-hill Rebased and changelog updated. Sorry it took so long to get back to this. Last week was crazy IRL. Looks like I missed the v2.1 release train, too. Crap!

luke-hill · 2020-06-23T09:24:35Z

Trying to see if the failure was a genuine timeout or something else (Fairly sure travis just had a moment).

luke-hill

Few comments.

luke-hill · 2020-06-23T09:25:12Z

CHANGELOG.md


 ### Changed

 *

 ### Fixed

- *
+ * Database cleaning silently fails when using database_cleaner v2 adapter gems


should this say something like "no longer fails", or have I misunderstood this?

Ah, I think I see. I was describing the bug, and you would prefer that I describe the bugfix?

features/support/cucumber_rails_helper.rb

luke-hill · 2020-06-23T09:26:10Z

lib/cucumber/rails/hooks/database_cleaner.rb

+  begin
+    require 'database_cleaner'
+  rescue LoadError
+    Cucumber.logger.debug('database_cleaner not present.')


Something like that yeh

botandrose · 2020-06-23T20:26:37Z

@luke-hill I have just pushed the requested changes the changelog message and the warning message.

aslakhellesoy · 2020-06-25T10:28:13Z

Hi @botandrose,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

botandrose marked this pull request as ready for review June 6, 2020 23:23

olleolleolle approved these changes Jun 7, 2020

View reviewed changes

luke-hill reviewed Jun 9, 2020

View reviewed changes

luke-hill added the Awaiting feedback - reporter label Jun 15, 2020

failing test.

4b6d9ee

botandrose force-pushed the database_cleaner_2_adapter_gems_compat branch from 3314149 to 050995a Compare June 17, 2020 17:50

luke-hill closed this Jun 23, 2020

luke-hill reopened this Jun 23, 2020

luke-hill reviewed Jun 23, 2020

View reviewed changes

compatibility with database_cleaner v2 family of gems.

ccd910c

botandrose force-pushed the database_cleaner_2_adapter_gems_compat branch from 050995a to ccd910c Compare June 23, 2020 20:26

luke-hill approved these changes Jun 25, 2020

View reviewed changes

luke-hill merged commit e53e3de into cucumber:master Jun 25, 2020

botandrose deleted the database_cleaner_2_adapter_gems_compat branch July 1, 2020 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with database_cleaner v2 adapter gems #467

Compatibility with database_cleaner v2 adapter gems #467

botandrose commented Jun 6, 2020 •

edited

Loading

botandrose commented Jun 6, 2020 •

edited

Loading

botandrose commented Jun 6, 2020

olleolleolle left a comment

luke-hill commented Jun 8, 2020

luke-hill commented Jun 8, 2020

BrianHawley commented Jun 8, 2020

luke-hill commented Jun 9, 2020

luke-hill Jun 9, 2020

botandrose Jun 17, 2020

luke-hill Jun 23, 2020

luke-hill commented Jun 16, 2020

botandrose commented Jun 17, 2020

luke-hill commented Jun 23, 2020

luke-hill left a comment

luke-hill Jun 23, 2020

botandrose Jun 23, 2020

luke-hill Jun 23, 2020

botandrose commented Jun 23, 2020

aslakhellesoy commented Jun 25, 2020

Compatibility with database_cleaner v2 adapter gems #467

Compatibility with database_cleaner v2 adapter gems #467

Conversation

botandrose commented Jun 6, 2020 • edited Loading

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

botandrose commented Jun 6, 2020 • edited Loading

botandrose commented Jun 6, 2020

olleolleolle left a comment

Choose a reason for hiding this comment

luke-hill commented Jun 8, 2020

luke-hill commented Jun 8, 2020

BrianHawley commented Jun 8, 2020

luke-hill commented Jun 9, 2020

luke-hill Jun 9, 2020

Choose a reason for hiding this comment

botandrose Jun 17, 2020

Choose a reason for hiding this comment

luke-hill Jun 23, 2020

Choose a reason for hiding this comment

luke-hill commented Jun 16, 2020

botandrose commented Jun 17, 2020

luke-hill commented Jun 23, 2020

luke-hill left a comment

Choose a reason for hiding this comment

luke-hill Jun 23, 2020

Choose a reason for hiding this comment

botandrose Jun 23, 2020

Choose a reason for hiding this comment

luke-hill Jun 23, 2020

Choose a reason for hiding this comment

botandrose commented Jun 23, 2020

aslakhellesoy commented Jun 25, 2020

botandrose commented Jun 6, 2020 •

edited

Loading

botandrose commented Jun 6, 2020 •

edited

Loading