From 5947e10578194f663584c48cd663f0638a761def Mon Sep 17 00:00:00 2001 From: Alejandro Dustet Date: Fri, 26 Apr 2019 15:48:37 -0400 Subject: [PATCH] Deprecate and move to Internal sequence methods Why: Another run of internal methods that should not be publicly available from the base namespace. This time the sequence involving methods were moved. It's worth noticing that the ```Internal``` class is getting crowded. Maybe we can start name-spacing the internal groups into modules under ```internal/sequence.rb``` ```internal/trait.rb``` and so on. Thoughts? This PR: - Moves the ```register_sequence```, ```sequence_by_name```, ```sequences``` and ```rewind_sequences``` to the ```FactoryBot::Internal``` module. - Deprecates uses of ```sequence_by_name```, and ```sequences``` from the ```FactoryBot``` module. - Refactor rewind sequences test to use spies This is one of the steps towards fixing [this issue](https://github.com/thoughtbot/factory_bot/pull/1285#1281) --- lib/factory_bot.rb | 23 ++++-------- lib/factory_bot/declaration/implicit.rb | 2 +- lib/factory_bot/internal.rb | 18 +++++++++- lib/factory_bot/syntax/default.rb | 4 +-- lib/factory_bot/syntax/methods.rb | 4 +-- spec/factory_bot/attribute/dynamic_spec.rb | 8 ++++- spec/factory_bot/attribute/sequence_spec.rb | 2 +- spec/factory_bot/declaration/implicit_spec.rb | 4 +-- spec/factory_bot/internal_spec.rb | 36 +++++++++++++++++++ spec/factory_bot_spec.rb | 2 +- 10 files changed, 75 insertions(+), 28 deletions(-) diff --git a/lib/factory_bot.rb b/lib/factory_bot.rb index 96ff509..0a1bb59 100644 --- a/lib/factory_bot.rb +++ b/lib/factory_bot.rb @@ -87,8 +87,11 @@ module FactoryBot :constructor, to: :configuration - delegate :trait_by_name, + delegate :register_sequence, :register_trait, + :rewind_sequences, + :sequence_by_name, + :trait_by_name, to: Internal attr_accessor :allow_class_lookup @@ -96,6 +99,8 @@ module FactoryBot deprecate :allow_class_lookup, :allow_class_lookup=, :register_trait, + :sequence_by_name, + :sequences, :trait_by_name, :traits, deprecator: Deprecation @@ -112,22 +117,6 @@ module FactoryBot factories.find(name) end - def self.register_sequence(sequence) - sequence.names.each do |name| - sequences.register(name, sequence) - end - sequence - end - - def self.sequence_by_name(name) - sequences.find(name) - end - - def self.rewind_sequences - sequences.each(&:rewind) - Internal.rewind_inline_sequences - end - def self.register_strategy(strategy_name, strategy_class) strategies.register(strategy_name, strategy_class) StrategySyntaxMethodRegistrar.new(strategy_name).define_strategy_methods diff --git a/lib/factory_bot/declaration/implicit.rb b/lib/factory_bot/declaration/implicit.rb index e4fa8ea..6901a64 100644 --- a/lib/factory_bot/declaration/implicit.rb +++ b/lib/factory_bot/declaration/implicit.rb @@ -23,7 +23,7 @@ module FactoryBot def build if FactoryBot.factories.registered?(name) [Attribute::Association.new(name, name, {})] - elsif FactoryBot.sequences.registered?(name) + elsif FactoryBot::Internal.sequences.registered?(name) [Attribute::Sequence.new(name, name, @ignored)] else @factory.inherit_traits([name]) diff --git a/lib/factory_bot/internal.rb b/lib/factory_bot/internal.rb index e0fea58..1f3e85c 100644 --- a/lib/factory_bot/internal.rb +++ b/lib/factory_bot/internal.rb @@ -2,7 +2,7 @@ module FactoryBot # @api private module Internal class << self - delegate :inline_sequences, :traits, to: :configuration + delegate :inline_sequences, :sequences, :traits, to: :configuration def configuration @configuration ||= Configuration.new @@ -30,6 +30,22 @@ module FactoryBot def trait_by_name(name) traits.find(name) end + + def register_sequence(sequence) + sequence.names.each do |name| + sequences.register(name, sequence) + end + sequence + end + + def sequence_by_name(name) + sequences.find(name) + end + + def rewind_sequences + sequences.each(&:rewind) + rewind_inline_sequences + end end end end diff --git a/lib/factory_bot/syntax/default.rb b/lib/factory_bot/syntax/default.rb index 9ad03cc..f772574 100644 --- a/lib/factory_bot/syntax/default.rb +++ b/lib/factory_bot/syntax/default.rb @@ -26,7 +26,7 @@ module FactoryBot end def sequence(name, *args, &block) - FactoryBot.register_sequence(Sequence.new(name, *args, &block)) + Internal.register_sequence(Sequence.new(name, *args, &block)) end def trait(name, &block) @@ -54,7 +54,7 @@ module FactoryBot private def configuration - FactoryBot::Internal.configuration + Internal.configuration end end diff --git a/lib/factory_bot/syntax/methods.rb b/lib/factory_bot/syntax/methods.rb index 72ba34e..f2625f0 100644 --- a/lib/factory_bot/syntax/methods.rb +++ b/lib/factory_bot/syntax/methods.rb @@ -111,7 +111,7 @@ module FactoryBot # Returns: # The next value in the sequence. (Object) def generate(name) - FactoryBot.sequence_by_name(name).next + Internal.sequence_by_name(name).next end # Generates and returns the list of values in a sequence. @@ -126,7 +126,7 @@ module FactoryBot # The next value in the sequence. (Object) def generate_list(name, count) (1..count).map do - FactoryBot.sequence_by_name(name).next + Internal.sequence_by_name(name).next end end end diff --git a/spec/factory_bot/attribute/dynamic_spec.rb b/spec/factory_bot/attribute/dynamic_spec.rb index bfc9527..b8088a7 100644 --- a/spec/factory_bot/attribute/dynamic_spec.rb +++ b/spec/factory_bot/attribute/dynamic_spec.rb @@ -45,7 +45,13 @@ describe FactoryBot::Attribute::Dynamic do end context "with a block returning a sequence" do - let(:block) { -> { FactoryBot.register_sequence(FactoryBot::Sequence.new(:email, 1) { |n| "foo#{n}" }) } } + let(:block) do + -> do + FactoryBot::Internal.register_sequence( + FactoryBot::Sequence.new(:email, 1) { |n| "foo#{n}" }, + ) + end + end it "raises a sequence abuse error" do expect { subject.to_proc.call }.to raise_error(FactoryBot::SequenceAbuseError) diff --git a/spec/factory_bot/attribute/sequence_spec.rb b/spec/factory_bot/attribute/sequence_spec.rb index 03297db..13d8388 100644 --- a/spec/factory_bot/attribute/sequence_spec.rb +++ b/spec/factory_bot/attribute/sequence_spec.rb @@ -4,7 +4,7 @@ describe FactoryBot::Attribute::Sequence do let(:sequence) { FactoryBot::Sequence.new(sequence_name, 5) { |n| "Name #{n}" } } subject { FactoryBot::Attribute::Sequence.new(name, sequence_name, false) } - before { FactoryBot.register_sequence(sequence) } + before { FactoryBot::Internal.register_sequence(sequence) } its(:name) { should eq name } diff --git a/spec/factory_bot/declaration/implicit_spec.rb b/spec/factory_bot/declaration/implicit_spec.rb index 56e5cd8..f82dae2 100644 --- a/spec/factory_bot/declaration/implicit_spec.rb +++ b/spec/factory_bot/declaration/implicit_spec.rb @@ -22,7 +22,7 @@ describe FactoryBot::Declaration::Implicit do context "with a known sequence" do it "does not create an assocition attribute" do - allow(FactoryBot.sequences).to receive(:registered?).and_return true + allow(FactoryBot::Internal.sequences).to receive(:registered?).and_return true declaration = FactoryBot::Declaration::Implicit.new(:name) attribute = declaration.to_attributes.first @@ -31,7 +31,7 @@ describe FactoryBot::Declaration::Implicit do end it "creates a sequence attribute" do - allow(FactoryBot.sequences).to receive(:registered?).and_return true + allow(FactoryBot::Internal.sequences).to receive(:registered?).and_return true declaration = FactoryBot::Declaration::Implicit.new(:name) attribute = declaration.to_attributes.first diff --git a/spec/factory_bot/internal_spec.rb b/spec/factory_bot/internal_spec.rb index f16464d..d0c8b72 100644 --- a/spec/factory_bot/internal_spec.rb +++ b/spec/factory_bot/internal_spec.rb @@ -17,4 +17,40 @@ describe FactoryBot::Internal do expect(FactoryBot::Internal.trait_by_name(trait.name)).to eq trait end end + + describe ".register_sequence" do + it "registers the provided sequence" do + sequence = FactoryBot::Sequence.new(:email) + configuration = FactoryBot::Internal.configuration + expect { FactoryBot::Internal.register_sequence(sequence) }. + to change { configuration.sequences.count }. + from(0). + to(1) + end + end + + describe ".sequence_by_name" do + it "finds a registered sequence" do + sequence = FactoryBot::Sequence.new(:email) + FactoryBot::Internal.register_sequence(sequence) + expect(FactoryBot::Internal.sequence_by_name(sequence.name)).to eq sequence + end + end + + describe ".rewind_sequences" do + it "rewinds the sequences and the internal sequences" do + sequence = instance_double(FactoryBot::Sequence, names: ["email"]) + allow(sequence).to receive(:rewind) + FactoryBot::Internal.register_sequence(sequence) + + inline_sequence = instance_double(FactoryBot::Sequence) + allow(inline_sequence).to receive(:rewind) + FactoryBot::Internal.register_inline_sequence(inline_sequence) + + FactoryBot::Internal.rewind_sequences + + expect(sequence).to have_received(:rewind).exactly(:once) + expect(inline_sequence).to have_received(:rewind).exactly(:once) + end + end end diff --git a/spec/factory_bot_spec.rb b/spec/factory_bot_spec.rb index 5a41fa5..55bed65 100644 --- a/spec/factory_bot_spec.rb +++ b/spec/factory_bot_spec.rb @@ -8,7 +8,7 @@ describe FactoryBot do expect(FactoryBot.factory_by_name(factory.name)).to eq factory end - it "finds a registered sequence" do + it "finds a registered sequence", :silence_deprecation do FactoryBot.register_sequence(sequence) expect(FactoryBot.sequence_by_name(sequence.name)).to eq sequence end