Refactor tests to be less brittle

Author: jonleighton

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -538,15 +538,27 @@ def test_adding_a_collection
   end
 
   def test_transactions_when_adding_to_persisted
-    force_signal37_to_load_all_clients_of_firm
-    Client.expects(:transaction)
-    companies(:first_firm).clients_of_firm.concat(Client.new("name" => "Natural Company"))
+    good = Client.new(:name => "Good")
+    bad  = Client.new(:name => "Bad", :raise_on_save => true)
+
+    begin
+      companies(:first_firm).clients_of_firm.concat(good, bad)
+    rescue Client::RaisedOnSave
+    end
+
+    assert !companies(:first_firm).clients_of_firm(true).include?(good)
   end
 
   def test_transactions_when_adding_to_new_record
-    Client.expects(:transaction).never
-    firm = Firm.new
-    firm.clients_of_firm.concat(Client.new("name" => "Natural Company"))
+    prev_ignored_sql = ActiveRecord::SQLCounter.ignored_sql
+    ActiveRecord::SQLCounter.ignored_sql = []
+
+    assert_no_queries do
+      firm = Firm.new
+      firm.clients_of_firm.concat(Client.new("name" => "Natural Company"))
+    end
+  ensure
+    ActiveRecord::SQLCounter.ignored_sql = prev_ignored_sql
   end
 
   def test_new_aliased_to_build
@@ -791,18 +803,31 @@ def test_delete_all_with_not_yet_loaded_association_collection
   end
 
   def test_transaction_when_deleting_persisted
-    force_signal37_to_load_all_clients_of_firm
-    client = companies(:first_firm).clients_of_firm.create("name" => "Another Client")
-    Client.expects(:transaction)
-    companies(:first_firm).clients_of_firm.delete(client)
+    good = Client.new(:name => "Good")
+    bad  = Client.new(:name => "Bad", :raise_on_destroy => true)
+
+    companies(:first_firm).clients_of_firm = [good, bad]
+
+    begin
+      companies(:first_firm).clients_of_firm.destroy(good, bad)
+    rescue Client::RaisedOnDestroy
+    end
+
+    assert_equal [good, bad], companies(:first_firm).clients_of_firm(true)
   end
 
   def test_transaction_when_deleting_new_record
-    client = Client.new("name" => "New Client")
-    firm = Firm.new
-    firm.clients_of_firm << client
-    Client.expects(:transaction).never
-    firm.clients_of_firm.delete(client)
+    prev_ignored_sql = ActiveRecord::SQLCounter.ignored_sql
+    ActiveRecord::SQLCounter.ignored_sql = []
+
+    assert_no_queries do
+      firm = Firm.new
+      client = Client.new("name" => "New Client")
+      firm.clients_of_firm << client
+      firm.clients_of_firm.destroy(client)
+    end
+  ensure
+    ActiveRecord::SQLCounter.ignored_sql = prev_ignored_sql
   end
 
   def test_clearing_an_association_collection
@@ -1139,21 +1164,29 @@ def test_replace_failure
   end
 
   def test_transactions_when_replacing_on_persisted
-    firm = Firm.find(:first, :order => "id")
-    firm.clients = [companies(:first_client)]
-    assert firm.save, "Could not save firm"
-    firm.reload
+    good = Client.new(:name => "Good")
+    bad  = Client.new(:name => "Bad", :raise_on_save => true)
 
-    Client.expects(:transaction)
-    firm.clients_of_firm = [Client.new("name" => "Natural Company")]
+    companies(:first_firm).clients_of_firm = [good]
+
+    begin
+      companies(:first_firm).clients_of_firm = [bad]
+    rescue Client::RaisedOnSave
+    end
+
+    assert_equal [good], companies(:first_firm).clients_of_firm(true)
   end
 
   def test_transactions_when_replacing_on_new_record
-    firm = Firm.new
-    firm.clients_of_firm << Client.new("name" => "Natural Company")
+    prev_ignored_sql = ActiveRecord::SQLCounter.ignored_sql
+    ActiveRecord::SQLCounter.ignored_sql = []
 
-    Client.expects(:transaction).never
-    firm.clients_of_firm = [Client.new("name" => "New Client")]
+    assert_no_queries do
+      firm = Firm.new
+      firm.clients_of_firm = [Client.new("name" => "New Client")]
+    end
+  ensure
+    ActiveRecord::SQLCounter.ignored_sql = prev_ignored_sql
   end
 
   def test_get_ids

activerecord/test/cases/helper.rb

@@ -57,11 +57,12 @@ def with_active_record_default_timezone(zone)
 
 module ActiveRecord
   class SQLCounter
-    IGNORED_SQL = [/^PRAGMA (?!(table_info))/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/, /^SHOW max_identifier_length/, /^BEGIN/, /^COMMIT/]
+    cattr_accessor :ignored_sql
+    self.ignored_sql = [/^PRAGMA (?!(table_info))/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/, /^SHOW max_identifier_length/, /^BEGIN/, /^COMMIT/]
 
     # FIXME: this needs to be refactored so specific database can add their own
     # ignored SQL.  This ignored SQL is for Oracle.
-    IGNORED_SQL.concat [/^select .*nextval/i, /^SAVEPOINT/, /^ROLLBACK TO/, /^\s*select .* from all_triggers/im]
+    ignored_sql.concat [/^select .*nextval/i, /^SAVEPOINT/, /^ROLLBACK TO/, /^\s*select .* from all_triggers/im]
 
     def initialize
       $queries_executed = []
@@ -73,7 +74,7 @@ def call(name, start, finish, message_id, values)
       # FIXME: this seems bad. we should probably have a better way to indicate
       # the query was cached
       unless 'CACHE' == values[:name]
-        $queries_executed << sql unless IGNORED_SQL.any? { |r| sql =~ r }
+        $queries_executed << sql unless self.class.ignored_sql.any? { |r| sql =~ r }
       end
     end
   end

activerecord/test/models/company.rb

@@ -124,6 +124,18 @@ class Client < Company
   has_many :accounts, :through => :firm
   belongs_to :account
 
+  class RaisedOnSave < RuntimeError; end
+  attr_accessor :raise_on_save
+  before_save do
+    raise RaisedOnSave if raise_on_save
+  end
+
+  class RaisedOnDestroy < RuntimeError; end
+  attr_accessor :raise_on_destroy
+  before_destroy do
+    raise RaisedOnDestroy if raise_on_destroy
+  end
+
   # Record destruction so we can test whether firm.clients.clear has
   # is calling client.destroy, deleting from the database, or setting
   # foreign keys to NULL.