Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Refactor NestedAttributesWithCallbacksTest for clarity

1) Use `assert` and `refute` where possible.
2) Separately include the setup, subject, and assertions in each test - don't hide the tested call in an assertion method.
3) Name things based on their role rather than incidental facts about them - e.g. `@bird[1]` -> `bird_to_destroy`. `bird2_deletion_attributes` -> `destroy_bird_attributes`.
4) Use more succinct naming where possible - e.g. `birds_with_callback` -> `birds_with_add`, `@pirate_with_two_birds` -> `@pirate`
  • Loading branch information...
commit cc368c3ab39bc376181d883c31877d9729fe3423 1 parent d35e900
@Empact Empact authored
Showing with 66 additions and 67 deletions.
  1. +66 −67 activerecord/test/cases/nested_attributes_with_callbacks_test.rb
View
133 activerecord/test/cases/nested_attributes_with_callbacks_test.rb
@@ -2,144 +2,143 @@
require "models/pirate"
require "models/bird"
-class TestNestedAttributesWithCallbacksInterferingWithAssignment < ActiveRecord::TestCase
- Pirate.has_many(:birds_with_interfering_callback,
+class NestedAttributesWithCallbacksTest < ActiveRecord::TestCase
+ Pirate.has_many(:birds_with_add_load,
:class_name => "Bird",
:before_add => proc { |p,b|
@@add_callback_called << b
- p.birds_with_interfering_callback.to_a
+ p.birds_with_add_load.to_a
})
- Pirate.has_many(:birds_with_callback,
+ Pirate.has_many(:birds_with_add,
:class_name => "Bird",
:before_add => proc { |p,b| @@add_callback_called << b })
- Pirate.accepts_nested_attributes_for(:birds_with_interfering_callback,
- :birds_with_callback,
+ Pirate.accepts_nested_attributes_for(:birds_with_add_load,
+ :birds_with_add,
:allow_destroy => true)
def setup
@@add_callback_called = []
- @expect_callbacks_for = []
- @pirate_with_two_birds = Pirate.new.tap do |pirate|
+ @pirate = Pirate.new.tap do |pirate|
pirate.catchphrase = "Don't call me!"
pirate.birds_attributes = [{:name => 'Bird1'},{:name => 'Bird2'}]
pirate.save!
end
- @birds = @pirate_with_two_birds.birds.to_a
- @birds_attributes = @birds.map do |bird|
+ @birds = @pirate.birds.to_a
+ end
+
+ def bird_to_update
+ @birds[0]
+ end
+
+ def bird_to_destroy
+ @birds[1]
+ end
+
+ def existing_birds_attributes
+ @birds.map do |bird|
bird.attributes.slice("id","name")
end
end
def new_birds
- @pirate_with_two_birds.birds_with_callback.to_a - @birds
+ @pirate.birds_with_add.to_a - @birds
end
def new_bird_attributes
[{'name' => "New Bird"}]
end
- def bird2_deletion_attributes
- [{'id' => @birds[1].id.to_s, "_destroy" => true}]
+ def destroy_bird_attributes
+ [{'id' => bird_to_destroy.id.to_s, "_destroy" => true}]
end
def update_new_and_destroy_bird_attributes
[{'id' => @birds[0].id.to_s, 'name' => 'New Name'},
{'name' => "New Bird"},
- {'id' => @birds[1].id.to_s, "_destroy" => true}]
+ {'id' => bird_to_destroy.id.to_s, "_destroy" => true}]
end
# Characterizing when :before_add callback is called
test ":before_add called for new bird when not loaded" do
- assert_birds_with_callback_not_loaded
- assert_callback_called_for_new_bird_assignment
+ refute @pirate.birds_with_add.loaded?
+ @pirate.birds_with_add_attributes = new_bird_attributes
+ assert_new_bird_with_callback_called
end
test ":before_add called for new bird when loaded" do
- @pirate_with_two_birds.birds_with_callback.load_target
- assert_callback_called_for_new_bird_assignment
+ @pirate.birds_with_add.load_target
+ @pirate.birds_with_add_attributes = new_bird_attributes
+ assert_new_bird_with_callback_called
end
- def assert_callback_called_for_new_bird_assignment
- @pirate_with_two_birds.birds_with_callback_attributes = new_bird_attributes
- assert_equal(1,new_birds.size)
- assert_callback_called_for_new_birds
+ def assert_new_bird_with_callback_called
+ assert_equal(1, new_birds.size)
+ assert_equal(new_birds, @@add_callback_called)
end
test ":before_add not called for identical assignment when not loaded" do
- assert_birds_with_callback_not_loaded
- assert_callback_not_called_for_identical_assignment
+ refute @pirate.birds_with_add.loaded?
+ @pirate.birds_with_add_attributes = existing_birds_attributes
+ assert_callbacks_not_called
end
test ":before_add not called for identical assignment when loaded" do
- @pirate_with_two_birds.birds_with_callback.load_target
- assert_callback_not_called_for_identical_assignment
- end
-
- def assert_callback_not_called_for_identical_assignment
- @pirate_with_two_birds.birds_with_callback_attributes = @birds_attributes
- assert_equal([],new_birds)
- assert_callback_called_for_new_birds
+ @pirate.birds_with_add.load_target
+ @pirate.birds_with_add_attributes = existing_birds_attributes
+ assert_callbacks_not_called
end
test ":before_add not called for destroy assignment when not loaded" do
- assert_birds_with_callback_not_loaded
- assert_callback_not_called_for_destroy_assignment
- end
-
- test ":before_add not called for destroy assignment when loaded" do
- @pirate_with_two_birds.birds_with_callback.load_target
- assert_callback_not_called_for_destroy_assignment
- end
-
- def assert_callback_not_called_for_destroy_assignment
- @pirate_with_two_birds.birds_with_callback_attributes =
- bird2_deletion_attributes
- assert_callback_called_for_new_birds
+ refute @pirate.birds_with_add.loaded?
+ @pirate.birds_with_add_attributes = destroy_bird_attributes
+ assert_callbacks_not_called
end
- def assert_birds_with_callback_not_loaded
- assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?)
+ test ":before_add not called for deletion assignment when loaded" do
+ @pirate.birds_with_add.load_target
+ @pirate.birds_with_add_attributes = destroy_bird_attributes
+ assert_callbacks_not_called
end
- def assert_callback_called_for_new_birds
- assert_equal(new_birds,@@add_callback_called)
+ def assert_callbacks_not_called
+ assert_empty new_birds
+ assert_empty @@add_callback_called
end
# Ensuring that the records in the association target are updated,
# whether the association is loaded before or not
test "Assignment updates records in target when not loaded" do
- assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?)
- assert_assignment_affects_records_in_target(:birds_with_callback)
+ refute @pirate.birds_with_add.loaded?
+ @pirate.birds_with_add_attributes = update_new_and_destroy_bird_attributes
+ assert_assignment_affects_records_in_target(:birds_with_add)
end
test "Assignment updates records in target when loaded" do
- @pirate_with_two_birds.birds_with_callback.load_target
- assert_assignment_affects_records_in_target(:birds_with_callback)
+ @pirate.birds_with_add.load_target
+ @pirate.birds_with_add_attributes = update_new_and_destroy_bird_attributes
+ assert_assignment_affects_records_in_target(:birds_with_add)
end
test("Assignment updates records in target when not loaded" +
" and callback loads target") do
- assert_equal(false,
- @pirate_with_two_birds.birds_with_interfering_callback.loaded?)
- assert_assignment_affects_records_in_target(
- :birds_with_interfering_callback)
+ refute @pirate.birds_with_add_load.loaded?
+ @pirate.birds_with_add_load_attributes = update_new_and_destroy_bird_attributes
+ assert_assignment_affects_records_in_target(:birds_with_add_load)
end
test("Assignment updates records in target when loaded" +
" and callback loads target") do
- @pirate_with_two_birds.birds_with_interfering_callback.load_target
- assert_assignment_affects_records_in_target(
- :birds_with_interfering_callback)
+ @pirate.birds_with_add_load.load_target
+ @pirate.birds_with_add_load_attributes = update_new_and_destroy_bird_attributes
+ assert_assignment_affects_records_in_target(:birds_with_add_load)
end
def assert_assignment_affects_records_in_target(association_name)
- @pirate_with_two_birds.send("#{association_name}_attributes=",
- update_new_and_destroy_bird_attributes)
- association = @pirate_with_two_birds.send(association_name)
- birds_in_target = @birds.map { |b| association.detect { |b_in_t| b_in_t == b }}
- assert_equal(true,birds_in_target[0].name_changed?,'First record not changed')
- assert_equal(true,birds_in_target[1].marked_for_destruction?,
- 'Second record not marked for destruction')
+ association = @pirate.send(association_name)
+ assert association.detect {|b| b == bird_to_update }.name_changed?,
+ 'Update record not updated'
+ assert association.detect {|b| b == bird_to_destroy }.marked_for_destruction?,
+ 'Destroy record not marked for destruction'
end
end
Please sign in to comment.
Something went wrong with that request. Please try again.