From 89d5e944d5aeeb29ff5feb34a94c5dccd0d0c796 Mon Sep 17 00:00:00 2001 From: Joshua Clayton Date: Fri, 13 Apr 2012 14:20:19 -0400 Subject: [PATCH] Refactor Strategies This changes Strategy from a class to a module and removes inheritance. It introduces an Evaluation facade to make building strategies easier --- lib/factory_girl.rb | 8 ++++- lib/factory_girl/callback_runner.rb | 20 +++++++++++ lib/factory_girl/evaluation.rb | 23 +++++++++++++ lib/factory_girl/evaluator.rb | 19 +---------- lib/factory_girl/evaluator_class_definer.rb | 5 +-- lib/factory_girl/factory.rb | 9 +++-- lib/factory_girl/strategy.rb | 33 ------------------- lib/factory_girl/strategy/attributes_for.rb | 8 ++--- lib/factory_girl/strategy/build.rb | 10 +++--- lib/factory_girl/strategy/create.rb | 16 ++++----- lib/factory_girl/strategy/null.rb | 4 +-- lib/factory_girl/strategy/stub.rb | 12 +++---- lib/factory_girl/strategy_calculator.rb | 2 +- .../evaluator_class_definer_spec.rb | 14 ++------ spec/factory_girl/factory_spec.rb | 2 +- .../strategy/attributes_for_spec.rb | 10 +++--- spec/factory_girl/strategy/create_spec.rb | 20 ++++++++--- spec/factory_girl/strategy/stub_spec.rb | 13 ++++---- spec/factory_girl/strategy_calculator_spec.rb | 10 ------ spec/factory_girl/strategy_spec.rb | 21 ------------ spec/support/shared_examples/strategy.rb | 30 +++++------------ 21 files changed, 122 insertions(+), 167 deletions(-) create mode 100644 lib/factory_girl/callback_runner.rb create mode 100644 lib/factory_girl/evaluation.rb delete mode 100644 lib/factory_girl/strategy.rb delete mode 100644 spec/factory_girl/strategy_spec.rb diff --git a/lib/factory_girl.rb b/lib/factory_girl.rb index a13325f..adf2e4f 100644 --- a/lib/factory_girl.rb +++ b/lib/factory_girl.rb @@ -3,16 +3,22 @@ require "active_support/core_ext/module/delegation" require 'factory_girl/errors' require 'factory_girl/factory_runner' require 'factory_girl/strategy_calculator' -require 'factory_girl/strategy' +require "factory_girl/strategy/build" +require "factory_girl/strategy/create" +require "factory_girl/strategy/attributes_for" +require "factory_girl/strategy/stub" +require "factory_girl/strategy/null" require 'factory_girl/registry' require 'factory_girl/null_factory' require 'factory_girl/null_object' +require 'factory_girl/evaluation' require 'factory_girl/factory' require 'factory_girl/attribute_assigner' require 'factory_girl/evaluator' require 'factory_girl/evaluator_class_definer' require 'factory_girl/attribute' require 'factory_girl/callback' +require 'factory_girl/callback_runner' require 'factory_girl/declaration_list' require 'factory_girl/declaration' require 'factory_girl/sequence' diff --git a/lib/factory_girl/callback_runner.rb b/lib/factory_girl/callback_runner.rb new file mode 100644 index 0000000..723e223 --- /dev/null +++ b/lib/factory_girl/callback_runner.rb @@ -0,0 +1,20 @@ +module FactoryGirl + class CallbackRunner + def initialize(callbacks, evaluator) + @callbacks = callbacks + @evaluator = evaluator + end + + def update(name, result_instance) + callbacks_by_name(name).each do |callback| + callback.run(result_instance, @evaluator) + end + end + + private + + def callbacks_by_name(name) + @callbacks.select {|callback| callback.name == name } + end + end +end diff --git a/lib/factory_girl/evaluation.rb b/lib/factory_girl/evaluation.rb new file mode 100644 index 0000000..0c9ce85 --- /dev/null +++ b/lib/factory_girl/evaluation.rb @@ -0,0 +1,23 @@ +require "observer" + +module FactoryGirl + class Evaluation + include Observable + + def initialize(attribute_assigner, to_create) + @attribute_assigner = attribute_assigner + @to_create = to_create + end + + def create(result_instance) + @to_create[result_instance] + end + + delegate :object, :hash, to: :@attribute_assigner + + def notify(name, result_instance) + changed + notify_observers(name, result_instance) + end + end +end diff --git a/lib/factory_girl/evaluator.rb b/lib/factory_girl/evaluator.rb index ccf100f..42e7f3d 100644 --- a/lib/factory_girl/evaluator.rb +++ b/lib/factory_girl/evaluator.rb @@ -3,7 +3,7 @@ require "active_support/core_ext/class/attribute" module FactoryGirl class Evaluator - class_attribute :callbacks, :attribute_lists + class_attribute :attribute_lists def self.attribute_list AttributeList.new.tap do |list| @@ -25,8 +25,6 @@ module FactoryGirl @overrides.each do |name, value| singleton_class.send :define_method, name, lambda { value } end - - @build_strategy.add_observer(CallbackRunner.new(self.class.callbacks, self)) end def association(factory_name, overrides = {}) @@ -56,20 +54,5 @@ module FactoryGirl def __overrides @overrides end - - private - - class CallbackRunner - def initialize(callbacks, evaluator) - @callbacks = callbacks - @evaluator = evaluator - end - - def update(name, result_instance) - @callbacks.select {|callback| callback.name == name }.each do |callback| - callback.run(result_instance, @evaluator) - end - end - end end end diff --git a/lib/factory_girl/evaluator_class_definer.rb b/lib/factory_girl/evaluator_class_definer.rb index 89ca0c3..6a1ee1e 100644 --- a/lib/factory_girl/evaluator_class_definer.rb +++ b/lib/factory_girl/evaluator_class_definer.rb @@ -1,8 +1,7 @@ module FactoryGirl class EvaluatorClassDefiner - def initialize(attributes, callbacks, parent_class) + def initialize(attributes, parent_class) @parent_class = parent_class - @callbacks = callbacks @attributes = attributes attributes.each do |attribute| @@ -12,8 +11,6 @@ module FactoryGirl def evaluator_class @evaluator_class ||= Class.new(@parent_class).tap do |klass| - klass.callbacks ||= [] - klass.callbacks += @callbacks klass.attribute_lists ||= [] klass.attribute_lists += [@attributes] end diff --git a/lib/factory_girl/factory.rb b/lib/factory_girl/factory.rb index 6ccaec7..d4d8ec6 100644 --- a/lib/factory_girl/factory.rb +++ b/lib/factory_girl/factory.rb @@ -35,7 +35,10 @@ module FactoryGirl evaluator = evaluator_class.new(strategy, overrides.symbolize_keys) attribute_assigner = AttributeAssigner.new(evaluator, &instance_builder) - strategy.result(attribute_assigner, to_create).tap(&block) + evaluation = Evaluation.new(attribute_assigner, to_create) + evaluation.add_observer(CallbackRunner.new(callbacks, evaluator)) + + strategy.result(evaluation).tap(&block) end def human_names @@ -97,7 +100,7 @@ module FactoryGirl end def evaluator_class - @evaluator_class ||= EvaluatorClassDefiner.new(attributes, callbacks, parent.evaluator_class).evaluator_class + @evaluator_class ||= EvaluatorClassDefiner.new(attributes, parent.evaluator_class).evaluator_class end def attributes @@ -110,7 +113,7 @@ module FactoryGirl end def callbacks - processing_order.map {|factory| factory.callbacks }.flatten + parent.callbacks + processing_order.map {|factory| factory.callbacks }.flatten end def constructor diff --git a/lib/factory_girl/strategy.rb b/lib/factory_girl/strategy.rb deleted file mode 100644 index 89cb5d6..0000000 --- a/lib/factory_girl/strategy.rb +++ /dev/null @@ -1,33 +0,0 @@ -require "factory_girl/strategy/build" -require "factory_girl/strategy/create" -require "factory_girl/strategy/attributes_for" -require "factory_girl/strategy/stub" -require "factory_girl/strategy/null" -require "observer" - -module FactoryGirl - class Strategy #:nodoc: - include Observable - - def association(runner) - raise NotImplementedError, "Strategies must return an association" - end - - def result(attribute_assigner, to_create) - raise NotImplementedError, "Strategies must return a result" - end - - def self.ensure_strategy_exists!(strategy) - unless Strategy.const_defined? strategy.to_s.camelize - raise ArgumentError, "Unknown strategy: #{strategy}" - end - end - - private - - def run_callbacks(name, result_instance) - changed - notify_observers(name, result_instance) - end - end -end diff --git a/lib/factory_girl/strategy/attributes_for.rb b/lib/factory_girl/strategy/attributes_for.rb index cc17a32..c0fd2a2 100644 --- a/lib/factory_girl/strategy/attributes_for.rb +++ b/lib/factory_girl/strategy/attributes_for.rb @@ -1,12 +1,12 @@ module FactoryGirl - class Strategy #:nodoc: - class AttributesFor < Strategy #:nodoc: + module Strategy + class AttributesFor def association(runner) runner.run(Strategy::Null) end - def result(attribute_assigner, to_create) - attribute_assigner.hash + def result(evaluation) + evaluation.hash end end end diff --git a/lib/factory_girl/strategy/build.rb b/lib/factory_girl/strategy/build.rb index 4832ac3..f9ef709 100644 --- a/lib/factory_girl/strategy/build.rb +++ b/lib/factory_girl/strategy/build.rb @@ -1,13 +1,13 @@ module FactoryGirl - class Strategy #:nodoc: - class Build < Strategy #:nodoc: + module Strategy + class Build def association(runner) runner.run end - def result(attribute_assigner, to_create) - attribute_assigner.object.tap do |result_instance| - run_callbacks(:after_build, result_instance) + def result(evaluation) + evaluation.object.tap do |instance| + evaluation.notify(:after_build, instance) end end end diff --git a/lib/factory_girl/strategy/create.rb b/lib/factory_girl/strategy/create.rb index 0551208..fd5986d 100644 --- a/lib/factory_girl/strategy/create.rb +++ b/lib/factory_girl/strategy/create.rb @@ -1,16 +1,16 @@ module FactoryGirl - class Strategy #:nodoc: - class Create < Strategy #:nodoc: + module Strategy + class Create def association(runner) runner.run end - def result(attribute_assigner, to_create) - attribute_assigner.object.tap do |result_instance| - run_callbacks(:after_build, result_instance) - run_callbacks(:before_create, result_instance) - to_create[result_instance] - run_callbacks(:after_create, result_instance) + def result(evaluation) + evaluation.object.tap do |instance| + evaluation.notify(:after_build, instance) + evaluation.notify(:before_create, instance) + evaluation.create(instance) + evaluation.notify(:after_create, instance) end end end diff --git a/lib/factory_girl/strategy/null.rb b/lib/factory_girl/strategy/null.rb index a74afd2..cd4ec31 100644 --- a/lib/factory_girl/strategy/null.rb +++ b/lib/factory_girl/strategy/null.rb @@ -1,10 +1,10 @@ module FactoryGirl - class Strategy + module Strategy class Null def association(runner) end - def result(attribute_assigner, to_create) + def result(evaluation) end end end diff --git a/lib/factory_girl/strategy/stub.rb b/lib/factory_girl/strategy/stub.rb index ca8740a..b6f6253 100644 --- a/lib/factory_girl/strategy/stub.rb +++ b/lib/factory_girl/strategy/stub.rb @@ -1,16 +1,16 @@ module FactoryGirl - class Strategy - class Stub < Strategy #:nodoc: + module Strategy + class Stub @@next_id = 1000 def association(runner) runner.run(Strategy::Stub) end - def result(attribute_assigner, to_create) - attribute_assigner.object.tap do |result_instance| - stub_database_interaction_on_result(result_instance) - run_callbacks(:after_stub, result_instance) + def result(evaluation) + evaluation.object.tap do |instance| + stub_database_interaction_on_result(instance) + evaluation.notify(:after_stub, instance) end end diff --git a/lib/factory_girl/strategy_calculator.rb b/lib/factory_girl/strategy_calculator.rb index e62f7cf..86ab371 100644 --- a/lib/factory_girl/strategy_calculator.rb +++ b/lib/factory_girl/strategy_calculator.rb @@ -15,7 +15,7 @@ module FactoryGirl private def strategy_is_object? - @name_or_object.is_a?(Class) && @name_or_object.ancestors.include?(::FactoryGirl::Strategy) + @name_or_object.is_a?(Class) end def strategy_name_to_object diff --git a/spec/factory_girl/evaluator_class_definer_spec.rb b/spec/factory_girl/evaluator_class_definer_spec.rb index 8449ecf..afb1377 100644 --- a/spec/factory_girl/evaluator_class_definer_spec.rb +++ b/spec/factory_girl/evaluator_class_definer_spec.rb @@ -4,10 +4,9 @@ describe FactoryGirl::EvaluatorClassDefiner do let(:simple_attribute) { stub("simple attribute", name: :simple, to_proc: lambda { 1 }) } let(:relative_attribute) { stub("relative attribute", name: :relative, to_proc: lambda { simple + 1 }) } let(:attribute_that_raises_a_second_time) { stub("attribute that would raise without a cache", name: :raises_without_proper_cache, to_proc: lambda { raise "failed" if @run; @run = true; nil }) } - let(:callbacks) { [stub("callback 1"), stub("callback 2")] } let(:attributes) { [simple_attribute, relative_attribute, attribute_that_raises_a_second_time] } - let(:class_definer) { FactoryGirl::EvaluatorClassDefiner.new(attributes, callbacks, FactoryGirl::Evaluator) } + let(:class_definer) { FactoryGirl::EvaluatorClassDefiner.new(attributes, FactoryGirl::Evaluator) } let(:evaluator) { class_definer.evaluator_class.new(stub("build strategy", add_observer: true)) } it "returns an evaluator when accessing the evaluator class" do @@ -32,23 +31,14 @@ describe FactoryGirl::EvaluatorClassDefiner do class_definer.evaluator_class.attribute_lists.should == [attributes] end - it "sets callbacks on the evaluator class" do - class_definer.evaluator_class.callbacks.should == callbacks - end - context "with a custom evaluator as a parent class" do - let(:child_callbacks) { [stub("child callback 1"), stub("child callback 2")] } let(:child_attributes) { [stub("child attribute", name: :simple, to_proc: lambda { 1 })] } - let(:child_definer) { FactoryGirl::EvaluatorClassDefiner.new(child_attributes, child_callbacks, class_definer.evaluator_class) } + let(:child_definer) { FactoryGirl::EvaluatorClassDefiner.new(child_attributes, class_definer.evaluator_class) } subject { child_definer.evaluator_class } it "bases its attribute lists on itself and its parent evaluator" do subject.attribute_lists.should == [attributes, child_attributes] end - - it "bases its callbacks on itself and its parent evaluator" do - subject.callbacks.should == callbacks + child_callbacks - end end end diff --git a/spec/factory_girl/factory_spec.rb b/spec/factory_girl/factory_spec.rb index 50b1a1b..14245bb 100644 --- a/spec/factory_girl/factory_spec.rb +++ b/spec/factory_girl/factory_spec.rb @@ -25,7 +25,7 @@ describe FactoryGirl::Factory do factory.run(FactoryGirl::Strategy::Build, {}) - strategy.should have_received(:result).with(instance_of(FactoryGirl::AttributeAssigner), block) + strategy.should have_received(:result).with(instance_of(FactoryGirl::Evaluation)) end it "returns associations" do diff --git a/spec/factory_girl/strategy/attributes_for_spec.rb b/spec/factory_girl/strategy/attributes_for_spec.rb index 9d9feec..5896066 100644 --- a/spec/factory_girl/strategy/attributes_for_spec.rb +++ b/spec/factory_girl/strategy/attributes_for_spec.rb @@ -1,18 +1,18 @@ require 'spec_helper' describe FactoryGirl::Strategy::AttributesFor do - let(:result) { { name: "John Doe", gender: "Male", admin: false } } - let(:attribute_assigner) { stub("attribute assigner", hash: result) } + let(:result) { { name: "John Doe", gender: "Male", admin: false } } + let(:evaluation) { stub("evaluation", hash: result) } it_should_behave_like "strategy without association support" - it "returns the hash from the attribute assigner" do - subject.result(attribute_assigner, lambda {|item| item }).should == result + it "returns the hash from the evaluation" do + subject.result(evaluation).should == result end it "does not run the to_create block" do expect do - subject.result(attribute_assigner, lambda {|item| raise "failed" }) + subject.result(evaluation) end.to_not raise_error end end diff --git a/spec/factory_girl/strategy/create_spec.rb b/spec/factory_girl/strategy/create_spec.rb index a124774..104c8a1 100644 --- a/spec/factory_girl/strategy/create_spec.rb +++ b/spec/factory_girl/strategy/create_spec.rb @@ -5,9 +5,21 @@ describe FactoryGirl::Strategy::Create do it_should_behave_like "strategy with callbacks", :after_build, :before_create, :after_create it "runs a custom create block" do - block_run = false - block = lambda {|instance| block_run = true } - subject.result(stub("assigner", object: stub("result instance")), block) - block_run.should be_true + evaluation_class = Class.new do + def initialize + @block_run = false + end + + attr_reader :block_run + + def create(*instance) + @block_run = true + end + end + + evaluation = evaluation_class.new + evaluation.stubs(object: nil, notify: nil) + subject.result(evaluation) + evaluation.block_run.should be_true end end diff --git a/spec/factory_girl/strategy/stub_spec.rb b/spec/factory_girl/strategy/stub_spec.rb index 6073647..e999a6a 100644 --- a/spec/factory_girl/strategy/stub_spec.rb +++ b/spec/factory_girl/strategy/stub_spec.rb @@ -14,25 +14,24 @@ describe FactoryGirl::Strategy::Stub do end.new end - let(:assigner) { stub("attribute assigner", object: result_instance) } - let(:to_create) { lambda {|instance| instance } } + let(:evaluation) { stub("evaluation", object: result_instance, notify: true) } - it { subject.result(assigner, to_create).should_not be_new_record } - it { subject.result(assigner, to_create).should be_persisted } + it { subject.result(evaluation).should_not be_new_record } + it { subject.result(evaluation).should be_persisted } it "assigns created_at" do - created_at = subject.result(assigner, to_create).created_at + created_at = subject.result(evaluation).created_at created_at.should == Time.now Timecop.travel(150000) - subject.result(assigner, to_create).created_at.should == created_at + subject.result(evaluation).created_at.should == created_at end [:save, :destroy, :connection, :reload, :update_attribute].each do |database_method| it "raises when attempting to connect to the database by calling #{database_method}" do expect do - subject.result(assigner, to_create).send(database_method) + subject.result(evaluation).send(database_method) end.to raise_error(RuntimeError, "stubbed models are not allowed to access the database") end end diff --git a/spec/factory_girl/strategy_calculator_spec.rb b/spec/factory_girl/strategy_calculator_spec.rb index f5b3641..b9d5006 100644 --- a/spec/factory_girl/strategy_calculator_spec.rb +++ b/spec/factory_girl/strategy_calculator_spec.rb @@ -8,16 +8,6 @@ describe FactoryGirl::StrategyCalculator, "with a FactoryGirl::Strategy object" end end -describe FactoryGirl::StrategyCalculator, "with a non-FactoryGirl::Strategy object" do - before { define_class "MyAwesomeStrategy" } - - let(:strategy) { MyAwesomeStrategy } - - it "returns the strategy object" do - expect { FactoryGirl::StrategyCalculator.new(strategy).strategy }.to raise_error "unrecognized method MyAwesomeStrategy" - end -end - describe FactoryGirl::StrategyCalculator do it "returns the correct strategy object for :build" do FactoryGirl::StrategyCalculator.new(:build).strategy.should == FactoryGirl::Strategy::Build diff --git a/spec/factory_girl/strategy_spec.rb b/spec/factory_girl/strategy_spec.rb deleted file mode 100644 index 81fed5c..0000000 --- a/spec/factory_girl/strategy_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'spec_helper' - -describe FactoryGirl::Strategy do - it "raises an error when asking for the result" do - expect { subject.result(stub("assigner"), lambda {|instance| instance }) }.to raise_error(NotImplementedError, "Strategies must return a result") - end - - it "raises an error when asking for the association" do - expect { subject.association(stub("runner")) }.to raise_error(NotImplementedError, "Strategies must return an association") - end -end - -describe FactoryGirl::Strategy, ".ensure_strategy_exists!" do - it "raises when passed a nonexistent strategy" do - expect { FactoryGirl::Strategy.ensure_strategy_exists!(:nonexistent) }.to raise_error(ArgumentError, "Unknown strategy: nonexistent") - end - - it "doesn't raise when passed a valid strategy" do - expect { FactoryGirl::Strategy.ensure_strategy_exists!(:create) }.to_not raise_error - end -end diff --git a/spec/support/shared_examples/strategy.rb b/spec/support/shared_examples/strategy.rb index 0aa7408..31c2abc 100644 --- a/spec/support/shared_examples/strategy.rb +++ b/spec/support/shared_examples/strategy.rb @@ -70,36 +70,22 @@ shared_examples_for "strategy with strategy: :build" do |factory_girl_strategy_c end shared_examples_for "strategy with callbacks" do |*callback_names| - let(:callback_observer) do - define_class("CallbackObserver") do - attr_reader :callbacks_called - - def initialize - @callbacks_called = [] - end - - def update(callback_name, assigner) - @callbacks_called << [callback_name, assigner] - end - end.new - end - let(:result_instance) do define_class("ResultInstance") do attr_accessor :id end.new end - let(:assigner) { stub("attribute assigner", object: result_instance) } + let(:evaluation) { stub("evaluation", object: result_instance, notify: true, create: nil) } - before { subject.add_observer(callback_observer) } - - it "runs the callbacks #{callback_names} with the assigner's object" do - subject.result(assigner, lambda {|instance| instance }) - callback_observer.callbacks_called.should == callback_names.map {|name| [name, assigner.object] } + it "runs the callbacks #{callback_names} with the evaluation's object" do + subject.result(evaluation) + callback_names.each do |name| + evaluation.should have_received(:notify).with(name, evaluation.object) + end end - it "returns the object from the assigner" do - subject.result(assigner, lambda {|instance| instance }).should == assigner.object + it "returns the object from the evaluation" do + subject.result(evaluation).should == evaluation.object end end