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

Cucumber::Rails::Database.javascript_strategy is too simple #537

Open
jherdman opened this issue Apr 7, 2022 · 12 comments
Open

Cucumber::Rails::Database.javascript_strategy is too simple #537

jherdman opened this issue Apr 7, 2022 · 12 comments

Comments

@jherdman
Copy link

jherdman commented Apr 7, 2022

πŸ‘“ What did you see?

When database_cleaner-redis AND database_cleaner-active_record are in play, I cannot correctly configure them for the Cucumber::Rails::Database.javascript_strategy. I'm forced to consolidate upon a single strategy.

βœ… What did you expect to see?

I am able to set the correct strategy for any given database cleaner adapter I'm using.

πŸ“¦ Which tool/library version are you using?

cucumber-rails v2.5.1

πŸ”¬ How could we reproduce it?

Steps to reproduce the behavior:

  1. Set up some Rails app with cucumber-rails, Redis, and database-cleaner with both database_cleaner-active_record and database_cleaner-redis
  2. Add the following to features/support/env.rb:
# Remove/comment out the lines below if your app doesn't have a database.
# For some databases (like MongoDB and CouchDB) you may need to use :truncation instead.
begin
  DatabaseCleaner[:active_record].strategy = :transaction
  DatabaseCleaner[:redis].strategy = :deletion
rescue NameError
  raise "You need to add database_cleaner to your Gemfile (in the :test group) if you wish to use it."
end

Before('@javascript') do
  DatabaseCleaner[:redis].strategy = :deletion
end
  1. Run your Cucumber suite
  2. See error:
Using the default profile...
@javascript
Feature: Some feature
  As an...
  I want to...
  So that....

  Background:                                                           # features/something.feature:8
      The 'truncation' strategy does not exist for the redis ORM!  Available strategies: deletion (DatabaseCleaner::UnknownStrategySpecified)

πŸ“š Any additional context?


This text was originally generated from a template, then edited by hand. You can modify the template here.

@jherdman
Copy link
Author

jherdman commented Apr 8, 2022

Reproduction https://github.com/jherdman/cucumber-rails-database-cleaner-repro. Simply running bin/cucumber after clone, and install will reproduce.

I think my ideal world would be the total elimination of the default JavaScript strategy. We already have the ability to do before/after/around hooks using tags for @javascript, why do we need anything more than that?

@jherdman
Copy link
Author

jherdman commented Apr 8, 2022

Should anyone find this and need a work around you can simply "alias" the deletion strategy as truncation as follows:

module DatabaseCleaner
  module Redis
    Truncation = Deletion
  end
end

@luke-hill
Copy link
Contributor

Instead of removing the default strategy, how would you feel if instead the default was set to the new null strategy. Which does nothing?

This would be much easier to do (Yes it would still be a breaking change). But it would be a trivial task as opposed to a bigger task.

@jherdman
Copy link
Author

Perhaps simply offering a null strategy in which one must configure the Before/After hooks would be less disruptive? This would resolve my issue.

@luke-hill
Copy link
Contributor

So we have a new null strategy. But this must be configured. However this still cannot be default set (We forcibly set the default as you are seeing).

What I'm proposing is not removing the forcible setting, but instead forcibly setting it to the NullStrategy. Which would be a trivial (breaking), change. This then in theory would not cause issues for yourself.

I can make a sample branch if you would prefer to trial out the behaviour?

@jherdman
Copy link
Author

That'd be great!

@luke-hill
Copy link
Contributor

I'll get something up by the end of the week. there's no guarantees this will fix your issue but we can see.

We've been discussing doing a major release soon for some other small things, so the major release isn't really a concern.

@luke-hill
Copy link
Contributor

And I pushed this down the priority queue again :( I'll try get to this soon, or failing that if you do feel free.

@luke-hill
Copy link
Contributor

@jherdman given the lack of other commenting on here? I'm unsure if this is as important as originally mentioned.

Is this still an issue for you?

@jherdman
Copy link
Author

This is no longer an issue for me. Please feel free to close this issue if you feel that it's resolved to your satisfaction.

@luke-hill
Copy link
Contributor

It's something I don't think I have enough knowledge of to make a breaking change on.

Is this no longer an issue because of the monkeypatch you have done? Or something else?

@jherdman
Copy link
Author

I'm no longer working on the project where this caused the problems for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants