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

rspec should not require user to call rake db:test:prepare #949

Closed
rdpoor opened this issue Mar 9, 2014 · 19 comments
Closed

rspec should not require user to call rake db:test:prepare #949

rdpoor opened this issue Mar 9, 2014 · 19 comments

Comments

@rdpoor
Copy link

rdpoor commented Mar 9, 2014

The user should not need to call rake db:test:prepare prior to running the tests. (I cannot point to when this became an issue, but I know it's an issue in Ruby 2.0 / Rails 4.0 / rspec-rails 2.14.)

FWIW, I argued for reinstating documentation on db:test:prepare in rails/rails#2560 (comment), and zzak sensibly pointed out that user's shouldn't need to call it.

This is probably related to #663, #944 and #936, but I admit that I haven't kept up with the more recent developments.

Feel free to close this if it's already been addressed. Thanks.

@JonRowe
Copy link
Member

JonRowe commented Mar 10, 2014

Can you elaborate? We should still be executing the db:test:prepare task for you on Rails 4.0 (not 4.1) and the latest release of 2.14. If not can you try to reproduce this in a bare repo and send us a link?

@rdpoor
Copy link
Author

rdpoor commented Mar 10, 2014

After poking at it, I'll concede that this may just be operator error, but then I'd argue it may be a documentation bug. Consider the following description of the problem and tell me which you think it is:

  1. built a fresh rails app (with rspec-rails, of course)
  2. generated a Muse model with a :name field and a muse_spec to test it.
  3. ran $ rake db:create
  4. ran $ rake db:migrate (NOT with RAILS_ENV=test)
  5. ran $ rspec --no-color. Test failed
  6. ran $ rake db:test:prepare
  7. ran $ rspec --no-color. Test passes.

You may argue that at step 4. I should have run rake db:migrate RAILS_ENV=test, and that would make everything work. I'd counter by saying that somewhere back in the days of Rails 3.x, I got used to typing rake db:migrate; rake db:test:prepare. According to stack overflow, a lot of other people got used to doing this as well ; it's offered as an answer to "why don't my rspec tests work?"

Where is the documentation that says we should be running rake db:migrate RAILS_ENV=test instead?

@JonRowe
Copy link
Member

JonRowe commented Mar 10, 2014

All of those commands are provided by Rails, and is changing in 4.1 anyway because, I assume of this confusion.

@soulcutter
Copy link
Member

When you run rspec, are you using the rake task, or the rspec command?

@rdpoor
Copy link
Author

rdpoor commented Mar 10, 2014

Raw rspec command. (Edited comment above to emphasize the fact.)

@rdpoor
Copy link
Author

rdpoor commented Mar 10, 2014

@JonRowe: The Rails team made a conscious decision to remove the documentation for rake db:test:prepare on the basis that users shouldn't need to call it directly and further suggested that rspec should't require it. This is what started me down this rabbit hole...

@soulcutter
Copy link
Member

The rake task invokes it… I'm not sure the naked rspec command actually should invoke it since not all specs depend will on the database, and you may be using the command to run individual files or tests.

@rdpoor
Copy link
Author

rdpoor commented Mar 10, 2014

@soulcutter: I can confirm that running rake spec properly conditions the database for testing (i.e. it obviates the need to call rake db:test:prepare after making a change to the schema).

But in all the documentation I've read, I've never seen any suggestion that rspec should be called via a rake task. Indeed, all the examples for command line args (e.g. --no-color) or autotest show rspec being called directly, not as a rake task.

Am I (and the StackOverflow community) missing something?

@myronmarston
Copy link
Member

I haven't done rails in a long time (most of my rails experience is from the rails 2.x days), but if memory serves, rspec has never prepared the test database. Historically, the means to prepare the test database has only been made available as a rake task, so it made sense to make it a prerequisite of rake spec, but running it from rspec would have been more complicated and also bogged it down. Folks often want to use rspec to run individual specs and get fast feedback and doing the db:test:prepare logic would add a few seconds to the spec run.

At this point, it sounds like it's enough of a pain point for users that we should consider changing that, particularly with the rails core team hiding db:test:prepare and declaring it's not something users should ever run by hand. Here's my suggestion:

  • Make it configurable. Some users will not want this behavior.
  • Make the config "smart" by default: for example, maybe it would only default to on if defined?(ActiveRecord).

@JonRowe
Copy link
Member

JonRowe commented Mar 10, 2014

I believe the task is going away in Rails 4.1 anyway (being replaced by an API call), I'd be on board with making it a setting/or api call thats set to on in the default spec_helper but documented how to make it selective, e.g.:

config.before(:all, type: :model) do
  config.ensure_database_migrated!
end

@myronmarston
Copy link
Member

You'd want before(:suite), not before(:all) as you want it only once, not once per example group.

@JonRowe
Copy link
Member

JonRowe commented Mar 10, 2014

True

@soulcutter
Copy link
Member

I did some digging into how this is managed in Rails 4.1 and it looks isolated to this commit: rails/rails@ff7ab3b

So, there's ActiveRecord::Base.maintain_test_schema which defaults to true that controls the behavior of whether or not ActiveRecord::Migration.maintain_test_schema! does anything. Seems fairly easy to integrate with.

Harder to figure out the pre-4.1 behavior IMO

@JonRowe
Copy link
Member

JonRowe commented Mar 11, 2014

Yeah I already added ActiveRecord::Migration.maintain_test_schema! to the default spec_helper for 4.1, I'm just wondering if it makes sense figuring out whatever older Rails did and then having an rspec-rails api we maintain that replicates it on < 4.1 and calls this > 4.1

@JonRowe
Copy link
Member

JonRowe commented Mar 12, 2014

BTW @rdpoor have you tried Rails 4.1 and the new spec helper?

@rdpoor
Copy link
Author

rdpoor commented Mar 12, 2014

@JonRowe: not yet but I will. It will take a couple of days to get to it...

@tomasgregor
Copy link

@JonRowe I have tried Rails 4.1 with rspec-rails 3.0.0.beta2 and the rails g rspec:install doesn't generate the line ActiveRecord::Migration.maintain_test_schema!

I also experience another issue - the examples are not cleaned after tests run (config.use_transactional_fixtures = true). I'm using FactoryGirls as well and this wasn't a problem before.

@cupakromer
Copy link
Member

@tomasgregor 3beta2 does not have the new spec_helper generator. You'll need to try against master or wait a bit until we release RC1. As for the other issue, if you can provide consistent repro-steps (I'm not able to repo at the moment), please open a new issue and I'll be happy to take a look.

@cupakromer
Copy link
Member

I think we can close this. We no longer call db:test:prepare in Rails 4.1: b8680f9

Based on my research the current master branch behavior is in line with how test_helper is working.

My only change would be to possibly turn:

if ::Rails::VERSION::STRING.to_f < 4.1
  Rake::Task["test:prepare"].invoke
end

into

if Rake::Task.task_defined?("test:prepare")
  Rake::Task["test:prepare"].invoke
end

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

No branches or pull requests

6 participants