Skip to content
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

Generate DatabaseCleaner setup in new apps #27

Closed
timriley opened this issue Jul 9, 2024 · 6 comments · Fixed by #28
Closed

Generate DatabaseCleaner setup in new apps #27

timriley opened this issue Jul 9, 2024 · 6 comments · Fixed by #28
Assignees

Comments

@timriley
Copy link
Member

timriley commented Jul 9, 2024

Cleaning the database between tests is essential functionality, and I don't think users should have to think about it.

We need a setup that:

  • Works with regular tests (transaction) as well as capybara-with-JS tests (truncation)
  • Works with multiple databases across slices

I think we should just generate the appropriate config into spec/support/database_cleaner.rb for the app database, and include some commented examples of how to make it work for additional databases.

Later (post-2.2) we can look at ways at codifying certain aspects of iterating over/interacting with the app's set of databases into hanami-db, and then find ways for that to be used to configure DatabaseCleaner.

@cllns
Copy link
Member

cllns commented Jul 9, 2024

Here's mine from my app, fwiw:

# frozen_string_literal: true

require "database_cleaner/sequel"

# We need to have this because we're using a relative path to a sqlite file
DatabaseCleaner.allow_remote_database_url = true

# We avoid touching the DB in our tests, except for request and repo specs
RSpec.configure do |config|
  config.before(:suite) do
    # This isn't necessary but a good safeguard in case our suite fails
    DatabaseCleaner[:sequel].clean_with(:deletion)
  end

  config.around(:each, type: :repo) do |example|
    DatabaseCleaner[:sequel].strategy = :transaction

    DatabaseCleaner.cleaning do
      example.run
    end
  end

  config.around(:each, type: :request) do |example|
    DatabaseCleaner[:sequel].strategy = :deletion

    DatabaseCleaner.cleaning do
      example.run
    end
  end
end

I don't know if we want to be as opinionated as this.

I think we should be able to avoid DatabaseCleaner.allow_remote_database_url = true, that might just be an idiosyncrasy because of how I set up my app (with DATABASE_URL=sqlite://./db/realworld_development.sqlite3)

@alassek
Copy link

alassek commented Oct 8, 2024

Mine is probably way too idiosyncratic to be useful

require "database_cleaner/sequel"
require "persistence"

RSpec.configure do |config|
  config.before :suite do
    Persistence.specification.each do |_, spec|
      DatabaseCleaner[:sequel, db: spec.slice["persistence.db"]].clean_with :truncation, { except: %w[schema_migrations system_metadata] }
    end
  end

  config.around :each, :slice do |example|
    container["persistence.db"].transaction(rollback: :always, auto_savepoint: true) do
      example.run
    end
  end
end

Although I think that part about excepting the schema_migrations table might be applicable?

@cllns does yours not interfere with your schema_migrations? I know for certain we were losing migration metadata without it.

@timriley timriley self-assigned this Oct 19, 2024
@timriley
Copy link
Member Author

timriley commented Oct 19, 2024

FYI, I've left a nudge here about getting a new database_cleaner released so we don't have to include DatabaseCleaner.allow_remote_database_url = or DatabaseCleaner.url_allowlist = [%r{^sqlite://}] in our default config.

Speaking of that default config, now that we have multiple gateways, I think it's fairly important that it now handles all possible database arrangements out of the box. I'm going to start work on putting something together.

@timriley
Copy link
Member Author

timriley commented Oct 19, 2024

Here's a first cut at a config that operates on all databases. It exposes more "wiring" than I would like, but with some comments added in, I think it would be OK as a good enough starting point.

require "database_cleaner/sequel"

RSpec.configure do |config|
  all_databases = -> {
    slices = [Hanami.app] + Hanami.app.slices.with_nested

    slices.each_with_object([]) { |slice, dbs|
      dbs.concat slice["db.rom"].gateways.values.map(&:connection) if slice["db.rom"]
    }.uniq
  }

  config.before :suite do
    all_databases.call.each do |db|
      DatabaseCleaner[:sequel, db:].clean_with :truncation, except: ["schema_migrations"]
    end
  end

  config.prepend_before :each, :db do |example|
    strategy = example.metadata[:js] ? :truncation : :transaction

    all_databases.call.each do |db|
      DatabaseCleaner[:sequel, db:].strategy = strategy
      DatabaseCleaner[:sequel, db:].start
    end
  end

  config.append_after :each, :db do
    all_databases.call.each do |db|
      DatabaseCleaner[:sequel, db:].clean
    end
  end
end

The downside of this config is that it is "eager", in that it'll go and operate on every database in the app, even if an individual test is being run that doesn't even use those databases.

In the future I think we can add APIs to hanami-db or hanami-rspec to make this nicer, as well as introduce a more opinionated structure to our default RSpec setup (e.g. a directory per slice, with the slice name added to the examples as metadata) so that the database cleaner config could know which specific databases to operate on.

However, at this current (late) stage in the 2.2 development cycle, I'd prefer to keep things simple and not rush to introduce new public API or directory structures. I think we could learn from some real-world usage and discussions with our community about what could serve them best.

What do you think?

@timriley
Copy link
Member Author

One minor thought: it would be nice if we could reduce [Hanami.app] + Hanami.app.slices.with_nested to a single method call. That feels like something we could reasonably add now, and it would certainly help reduce some distracting noise from this database cleaner setup.

@timriley timriley transferred this issue from hanami/cli Oct 19, 2024
@alassek
Copy link

alassek commented Oct 21, 2024

@timriley it would be nice if we could reduce [Hanami.app] + Hanami.app.slices.with_nested to a single method call.

Only difficuly would be thinking of an appropriate interface.

Hanami.containers?

@github-project-automation github-project-automation bot moved this from For review to Done in Hanami 2.2 Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants