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

Automatically maintain test database schema #13528

Merged
merged 1 commit into from Jan 2, 2014

Conversation

Projects
None yet
@jonleighton
Member

jonleighton commented Dec 29, 2013

  • Move check from generated helper to test_help.rb, so that all
    applications can benefit
  • Rather than just raising when the test schema has pending migrations,
    try to load in the schema and only raise if there are pending
    migrations afterwards

TODO:

  • We no longer need db:test:prepare and friends. Deprecate them.
  • Update docs/changelog
  • Add config option to allow users to opt-out of this behaviour
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 29, 2013

Contributor

Great work. If we are going to allow people to manually opt-out of this behaviour we should probably not remove db:test:prepare for the users that would prefer to do it manually. Or would they rather call load_schema_if_pending?

Contributor

josevalim commented Dec 29, 2013

Great work. If we are going to allow people to manually opt-out of this behaviour we should probably not remove db:test:prepare for the users that would prefer to do it manually. Or would they rather call load_schema_if_pending?

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Dec 29, 2013

Member

@josevalim I'm basically proposing we deprecate these: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/railties/databases.rake#L301-L364

I don't see what purpose they serve any more. People who want to do these things manually can do stuff like

rake db:schema:load RAILS_ENV=test

So I don't think we need these test-specific rake tasks at all?

Member

jonleighton commented Dec 29, 2013

@josevalim I'm basically proposing we deprecate these: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/railties/databases.rake#L301-L364

I don't see what purpose they serve any more. People who want to do these things manually can do stuff like

rake db:schema:load RAILS_ENV=test

So I don't think we need these test-specific rake tasks at all?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 29, 2013

Contributor

Sounds good!

Contributor

josevalim commented Dec 29, 2013

Sounds good!

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Dec 31, 2013

Member

@josevalim I consider this finished now, let me know if you have any more comments

Member

jonleighton commented Dec 31, 2013

@josevalim I consider this finished now, let me know if you have any more comments

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 1, 2014

Member

Seems good to me

Member

rafaelfranca commented Jan 1, 2014

Seems good to me

Automatically maintain test database schema
* Move check from generated helper to test_help.rb, so that all
  applications can benefit
* Rather than just raising when the test schema has pending migrations,
  try to load in the schema and only raise if there are pending
  migrations afterwards
* Opt out of the check by setting
  config.active_record.maintain_test_schema = false
* Deprecate db:test:* tasks. The test helper is now fully responsible
  for maintaining the test schema, so we don't need rake tasks for this.
  This is also a speed improvement since we're no longer reloading the
  test database on every call to "rake test".

jonleighton added a commit that referenced this pull request Jan 2, 2014

Merge pull request #13528 from jonleighton/maintain_test_schema
Automatically maintain test database schema

@jonleighton jonleighton merged commit d740438 into rails:master Jan 2, 2014

1 check passed

default The Travis CI build passed
Details
@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Jan 2, 2014

Member

cc @alindeman in case you want to add this to rspec-rails

Member

jonleighton commented Jan 2, 2014

cc @alindeman in case you want to add this to rspec-rails

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 2, 2014

Member

❤️ great work here.

Member

schneems commented Jan 2, 2014

❤️ great work here.

@jasontorres

This comment has been minimized.

Show comment
Hide comment
@jasontorres

jasontorres commented Jan 3, 2014

Great job!

@orendon

This comment has been minimized.

Show comment
Hide comment
@orendon

orendon commented Jan 3, 2014

👍

@tamouse

This comment has been minimized.

Show comment
Hide comment
@tamouse

tamouse Jan 4, 2014

this makes me weep with joy

tamouse commented Jan 4, 2014

this makes me weep with joy

@jaredbeck

This comment has been minimized.

Show comment
Hide comment
@jaredbeck

jaredbeck Mar 26, 2014

Contributor

Should rspec users add require 'rails/test_help' to their spec_helper? Thanks, looking forward to trying this out!

Contributor

jaredbeck commented Mar 26, 2014

Should rspec users add require 'rails/test_help' to their spec_helper? Thanks, looking forward to trying this out!

jaredbeck added a commit to usgo/gocongress that referenced this pull request Mar 26, 2014

Updates rails to 4.1.0.rc2
- [release notes][1]
- [release announcement][2]
- [deprecation of db:test:* tasks][3]
- Had to update `sass-rails` to 4.0.2 because of
an [issue with sprockets 2.12][4]

[1]: http://edgeguides.rubyonrails.org/4_1_release_notes.html
[2]: http://weblog.rubyonrails.org/2014/3/25/Rails-4-1-rc2/
[3]: rails/rails#13528
[4]: sstephenson/sprockets#540
@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Mar 28, 2014

Member

@jaredbeck yep could do, but I think ideally the rspec-rails gem would integrate support for this

Member

jonleighton commented Mar 28, 2014

@jaredbeck yep could do, but I think ideally the rspec-rails gem would integrate support for this

@swebb

This comment has been minimized.

Show comment
Hide comment
@swebb

swebb Apr 16, 2014

@jaredbeck I found adding require 'rails/test_help' caused rspec and minitest to run. Instead I added ActiveRecord::Migration.maintain_test_schema! to my spec_helper.

swebb commented Apr 16, 2014

@jaredbeck I found adding require 'rails/test_help' caused rspec and minitest to run. Instead I added ActiveRecord::Migration.maintain_test_schema! to my spec_helper.

@jaredbeck

This comment has been minimized.

Show comment
Hide comment
@jaredbeck

jaredbeck Apr 16, 2014

Contributor

I think ideally the rspec-rails gem would integrate support for this

If I'm understanding rspec/rspec-rails#946 correctly, newly-generated spec_helper files will include a call to maintain_test_schema!. Is that what you were thinking, Jon?

Contributor

jaredbeck commented Apr 16, 2014

I think ideally the rspec-rails gem would integrate support for this

If I'm understanding rspec/rspec-rails#946 correctly, newly-generated spec_helper files will include a call to maintain_test_schema!. Is that what you were thinking, Jon?

@kevintyll

This comment has been minimized.

Show comment
Hide comment
@kevintyll

kevintyll Apr 17, 2014

The only problem with removing the test load tasks, is that they purge the db before loading it, so now deleted tables will never be removed from the test db.

A separate issue is we are constantly branching and merging our app, so using the latest schema info to determine if we need to load the schema does not work for us. I may be switching to a branch that has a later migration than the one I just came from, but it won't load because the test db has a more recent migration. I'm solving the problem by comparing the structure.sql mtime to the test.log mtime.

Here is the patch file I created for our use. I don't think this is a global solution, but it works for us and may give you ideas if you agree that using the schema_migrations table is limited. The path file also includes patches for parallel_test, which adds additional issues:

# As of rails 4.1, the schema is loaded automatically if needed based on the schema_migrations table, the problem with this is
# with migrations created in different branches, that not very accurate.  This file overrides the classes necessary
# to load it based on comparing the structure.sql modified time and the test.log modified time.  It will load if the structure
# was modified since the last test run.

# This file also overrides the structure load task to do a purge before loading.


# This file gets loaded in the test and development environments from application.rb

if Rails.env.test?
  module ActiveRecord
    module Tasks # :nodoc:
      module DatabaseTasks
        # structure_load does not purge the db first, so deleted tables do not get deleted.
        # this gets called automatically from tests as of rails 4.1.0
        def structure_load_with_purge(*arguments)
          puts "Loading structure.sql"
          purge(current_config)
          structure_load_without_purge(*arguments)
          ActiveRecord::Migrator.last_test_run = Time.now
        end

        alias_method_chain :structure_load, :purge
      end
    end
  end

  module ActiveRecord
    class Migrator #:nodoc:
      class << self

        cattr_accessor :last_test_run

        def needs_migration?(connection = Base.connection)
          structure_updated = File.mtime('db/structure.sql')
          puts "Structure.sql modified: #{structure_updated}"
          puts "Last test run: #{last_test_run}"
          last_test_run < structure_updated
        end
      end
    end
  end


  Rails.application.config.before_configuration do
    # If running parallel tests, one process may get ahead of the others and write to the test.log file before another process can
    # check it to see if the structure should be loaded.  So some instances may load the structure, but not others.
    # Have all processes wait here until all have started so they can check the log file at the same time.
    if ENV['PARALLEL_TEST_GROUPS'].present?
      loop do
        break if ParallelTests.number_of_running_processes.to_s == ENV['PARALLEL_TEST_GROUPS'].to_s
        sleep 1
      end
    end
    ActiveRecord::Migrator.last_test_run = File.mtime('log/test.log') rescue Time.parse('2009-01-01')
  end
end

if Rails.env.development?
  require 'parallel_tests/tasks'
  module ParallelTests
    module Tasks
      class << self
        def check_for_pending_migrations
          # no op
          # we just want to load the structure file, we don't care if the dev db has not run all the migrations yet.
          # if the structure file is missing migrations, it will still error as of rails 4.1
        end
      end
    end
  end
end

kevintyll commented Apr 17, 2014

The only problem with removing the test load tasks, is that they purge the db before loading it, so now deleted tables will never be removed from the test db.

A separate issue is we are constantly branching and merging our app, so using the latest schema info to determine if we need to load the schema does not work for us. I may be switching to a branch that has a later migration than the one I just came from, but it won't load because the test db has a more recent migration. I'm solving the problem by comparing the structure.sql mtime to the test.log mtime.

Here is the patch file I created for our use. I don't think this is a global solution, but it works for us and may give you ideas if you agree that using the schema_migrations table is limited. The path file also includes patches for parallel_test, which adds additional issues:

# As of rails 4.1, the schema is loaded automatically if needed based on the schema_migrations table, the problem with this is
# with migrations created in different branches, that not very accurate.  This file overrides the classes necessary
# to load it based on comparing the structure.sql modified time and the test.log modified time.  It will load if the structure
# was modified since the last test run.

# This file also overrides the structure load task to do a purge before loading.


# This file gets loaded in the test and development environments from application.rb

if Rails.env.test?
  module ActiveRecord
    module Tasks # :nodoc:
      module DatabaseTasks
        # structure_load does not purge the db first, so deleted tables do not get deleted.
        # this gets called automatically from tests as of rails 4.1.0
        def structure_load_with_purge(*arguments)
          puts "Loading structure.sql"
          purge(current_config)
          structure_load_without_purge(*arguments)
          ActiveRecord::Migrator.last_test_run = Time.now
        end

        alias_method_chain :structure_load, :purge
      end
    end
  end

  module ActiveRecord
    class Migrator #:nodoc:
      class << self

        cattr_accessor :last_test_run

        def needs_migration?(connection = Base.connection)
          structure_updated = File.mtime('db/structure.sql')
          puts "Structure.sql modified: #{structure_updated}"
          puts "Last test run: #{last_test_run}"
          last_test_run < structure_updated
        end
      end
    end
  end


  Rails.application.config.before_configuration do
    # If running parallel tests, one process may get ahead of the others and write to the test.log file before another process can
    # check it to see if the structure should be loaded.  So some instances may load the structure, but not others.
    # Have all processes wait here until all have started so they can check the log file at the same time.
    if ENV['PARALLEL_TEST_GROUPS'].present?
      loop do
        break if ParallelTests.number_of_running_processes.to_s == ENV['PARALLEL_TEST_GROUPS'].to_s
        sleep 1
      end
    end
    ActiveRecord::Migrator.last_test_run = File.mtime('log/test.log') rescue Time.parse('2009-01-01')
  end
end

if Rails.env.development?
  require 'parallel_tests/tasks'
  module ParallelTests
    module Tasks
      class << self
        def check_for_pending_migrations
          # no op
          # we just want to load the structure file, we don't care if the dev db has not run all the migrations yet.
          # if the structure file is missing migrations, it will still error as of rails 4.1
        end
      end
    end
  end
end
@JuanitoFatas

This comment has been minimized.

Show comment
Hide comment
@JuanitoFatas

JuanitoFatas May 14, 2014

Contributor

How to remove the WARNING?

WARNING: db:test:prepare is deprecated. The Rails test helper now maintains your test schema automatically, see the release notes for details.

Do not use rake db:test:prepare ? Thanks.

Contributor

JuanitoFatas commented May 14, 2014

How to remove the WARNING?

WARNING: db:test:prepare is deprecated. The Rails test helper now maintains your test schema automatically, see the release notes for details.

Do not use rake db:test:prepare ? Thanks.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 14, 2014

Member

Yes. it should not be used anymore.

Member

rafaelfranca commented May 14, 2014

Yes. it should not be used anymore.

@JuanitoFatas

This comment has been minimized.

Show comment
Hide comment
@JuanitoFatas

JuanitoFatas May 14, 2014

Contributor

Thank you @rafaelfranca.

Contributor

JuanitoFatas commented May 14, 2014

Thank you @rafaelfranca.

@mediafinger

This comment has been minimized.

Show comment
Hide comment
@mediafinger

mediafinger May 19, 2014

Thx for deprecating rake db:test:prepare! ❤️

mediafinger commented May 19, 2014

Thx for deprecating rake db:test:prepare! ❤️

@talsafran

This comment has been minimized.

Show comment
Hide comment
@talsafran

talsafran Jun 6, 2014

If I create a migration and migrate it:

class CreateCookPhotos < ActiveRecord::Migration
  def change
    create_table :cook_photos do |t|
      t.integer :cook_id
      t.string :url

      t.timestamps
    end
  end
end

And then roll it back because I want to add or change something in the migration (say change the name of the url column to filename)

Then the test database doesn't seem to catch the change.

To alleviate this, I have to:

rake db:rollback RAILS_ENV=test
rake db:migrate RAILS_ENV=test

This is more work than just copying the schema over with something like rake db:test:prepare. Anything I could do to simplify this?

talsafran commented Jun 6, 2014

If I create a migration and migrate it:

class CreateCookPhotos < ActiveRecord::Migration
  def change
    create_table :cook_photos do |t|
      t.integer :cook_id
      t.string :url

      t.timestamps
    end
  end
end

And then roll it back because I want to add or change something in the migration (say change the name of the url column to filename)

Then the test database doesn't seem to catch the change.

To alleviate this, I have to:

rake db:rollback RAILS_ENV=test
rake db:migrate RAILS_ENV=test

This is more work than just copying the schema over with something like rake db:test:prepare. Anything I could do to simplify this?

@@ -389,6 +389,19 @@ def check_pending!
raise ActiveRecord::PendingMigrationError if ActiveRecord::Migrator.needs_migration?
end
def load_schema_if_pending!
if ActiveRecord::Migrator.needs_migration?

This comment has been minimized.

@spastorino

spastorino Aug 26, 2014

Member

seems like a ActiveRecord::Tasks::DatabaseTasks.purge is missing here. /cc @jonleighton. See #15787 Is what we were doing under test namespace

@spastorino

spastorino Aug 26, 2014

Member

seems like a ActiveRecord::Tasks::DatabaseTasks.purge is missing here. /cc @jonleighton. See #15787 Is what we were doing under test namespace

This comment has been minimized.

@spastorino

spastorino Aug 26, 2014

Member

So seems like it's already fixed #15394

@spastorino

spastorino Aug 26, 2014

Member

So seems like it's already fixed #15394

@GeekOnCoffee

This comment has been minimized.

Show comment
Hide comment
@GeekOnCoffee

GeekOnCoffee Oct 20, 2014

Any suggestions on how to hook into this behavior? I've read through it a few times looking for a good integration point but haven't found anywhere... looking to reseed when the test database is rebuilt, as we have seed data that's required for tests to pass

GeekOnCoffee commented Oct 20, 2014

Any suggestions on how to hook into this behavior? I've read through it a few times looking for a good integration point but haven't found anywhere... looking to reseed when the test database is rebuilt, as we have seed data that's required for tests to pass

senny added a commit that referenced this pull request Nov 24, 2014

bring back `db:test:prepare`.
This reverts deprecations added in #13528.
The task is brought back for two reasons:
  1. Give plugins a way to hook into the test database initialization process
  2. Give the user a way to force a test database synchronization

While `test:prepare` is still a dependency of every test task, `db:test:prepare`
no longer hooks into it. This means that `test:prepare` runs before the schema
is synchronized. Plugins, which insert data can now hook into `db:test:prepare`.

The automatic schema maintenance can't detect when a migration is rolled-back,
modified and reapplied. In this case the user has to fall back to `db:test:prepare`
to force the synchronization to happen.

@senny senny referenced this pull request Nov 24, 2014

Merged

bring back `db:test:prepare`. #17739

3 of 3 tasks complete

senny added a commit that referenced this pull request Nov 25, 2014

bring back `db:test:prepare`.
This reverts deprecations added in #13528.
The task is brought back for two reasons:
  1. Give plugins a way to hook into the test database initialization process
  2. Give the user a way to force a test database synchronization

While `test:prepare` is still a dependency of every test task, `db:test:prepare`
no longer hooks into it. This means that `test:prepare` runs before the schema
is synchronized. Plugins, which insert data can now hook into `db:test:prepare`.

The automatic schema maintenance can't detect when a migration is rolled-back,
modified and reapplied. In this case the user has to fall back to `db:test:prepare`
to force the synchronization to happen.
@curtislinden

This comment has been minimized.

Show comment
Hide comment
@curtislinden

curtislinden Apr 8, 2015

I'm wondering on the status of this? I'm looking for a way to maintain a test rails env that isn't called 'test'

curtislinden commented Apr 8, 2015

I'm wondering on the status of this? I'm looking for a way to maintain a test rails env that isn't called 'test'

@xhoy xhoy referenced this pull request Jun 23, 2015

Closed

Missing documentation #20671

maclover7 added a commit to maclover7/rails that referenced this pull request Dec 5, 2015

Remove unused deprecation notice
The `rake db:test:*` tasks were deprecated in #13528, but were
undeprecated and added back in via #17739.

@kkelleey kkelleey referenced this pull request Jul 28, 2016

Closed

New setup script #247

hennevogel added a commit to hennevogel/open-build-service that referenced this pull request Aug 22, 2016

[dist] Change the call to start backends
We can't exec start_test_backend from inside script/ as this
clashes with rails/rails#13528 somehow.

bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Aug 24, 2016

[dist] Change the call to start backends
We can't exec start_test_backend from inside script/ as this
clashes with rails/rails#13528 somehow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment