[PATCH] Unit Testing teardown doesn't clean up tests, causing FK constraint violations on next test

creates nuke_fixtures and adds it to teardown

bumping up so commiters notice

Have you seen transactional fixtures? In most cases, rollbacks are a nicer and far faster way to isolate your tests.

class FooTest < Test::Unit::TestCase
  self.use_transactional_fixtures = true
end

Regarding the patch implementation, I'd prefer to nuke once at the beginning of the test run then insert on setup / nuke on teardown rather than nuke+insert on setup / nuke on teardown. The additional database traffic (another nuke per test method) could really hurt.

And, sorry, we don't support REAL db ;-)

I've been using the following setup to work around the same problem.

1) In my Rakefile, :clone_structure_to_test now runs psql loading db/create.sql which only drops and creates the tables. No data is loaded (I have a separate db/insert.sql for that).

2) Add the following mixin to all test classes.

module DeleteFixturesInTeardownMixin

  def teardown
    self.class.fixture_table_names.reverse.each do |table_name|
      klass_name = Inflector.classify(table_name.to_s)
      if Object.const_defined?(klass_name)
        klass = Object.const_get(klass_name)
        klass.connection.delete("DELETE FROM #{table_name}", 'Fixture Delete')
      else
        flunk("Cannot find class for table '#{table_name}' to delete fixtures")
      end
    end
  end

end

Taking Jeremy advice, I changed the teardown method name here to xteardown so my teardown mixin wouldn't be called, and added self.use_transactional_fixtures = true to all classes, but I then get test failures with lots of

EWARNING: there is no transaction in progress EWARNING: there is no transaction in progress

and get the same test failures ActiveRecord::StatementInvalid: ERROR: update or delete on violates foreign key constraint

So this sounds like I'm your setup Jeremy. Anything different you do?

Regarding the patch, it assumes that all connections are through ActiveRecord::Base, which isn't the case. I have a setup where I use Postgresql and Sqlite. This mixin here gets the ActiveRecord object for the class from the table name and uses its connection to destroy the mixins.

And is there a reason why the teardown deletes is run in a transaction?

Regards,
Blair

This is NOT an enhancement. This is a bug in ActiveRecord not considering the implications of foreign keys. If it were an enhancement, then the current code would work just fine in the scenario I originally gave. If it worked but was quirky, that'd be one thing, but tests fail, and not because of any other reason than AR.

"Regarding the patch implementation, I'd prefer to nuke once at the beginning of the test run then insert on setup / nuke on teardown rather than nuke+insert on setup / nuke on teardown. The additional database traffic (another nuke per test method) could really hurt."

I have no vested interest either way as long as it works.

Pull into release candidate.

Changeset [2714].

Rolled back in [2730]. The solution gives the behavior you expect but sacrifices test speed since all loaded fixtures must be wiped between TestCases. Folks seem to prefer the speedy testing with the caveat that you have to watch out for dirty tables if you have foreign key constraints.

Testing + fixtures will be revisited post-1.0 to correct this issue while retaining test speed (tsort the tables by fkey dependency.) In the meantime, please declare fixtures that depend on others even if you don't use them directly.

Oh. my. god. How much was it slowing down testing? Was it measured? Did anyone even take another stab at an alternative faster-but-still-correct solution? Or did you guys decide that good QA/testing practices were simply secondary to shiny features (especially when it comes to that dreaded foreign key stuff that you all seem to hate so much)? Declaring all my fixtures in every testcase goes completely against the grain of well established unit testing procedures. Not only that, but it potentially hides bugs (eg, if a model isn't supposed to interact with another).

1. Up to twice as slow 2. Yup 3. Yes, I put considerable effort into it 4. No. You seem to have tunnel vision on DHH

I agree that having to declare essentially all of your fixtures for every test case sucks. Fixtures weren't designed with foreign key constraints in mind (or transactions, or ..., hence the status as 'relic'). I have a better implementation but despite my best efforts I was not able to make it fully backward compatible, so it is unacceptable for 1.0.

I recommend, as I have many others, that you keep a separate database loaded with your 'pristine' fixtures then, instead of the normal rake test dependencies, clone this database to your test db. Since your tests are running in rolled-back transactions, the test database is still pristine following your test run and doesn't need to be cloned again until your schema or fixtures change. Furthermore, you needn't declare fixtures in your test cases. I think this approach is better suited for anyone testing against a database with foreign key constraints.

I am open to your improvements. In your dreams, how could we make your app a joy to test?

I assumed that use_transactional_fixtures=true would create my fixtures in a transaction and roll them back after my test(s) finished. While I agree that the actual behavior (leaving fixture data in the database), makes sense, I think it's not intuitive and not well-documented. Sounds like I'm not the only person to trip on this. Could we update the fixture documentation to say something like:

'use_transactional_fixtures=true' will rollback all database changes after each test. However, fixture data are deleted, inserted, and committed. In other words, fixture data remains in the test database after each test.

use_transactional_fixtures is kind misleading. It's more like use_transactional_tests....

We have not only foreign key constraints, but also deletion constraints and triggers on update and delete to do logging for auditing purposes. We're basically unable to delete anything that goes in, and everything's order dependent, and, of course, we can't have double-inserts when loading fixtures. The normal fixture setup in Rails would wreak havoc when testing against our database schema. We (on both Oracle and Postgres) have a script which creates a full creation script for a blank db (as well as data loading for production installs, as well as teardown for those times when it's necesary -- we'll move to migrations once we're satisfied that all our whack experimentation with the db is over, which is near). We've overridden the rake tasks to drop the db and load our schema script. So, when we begin testing we start from a ready database with no data in it.

When we test we have the necessary fixtures listed for each test (not all fixtures for all tests -- why list them at all if you're going to always list them all?). We've added the following code to the test_helper.rb (this is an update to similar code posted elsewhere):

# neuter fixture deletions and multiple fixture insertions to work with database constraints
class Fixture
  attr_reader :class_name
end 
  
class Fixtures
  @@inserted_fixture_list ||= {}
  alias :original_insert_fixtures :insert_fixtures
      
  def insert_fixtures
    return if @@inserted_fixture_list[values[0].class_name]
    @@inserted_fixture_list[values[0].class_name] = true
    unless ActiveRecord::Base.connection.select_one("select 1 from #{fixture_class_to_table_name(values[0].class_name)}")
      original_insert_fixtures
    end
  end
    
  def delete_existing_fixtures() end

  # compute a fixture's table name from its (known) class name
  def fixture_class_to_table_name(class_name)
    class_name.gsub(/([a-z])([A-Z])/) { |match| "#{$1}_#{$2}" }.downcase
  end
end

We set:

  self.use_transactional_fixtures = true
  self.use_instantiated_fixtures  = false

We don't have any trouble running our unit and functional tests now on either Oracle or Postgres.

Here's the simple way I fix the dirty table problem:

run this: psql blah blah < db/create.sql before this: ruby test/unit/blah.rb

If the solution is this simple on the command line, why should it be more complicated in our test models? Why don't we add something like this to the test classes:

prepare_database :yes, :script => 'db/create.sql' (with defaults of no, db/create.sql)

This way, most users wouldn't notice any new slowness, and people who are aware of these problems can just switch the functionality on. Most of our users are probably using create.sql already, and they would probably be very comfortable using this feature. Another alternative would be to automate this behavior with rake before each test right? If it's done outside of each test script, it wouldn't factor so much into the timing/performance results.

I'm afraid I have to agree with Ryan on this. The speed of unit tests should be (mostly) irrelevant. A 2x speed increase is not worth removing features. If your tests are that slow you have too much data in your fixtures. Maintaining a separate database is too annoying and painful. Please give us back support for FK's in our tests!

I agree with Ryan as well. Unit testing is important. Foreign key constraints are important. Please please please support them together (at least optionally). I'll take the performance hit.

Thank you Rick for your code. Clean and easy to use. I have modified the way you resolve a class' name because it didn't worked as is for me.

# neuter fixture deletions and multiple fixture insertions to work with database constraints
class Fixture
  attr_reader :class_name
end 
  
class Fixtures
  @@inserted_fixture_list ||= {}
  alias :original_insert_fixtures :insert_fixtures
      
  def insert_fixtures
    return if @@inserted_fixture_list[values[0].class_name]
    @@inserted_fixture_list[values[0].class_name] = true
    unless ActiveRecord::Base.connection.select_one("select 1 from #{fixture_class_to_table_name(values[0].class_name)}")
      original_insert_fixtures
    end
  end
    
  def delete_existing_fixtures() end

  # compute a fixture's table name from its (known) class name
  def fixture_class_to_table_name(class_name)
    Inflector::tableize(class_name)
  end
end

I fell on the same problem as Ryan, and I think deleting fixtures after each test is the right way to solve the problem, since then you can be sure you left the db the same as it was before, and you know you arent going to break any other tests.

As to the performance issue, couldnt you simply introduce a class variable like self.delete_fixtures_on_teardown which would default to false, thus performance should be pretty much the same by default for users who dont want to use this feature.

I had a look at the code, and to do this nicely would require a bit of refactoring. Wasnt brave enough to give it a go...

Won't following solve the problem:

1. erase fixtures once when the test is initialized 2. load fixtures on each setup 3. erase them on each teardown

Replying to bitsweat: This solution worked well for me, except that sometimes we have to use a different table name than the fixture name.

I'm using:

class Fixtures

@@inserted_fixture_list = {} alias original_insert_fixtures insert_fixtures

def insert_fixtures

return if @@inserted_fixture_list[values[0].class_name] @@inserted_fixture_list[values[0].class_name] = true class_name = values[0].class_name if class_name.respond_to? "constantize"

table_name = values[0].class_name.constantize.table_name

else

table_name = values[0].class_name.table_name

end unless ActiveRecord::Base.connection.select_one("select 1 from #{table_name}")

original_insert_fixtures

end

end

def delete_existing_fixtures() end

end

Alisdair McDiarmid posted some code here:

http://blog.caboo.se/articles/2006/05/01/are-foreign-keys-worth-your-time

I put this in test_helper.rb:

class Fixtures
  # Oh for alias_method_chain
  alias :original_delete_existing_fixtures :delete_existing_fixtures
  alias :original_insert_fixtures :insert_fixtures

  def delete_existing_fixtures
    @connection.update "SET FOREIGN_KEY_CHECKS = 0", 'Fixtures deactivate foreign key checks.';
    original_delete_existing_fixtures
    @connection.update "SET FOREIGN_KEY_CHECKS = 1", 'Fixtures activate foreign key checks.';
  end

  def insert_fixtures
    @connection.update "SET FOREIGN_KEY_CHECKS = 0", 'Fixtures deactivate foreign key checks.';
    original_insert_fixtures
    @connection.update "SET FOREIGN_KEY_CHECKS = 1", 'Fixtures activate foreign key checks.';
  end
end

I don't understand why, if fixtures are 'relics', it was so incredibly important to revert a perfectly-good fix in order to speed them, at the cost of leaving them broken again. I certainly agree with others who have said that *at least optionally* I'd like the ability to have my tests tear down unwanted fixtures so that by the time my tests run, only the fixtures specified by the test exist.

Thanks a lot for your code, Rick. Sadly it seems I can't put it somewhere else than into test_helper.rb; I have a small plugin with "everyday codes" that I use for every project, so it would be great if one could add it somehow to a plugin...?

this patch is compatible with rails 1.2.3--2.01 .

idea: let the fixtures insertion participate in the same transaction with the every running test

module Test #:nodoc:
  module Unit #:nodoc:
    class TestCase #:nodoc:
      alias_method :old_setup_with_fixtures, :setup_with_fixtures unless method_defined?(:old_setup_with_fixtures)
      alias_method :old_teardown_with_fixtures, :teardown_with_fixtures unless method_defined?(:old_teardown_with_fixtures)
      def setup_with_fixtures
        if use_transactional_fixtures?

          ActiveRecord::Base.send :increment_open_transactions
          ActiveRecord::Base.connection.begin_db_transaction
          close_original_activerecord_transaction_methods
        end
        old_setup_with_fixtures
        if use_transactional_fixtures?
          open_original_activerecord_transaction_methods
        end
        
      
      end
      
      def teardown_with_fixtures
        if use_transactional_fixtures?
        	Fixtures.send :reset_cache  if Fixtures.respond_to?(:reset_cache)
          clear_fixtures_states_when_use_transactional_fixtures
        end
        old_teardown_with_fixtures       
      end      
      #prevent the next code:alias_method from trigger invoking the self.method_added introspected method
      class<<TestCase
        alias old_method_added method_added
        def method_added(m) 
          #do nothing
        end
      end

      alias_method :setup,:setup_with_fixtures
      alias_method :teardown,:teardown_with_fixtures
      #reopen the introspector class method:method_added
      class<<TestCase
        alias method_added old_method_added
      		
      end
      
      private
      def clear_fixtures_states_when_use_transactional_fixtures
        @@already_loaded_fixtures.clear if @@already_loaded_fixtures
        @loaded_fixtures.clear if  @loaded_fixtures

      end
      def close_original_activerecord_transaction_methods
        self.class.class_eval(%Q[
          	class<<ActiveRecord::Base
          		alias old_increment_open_transactions increment_open_transactions
          		alias old_decrement_open_transactions decrement_open_transactions
          		def increment_open_transactions
          			#do nothing
      			end
      			def decrement_open_transactions
          			#do nothing
      			end
      		end
          ])
        ActiveRecord::Base.connection.class.class_eval(%Q[
          	alias_method :old_begin_db_transaction,:begin_db_transaction
          	alias_method :old_commit_db_transaction,:commit_db_transaction
          	def begin_db_transaction
          		#do nothing
      		end
      		def commit_db_transaction
          		#do nothing
      		end
          ])
        
      end
      
      def open_original_activerecord_transaction_methods
        self.class.class_eval(%Q[
          	class<<ActiveRecord::Base
          		alias increment_open_transactions old_increment_open_transactions
          		alias decrement_open_transactions old_decrement_open_transactions
      		end
          ])
        ActiveRecord::Base.connection.class.class_eval(%Q[
          	alias_method :begin_db_transaction,:old_begin_db_transaction
          	alias_method :commit_db_transaction,:old_commit_db_transaction
          	
          ])
      end
    end
  end
end

Does anyone have a fix for this compatible with Rails 2.1.0? I had to upgrade because of the ruby exploit/bug and now testing is dead.