DatabaseCleaner / database_cleaner

Strategies for cleaning databases in Ruby. Can be used to ensure a clean state for testing.
https://www.rubydoc.info/github/DatabaseCleaner/database_cleaner
MIT License
2.93k stars 483 forks source link

1.8.1 mongoid adapter has a reference to moped. Is this right? #616

Closed Cruikshanks closed 4 years ago

Cruikshanks commented 4 years ago

We have 3 projects that currently use the database cleaner mongoid adapter for cleaning our MongoDb 3.6 database when testing.

The release of version 1.8.1 kicked of Dependabot to raise a PR in each to upgrade Database Cleaner. However, every one of them broke

The error we get is

An error occurred in a `before(:suite)` hook.
Failure/Error: DatabaseCleaner[:mongoid].strategy = :truncation

LoadError:
  cannot load such file -- database_cleaner/mongo/truncation_mixin
# ./spec/support/database_cleaner.rb:9:in `block (2 levels) in <top (required)>'

From our understanding of what 1.8 is doing to get ready for version 2.0, this is not unexpected. We understand 1.8 now needs us to explicitly reference the adapter gem required in our Gemfile. So we did that but still got an error.

LoadError:
  cannot load such file -- database_cleaner/moped/truncation_base
# ./spec/dummy/config/application.rb:12:in `<top (required)>'
# ./spec/dummy/config/environment.rb:2:in `require'
# ./spec/dummy/config/environment.rb:2:in `<top (required)>'
# ./spec/rails_helper.rb:6:in `require'
# ./spec/rails_helper.rb:6:in `<top (required)>'
# ./spec/controllers/waste_carriers_engine/registrations_spec.rb:3:in `require'
# ./spec/controllers/waste_carriers_engine/registrations_spec.rb:3:in `<top (required)>'

When we checked the database_cleaner-mongoid.gemspec it didn't seem to include any reference to database_cleaner-moped. But we did find a require call to it in DatabaseCleaner::Mongoid::Truncation

require 'database_cleaner/mongoid/base'
require 'database_cleaner/generic/truncation'
require 'database_cleaner/mongo/truncation_mixin'
require 'database_cleaner/mongo2/truncation_mixin'
require 'database_cleaner/moped/truncation_base' # <- Here
require 'mongoid/version'

module DatabaseCleaner
  module Mongoid
    class Truncation

We were able to get our tests running again by also including a reference to gem "database_cleaner-moped" in our Gemfile.

If you check the commits and their comments in https://github.com/DEFRA/waste-carriers-engine/pull/654 you should see what we attempted, and then what worked for us.

Based on what we found, we're just wondering whether the require 'database_cleaner/moped/truncation_base' is correct. If so, does the database_cleaner-moped gem need to be a dependency of database_cleaner-mongoid?

I hope that's clear. I may have added too much detail 😳. But feel free to call us out if we are just doing something stupid or we have overlooked something.

botandrose commented 4 years ago

@Cruikshanks Thank you for bringing this to my attention, and double thank you for the very detailed error report! This does seem to be a mistake on first look. Probably a typo on my part while I was splitting out the 10 different adapters. I'll take a closer look presently.

botandrose commented 4 years ago

Looking at this a bit more closely, I think there are really two issues:

  1. Upgrading to from 1.7 to 1.8 broke requiring strategies directly, e.g. require "database_cleaner/mongo/truncation_mixin" throws an exception now. This is a regression! 1.8 should be a backwards-compatible release with 1.7. If anything, this require pattern should display a deprecation warning, not throw an exception!
  2. The issue you've raised about the moped dependency
botandrose commented 4 years ago

@Cruikshanks Okay, I've looked at this even more closely, and it appears that the moped dependency is, in fact, intentional. It looks like database_cleaner/mongoid is actually choosing between database_cleaner/mongo, database_cleaner/mongo2, and database_cleaner/moped as its internal implementation, depending on the version on Mongoid that is loaded. When I split the library apart into adapters, I assumed that the adapters were all separate, but it turns out that that is not the case!

So, moving forward, I'm going to release a database_cleaner-mongoid v1.8.1 to address this issue. Then, I'll see if I can't figure out a way to deal with the direct strategy require breakage in database_cleaner proper.

botandrose commented 4 years ago

It goes deeper! In addition to DC::Mongoid relying on those three separate implementations, DC::Ohm depends on DC::Redis, and DC::MongoMapper depends on DC::Mongo! Honestly, I'm embarassed that I didn't notice this when extracting the adapters. Bugfix releases forthcoming!

botandrose commented 4 years ago

@Cruikshanks would you do me a favor and test your app with the #617 branch to verify that it has resolved this issue for you? To be clear, please note that at the moment, its just the base database_cleaner gem, not the database_cleaner-mongoid adapter, so your Gemfile would look something like:

gem "database_cleaner", git: "https://github.com/DatabaseCleaner/database_cleaner", branch: "inline_mongoid_dependencies"

If this resolves this issue for you, I'll release database_cleaner-mongoid v1.8.1 immediately, and likely follow that up with a v1.8.2 of database_cleaner once I resolve a few more other known issues.

Thank you for your help!

vitalinfo commented 4 years ago

@botandrose it does work for me (1.8.1 doesn't work for me as well)

botandrose commented 4 years ago

@vitalinfo Awesome! Thank you for testing it. I'll merge and release database_cleaner-mongoid v1.8.1 presently.

botandrose commented 4 years ago

https://rubygems.org/gems/database_cleaner-mongoid/versions/1.8.1 :rocket:

However, note this issue won't be resolved in the base database_cleaner gem until we release v1.8.2, which will contain a few additional bugfixes.

Cruikshanks commented 4 years ago

Thank you, thank you, thank you!! 😁 πŸŽ‰ πŸ₯‡

Cruikshanks commented 4 years ago

And just to confirm, Dependabot closed those original PR's, but the new ones it created for 1.8.2 all passed CI and were merged successfully this AM without needing to add additional dependencies or make changes to our Gemfiles.

So many thanks again πŸ‘

botandrose commented 4 years ago

That's wonderful news! Thanks for letting us know. Hopefully all future releases will be that non-eventful 🀞