From a6512cbd6ca0e120bb2f314ceb88a44724e15f22 Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 15 Jan 2014 11:30:55 +0100 Subject: [PATCH 1/6] no need to `return skip` in tests. `skip` is enough. --- activerecord/test/cases/adapters/postgresql/json_test.rb | 2 +- activerecord/test/cases/adapters/postgresql/range_test.rb | 2 +- activerecord/test/cases/adapters/postgresql/xml_test.rb | 2 +- activerecord/test/cases/base_test.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/activerecord/test/cases/adapters/postgresql/json_test.rb b/activerecord/test/cases/adapters/postgresql/json_test.rb index 01e7334aad..3daef399d8 100644 --- a/activerecord/test/cases/adapters/postgresql/json_test.rb +++ b/activerecord/test/cases/adapters/postgresql/json_test.rb @@ -21,7 +21,7 @@ class PostgresqlJSONTest < ActiveRecord::TestCase end end rescue ActiveRecord::StatementInvalid - return skip "do not test on PG without json" + skip "do not test on PG without json" end @column = JsonDataType.columns.find { |c| c.name == 'payload' } end diff --git a/activerecord/test/cases/adapters/postgresql/range_test.rb b/activerecord/test/cases/adapters/postgresql/range_test.rb index a56b8ac791..5c2d8e1c1d 100644 --- a/activerecord/test/cases/adapters/postgresql/range_test.rb +++ b/activerecord/test/cases/adapters/postgresql/range_test.rb @@ -26,7 +26,7 @@ if ActiveRecord::Base.connection.supports_ranges? end end rescue ActiveRecord::StatementInvalid - return skip "do not test on PG without range" + skip "do not test on PG without range" end insert_range(id: 101, diff --git a/activerecord/test/cases/adapters/postgresql/xml_test.rb b/activerecord/test/cases/adapters/postgresql/xml_test.rb index bf14b378d8..dd2a727afe 100644 --- a/activerecord/test/cases/adapters/postgresql/xml_test.rb +++ b/activerecord/test/cases/adapters/postgresql/xml_test.rb @@ -18,7 +18,7 @@ class PostgresqlXMLTest < ActiveRecord::TestCase end end rescue ActiveRecord::StatementInvalid - return skip "do not test on PG without xml" + skip "do not test on PG without xml" end @column = XmlDataType.columns.find { |c| c.name == 'payload' } end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 4bc6002bfe..2ec8a2f1e6 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1129,7 +1129,7 @@ class BasicsTest < ActiveRecord::TestCase k = Class.new(ak) k.table_name = "projects" orig_name = k.sequence_name - return skip "sequences not supported by db" unless orig_name + skip "sequences not supported by db" unless orig_name assert_equal k.reset_sequence_name, orig_name end From 8e1735e26c378b1ffa8f82ea00f1f498d39c7e9e Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 15 Jan 2014 11:42:26 +0100 Subject: [PATCH 2/6] tests without transactional fixtures need to cleanup afterwards. This fixes test order issues. --- activerecord/test/cases/autosave_association_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 517d2674a7..fe5de44409 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -574,6 +574,13 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase @ship = @pirate.create_ship(:name => 'Nights Dirty Lightning') end + def teardown + # We are running without transactional fixtures and need to cleanup. + Bird.delete_all + @ship.delete + @pirate.delete + end + # reload def test_a_marked_for_destruction_record_should_not_be_be_marked_after_reload @pirate.mark_for_destruction From f00524375ddefba587bb32e505de5787f0fae695 Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 15 Jan 2014 14:08:45 +0100 Subject: [PATCH 3/6] use `teardown` for cleanup, not `setup`. This solves order dependent issues where the last test leaked the query cache state. This resulted in the following error: ``` QueryCacheExpiryTest#test_cache_is_expired_by_habtm_delete [test/cases/query_cache_test.rb:275]: not all expectations were satisfied unsatisfied expectations: - expected exactly twice, invoked once: #.clear_query_cache(any_parameters) ``` --- activerecord/test/cases/query_cache_test.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index 5566563116..da8ae672fe 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -8,7 +8,7 @@ require 'rack' class QueryCacheTest < ActiveRecord::TestCase fixtures :tasks, :topics, :categories, :posts, :categories_posts - def setup + teardown do Task.connection.clear_query_cache ActiveRecord::Base.connection.disable_query_cache! end @@ -214,7 +214,7 @@ class QueryCacheExpiryTest < ActiveRecord::TestCase Post.find(1) # change the column definition - Post.connection.change_column :posts, :title, :string, :limit => 80 + Post.connection.change_column :posts, :title, :string, limit: 80 assert_nothing_raised { Post.find(1) } # restore the old definition @@ -241,7 +241,6 @@ class QueryCacheExpiryTest < ActiveRecord::TestCase def test_update Task.connection.expects(:clear_query_cache).times(2) - Task.cache do task = Task.find(1) task.starting = Time.now.utc @@ -251,7 +250,6 @@ class QueryCacheExpiryTest < ActiveRecord::TestCase def test_destroy Task.connection.expects(:clear_query_cache).times(2) - Task.cache do Task.find(1).destroy end @@ -259,7 +257,6 @@ class QueryCacheExpiryTest < ActiveRecord::TestCase def test_insert ActiveRecord::Base.connection.expects(:clear_query_cache).times(2) - Task.cache do Task.create! end From a7139008399718f7152d7a5485b106c7ba79bca2 Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 15 Jan 2014 20:33:37 +0100 Subject: [PATCH 4/6] isolate class attribute assignment in `migration_test.rb` This makes the tests inside `migration_test.rb` order independent. The assignments to `Reminder.table_name_prefix` and `Reminder.table_name_suffix` previously leaked and had impact on following test cases. This patch isolates the assignments on a throw-away subclass. --- activerecord/test/cases/migration_test.rb | 55 ++++++++++++++--------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 0363bf1048..f2e728146d 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -328,6 +328,7 @@ class MigrationTest < ActiveRecord::TestCase end def test_proper_table_name_on_migrator + reminder_class = new_isolated_reminder_class assert_deprecated do assert_equal "table", ActiveRecord::Migrator.proper_table_name('table') end @@ -335,30 +336,30 @@ class MigrationTest < ActiveRecord::TestCase assert_equal "table", ActiveRecord::Migrator.proper_table_name(:table) end assert_deprecated do - assert_equal "reminders", ActiveRecord::Migrator.proper_table_name(Reminder) + assert_equal "reminders", ActiveRecord::Migrator.proper_table_name(reminder_class) end - Reminder.reset_table_name + reminder_class.reset_table_name assert_deprecated do - assert_equal Reminder.table_name, ActiveRecord::Migrator.proper_table_name(Reminder) + assert_equal reminder_class.table_name, ActiveRecord::Migrator.proper_table_name(reminder_class) end # Use the model's own prefix/suffix if a model is given ActiveRecord::Base.table_name_prefix = "ARprefix_" ActiveRecord::Base.table_name_suffix = "_ARsuffix" - Reminder.table_name_prefix = 'prefix_' - Reminder.table_name_suffix = '_suffix' - Reminder.reset_table_name + reminder_class.table_name_prefix = 'prefix_' + reminder_class.table_name_suffix = '_suffix' + reminder_class.reset_table_name assert_deprecated do - assert_equal "prefix_reminders_suffix", ActiveRecord::Migrator.proper_table_name(Reminder) + assert_equal "prefix_reminders_suffix", ActiveRecord::Migrator.proper_table_name(reminder_class) end - Reminder.table_name_prefix = '' - Reminder.table_name_suffix = '' - Reminder.reset_table_name + reminder_class.table_name_prefix = '' + reminder_class.table_name_suffix = '' + reminder_class.reset_table_name # Use AR::Base's prefix/suffix if string or symbol is given ActiveRecord::Base.table_name_prefix = "prefix_" ActiveRecord::Base.table_name_suffix = "_suffix" - Reminder.reset_table_name + reminder_class.reset_table_name assert_deprecated do assert_equal "prefix_table_suffix", ActiveRecord::Migrator.proper_table_name('table') end @@ -368,28 +369,29 @@ class MigrationTest < ActiveRecord::TestCase end def test_proper_table_name_on_migration + reminder_class = new_isolated_reminder_class migration = ActiveRecord::Migration.new assert_equal "table", migration.proper_table_name('table') assert_equal "table", migration.proper_table_name(:table) - assert_equal "reminders", migration.proper_table_name(Reminder) - Reminder.reset_table_name - assert_equal Reminder.table_name, migration.proper_table_name(Reminder) + assert_equal "reminders", migration.proper_table_name(reminder_class) + reminder_class.reset_table_name + assert_equal reminder_class.table_name, migration.proper_table_name(reminder_class) # Use the model's own prefix/suffix if a model is given ActiveRecord::Base.table_name_prefix = "ARprefix_" ActiveRecord::Base.table_name_suffix = "_ARsuffix" - Reminder.table_name_prefix = 'prefix_' - Reminder.table_name_suffix = '_suffix' - Reminder.reset_table_name - assert_equal "prefix_reminders_suffix", migration.proper_table_name(Reminder) - Reminder.table_name_prefix = '' - Reminder.table_name_suffix = '' - Reminder.reset_table_name + reminder_class.table_name_prefix = 'prefix_' + reminder_class.table_name_suffix = '_suffix' + reminder_class.reset_table_name + assert_equal "prefix_reminders_suffix", migration.proper_table_name(reminder_class) + reminder_class.table_name_prefix = '' + reminder_class.table_name_suffix = '' + reminder_class.reset_table_name # Use AR::Base's prefix/suffix if string or symbol is given ActiveRecord::Base.table_name_prefix = "prefix_" ActiveRecord::Base.table_name_suffix = "_suffix" - Reminder.reset_table_name + reminder_class.reset_table_name assert_equal "prefix_table_suffix", migration.proper_table_name('table', migration.table_name_options) assert_equal "prefix_table_suffix", migration.proper_table_name(:table, migration.table_name_options) end @@ -538,6 +540,15 @@ class MigrationTest < ActiveRecord::TestCase ensure old_tz ? ENV['TZ'] = old_tz : ENV.delete('TZ') end + + # This is needed to isolate class_attribute assignments like `table_name_prefix` + # for each test case. + def new_isolated_reminder_class + Class.new(Reminder) { + def self.name; "Reminder"; end + def self.base_class; self; end + } + end end class ReservedWordsMigrationTest < ActiveRecord::TestCase From fca8b0318efb88a8fafe768443f05a99967a6489 Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 15 Jan 2014 20:34:53 +0100 Subject: [PATCH 5/6] we have `with_env_tz` as global test helper. Remove duplicate. --- activerecord/test/cases/finder_test.rb | 7 ------- activerecord/test/cases/migration_test.rb | 7 ------- 2 files changed, 14 deletions(-) diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 5125d5df2a..9b575557de 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -891,11 +891,4 @@ class FinderTest < ActiveRecord::TestCase ActiveRecord::Base.send(:replace_bind_variables, statement, vars) end end - - def with_env_tz(new_tz = 'US/Eastern') - old_tz, ENV['TZ'] = ENV['TZ'], new_tz - yield - ensure - old_tz ? ENV['TZ'] = old_tz : ENV.delete('TZ') - end end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index f2e728146d..1bda472d23 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -534,13 +534,6 @@ class MigrationTest < ActiveRecord::TestCase end protected - def with_env_tz(new_tz = 'US/Eastern') - old_tz, ENV['TZ'] = ENV['TZ'], new_tz - yield - ensure - old_tz ? ENV['TZ'] = old_tz : ENV.delete('TZ') - end - # This is needed to isolate class_attribute assignments like `table_name_prefix` # for each test case. def new_isolated_reminder_class From 6eab530429ed7573550931b259759168065ac0af Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Wed, 15 Jan 2014 20:53:05 +0100 Subject: [PATCH 6/6] reset column information after fiddling with `Encoding.default_internal` This solves order depndent problems in `base_test.rb`. The leaked column information crashed test cases using the `Weird` class later on: ``` 2) Error: BasicsTest#test_group_weirds_by_from: SyntaxError: /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:387: invalid hex escape ... attribute_before_type_cast("\x{A4CA}\x{A4DE}\x{A4A8}", *arg... ... ^ /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:387: invalid hex escape ...te_before_type_cast("\x{A4CA}\x{A4DE}\x{A4A8}", *args) ... ^ /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:387: invalid hex escape ...e_type_cast("\x{A4CA}\x{A4DE}\x{A4A8}", *args) ... ^ /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:385:in `module_eval' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:385:in `define_proxy_call' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:294:in `block in define_attribute_method' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:285:in `each' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:285:in `define_attribute_method' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:252:in `block in define_attribute_methods' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:252:in `each' /Users/senny/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:252:in `define_attribute_methods' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_methods.rb:72:in `block in define_attribute_methods' /Users/senny/.rbenv/versions/2.0.0-p353/lib/ruby/2.0.0/mutex_m.rb:73:in `synchronize' /Users/senny/.rbenv/versions/2.0.0-p353/lib/ruby/2.0.0/mutex_m.rb:73:in `mu_synchronize' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_methods.rb:69:in `define_attribute_methods' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_methods.rb:174:in `method_missing' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_assignment.rb:45:in `public_send' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_assignment.rb:45:in `_assign_attribute' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_assignment.rb:32:in `block in assign_attributes' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_assignment.rb:26:in `each' /Users/senny/Projects/rails/activerecord/lib/active_record/attribute_assignment.rb:26:in `assign_attributes' /Users/senny/Projects/rails/activerecord/lib/active_record/core.rb:458:in `init_attributes' /Users/senny/Projects/rails/activerecord/lib/active_record/core.rb:191:in `initialize' /Users/senny/Projects/rails/activerecord/lib/active_record/inheritance.rb:30:in `new' /Users/senny/Projects/rails/activerecord/lib/active_record/inheritance.rb:30:in `new' /Users/senny/Projects/rails/activerecord/lib/active_record/persistence.rb:33:in `create' test/cases/base_test.rb:646:in `test_group_weirds_by_from' ``` --- activerecord/test/cases/base_test.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 2ec8a2f1e6..983bcd9826 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -626,6 +626,7 @@ class BasicsTest < ActiveRecord::TestCase assert_equal ["EUC-JP"], Weird.columns.map {|c| c.name.encoding.name }.uniq ensure silence_warnings { Encoding.default_internal = old_default_internal } + Weird.reset_column_information end end