Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

db:test:prepare task not being called when schema_format is :sql #663

Closed
mathie opened this Issue · 10 comments

7 participants

@mathie

I'm wondering why the spec tasks' prerequisites are being altered if schema_format is set to sql. On rspec.rake#8, it's depending on db:test:prepare by default, but if the schema format is :sql, it changes to explicitly depending on db:test:clone_structure.

Looking at the prepare task, databases.rake#556, it looks like it should do the right thing in either case, so having a prerequisite on db:test:prepare should be sufficient?

I've a sneaking suspicion you're going to point out that didn't used to be the case. :) However, it has been on Rails 3 for > 2 years, and is on master, too.

The disadvantage of explicitly depending on db:test:clone_structure is that it doesn't allow other gems/plugins to provide database preparation tasks. In particular, the named_seeds gem from @metaskills hooks into db:test:prepare task to load seed data after the schema has been loaded.

What do you reckon to reverting 6d726b5?

@mathie

On second thoughts, it looks like there's a more generic hook task to depend upon: test:prepare. I wonder when that got introduced?

@dchelimsky
Owner

This was from pull request #517. @justinko, @Antiarchitect, do either of you recall what problem this solved?

@justinko

All I can say is this was a bad merge on my part for not looking at db:test:prepare. I say revert unless @Antiarchitect has a reason not to.

@Antiarchitect

No reason - do what you want.

@metaskills

Thanks for the mention @mathie, totally agree. The test:prepare is considered the public interface hook provided by railties and that is what ActiveRecord hooks into by calling db:test:prepare. My advice would be to not only revert 6d726b5, but to also change that line to call the public'ish test:prepare. Assuming different ORMs followed the proper path of ActiveRecord hooks into railties, this would work like a champ for every :orm, not just those equal to :active_record.

@alindeman
Collaborator

Thanks for the mention @mathie, totally agree. The test:prepare is considered the public interface hook provided by railties and that is what ActiveRecord hooks into by calling db:test:prepare. My advice would be to not only revert 6d726b5, but to also change that line to call the public'ish test:prepare. Assuming different ORMs followed the proper path of ActiveRecord hooks into railties, this would work like a champ for every :orm, not just those equal to :active_record.

I'm fine with this. Any objections?

@mathie

@alindeman Sounds good to me. Thanks for taking the time to look into this, everyone. :)

@metaskills metaskills referenced this issue in metaskills/named_seeds
Closed

Hook `db:test:seeds` in `db:test:prepare` #2

@alindeman alindeman referenced this issue from a commit
@alindeman alindeman Revert "Make rspec to copy schema structure if schema format is :sql."
This reverts commit 6d726b5.

* See discussion in #663
f5a227c
@alindeman alindeman closed this issue from a commit
@alindeman alindeman Uses test:prepare as the prereq
* Normally aliased as db:test:prepare
* But can be overridden by other ORMs
* Closes #663
b5e9963
@alindeman alindeman closed this in b5e9963
@alindeman
Collaborator

I'm not yet comfortable making test:prepare a prereq for all ORMs ... until I've researched that they do the right thing. Until then, feel free to ping me if your specific ORM does support it, and we'll add those one by one.

If you have a few minutes, please try the latest rspec-rails master to see if the commits above do the right thing in your project.

@metaskills

There has been some work in Rails 4.2.0 that changes which test task hook you might use. Details can be found here rails/rails#17739 but the short story is that if you want to do something to the test DB after it has been synchronized you should use db:test:prepare. The test:prepare is still valid, but the test DB is not synchronized yet for that task.

@cupakromer cupakromer reopened this
@cupakromer cupakromer closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.