Rewinding all sequences and not just the global ones #1078

damln · 2017-11-06T08:52:13Z

The previous PR (50eeb67) added a feature to rewind registered sequences but this one can rewind all sequences for all factories.

You can call FactoryBot.rewind_sequences this way:

before :all do
  FactoryBot.rewind_sequences
end

I had a specific use case with VCR and I needed to have the same model ids (and other attributes) in the requests to be sure the VCR cassettes will always match. Rewind all the sequences at the beginning of the specs was the simplest solution I found.

Let me know what you thing about this.

houndci-bot · 2017-11-06T08:52:19Z

spec/acceptance/sequence_resetting_spec.rb

@@ -22,4 +32,31 @@
    expect(email).to eq "[email protected]"
    expect(name).to eq "Joe"
  end
+
+  it "resets all sequences back to their starting values for factory specific sequences" do


Line is too long. [91/80]

houndci-bot · 2017-11-06T08:52:19Z

spec/acceptance/sequence_resetting_spec.rb

@@ -3,6 +3,16 @@
 describe "FactoryBot.rewind_sequences" do
  include FactoryBot::Syntax::Methods

+  before do
+    define_model('User', age: :integer)


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-11-06T09:37:09Z

spec/acceptance/sequence_resetting_spec.rb

+        sequence(:number)
+      end
+    end
+


Extra empty line detected at block body end.

composerinteralia

Thanks for your work on this. I think it makes sense that rewind_sequences should also rewind the factory-level sequences.

I want to look more into the consequences of registering factory-level sequences, and what the best way to do that would be.

composerinteralia · 2018-06-17T04:45:55Z

spec/acceptance/sequence_resetting_spec.rb

@@ -3,6 +3,24 @@
 describe "FactoryBot.rewind_sequences" do
  include FactoryBot::Syntax::Methods

+  before do


Let's put this setup right in the test rather than a before block, since the existing test doesn't use it.

composerinteralia · 2018-06-17T04:53:43Z

lib/factory_bot/sequence.rb

+    def uuids
+      proc = @proc || -> {}
+      names.map do |name|
+        [name, proc.try(:to_s)].join


to_s on a proc would look something like #<Proc:0x00007fc64bcd2b48>, correct? I don't love that. Is there something else we could use? Maybe an actual uuid? Or some other solution what we haven't thought of yet?

composerinteralia · 2018-06-17T04:54:19Z

lib/factory_bot.rb

@@ -107,6 +107,14 @@ def self.register_sequence(sequence)
    sequence
  end

+  def self.register_sequence_uuid(sequence)


I would rather not add another public method for this. It would be nice if there was a way we could reuse register_sequence for this. Let's think about it a bit longer.

composerinteralia · 2018-06-17T05:15:22Z

spec/acceptance/sequence_resetting_spec.rb

+    user1 = FactoryBot.create(:user)
+    user2 = FactoryBot.create(:user)
+
+    expect(user1.id).to eq 1


I think we can get rid of some of these expects. They are duplicating some of the other sequence tests.

I am imagining something more similar to the existing test:

it "resets inline sequences back to their starting value" do class User attr_accessor :email end FactoryBot.define do factory :user do sequence(:email) { |n| "somebody#{n}@example.com" } end end 2.times do build(:user) end FactoryBot.rewind_sequences user = build(:user) expect(user.email).to eq "[email protected]" end

composerinteralia · 2018-06-17T16:03:59Z

lib/factory_bot/definition_proxy.rb

@@ -121,6 +121,7 @@ def method_missing(name, *args, &block)
    # Except that no globally available sequence will be defined.
    def sequence(name, *args, &block)
      sequence = Sequence.new(name, *args, &block)


It occurs to me that the name may not be important for these inline sequences. Could we just use some unique identifier as the name and register with register_sequence?

Thinking out loud here. What if we had an InlineSequence class or something like that whose names method just returned [object_id.to_s]?

This was originally opened as #1078, but this addresses the review comments on that PR. By registering the inline sequences, we allow them to get rewound with `FactoryBot.rewind_sequences`. We register them with `__#{factory_name}_#{sequence_name}__` to avoid conflicting with any reasonably named global sequences, and to hint that we should not be generating values from these sequences directly. Co-authored-by: Damian Le Nouaille <[email protected]> Co-authored-by: Damian Galarza <[email protected]>

composerinteralia · 2018-08-05T21:41:09Z

I made the requested changes opened #1164 with you as a co-author. Thanks again for your help!

This was originally opened as #1078, but this addresses the review comments on that PR. By registering the inline sequences, we allow them to get rewound with `FactoryBot.rewind_sequences`. We register them with `__#{factory_name}_#{sequence_name}__` to avoid conflicting with any reasonably named global sequences, and to hint that we should not be generating values from these sequences directly. Co-authored-by: Damian Le Nouaille <[email protected]> Co-authored-by: Damian Galarza <[email protected]>

Fixes #1257 When sequence rewinding was first introduced in #1078 it only applied to globally defined sequences. To get rewinding to work for inline sequences as well we registered them "privately" in the global registry in #1164. Unfortunately in #1164 we did not take inline sequences inside traits into consideration. Since trait names are not unique, it is possibly to get a `FactoryBot::DuplicateDefinitionError` when defining two sequences that have the same name in two traits that have the same name. This PR abandons the idea of "privately" registering inline sequences, and instead keeps a separate list of all the inline sequences just for the purpose of rewinding them. The downside here is that we are adding another public method to FactoryBot. But I think it is similar enough to the other `register_` methods, which are technically public but are left undocumented and not really treated as part of the FactoryBot API.

Fixes #1257 When sequence rewinding was first introduced in #1078 it only applied to globally defined sequences. To get rewinding to work for inline sequences as well we registered them "privately" in the global registry in #1164. Unfortunately in #1164 we did not take inline sequences inside traits into consideration. Since trait names are not unique, it is possibly to get a `FactoryBot::DuplicateDefinitionError` when defining two sequences that have the same name in two traits that have the same name. This PR abandons the idea of "privately" registering inline sequences, and instead keeps a separate list of all the inline sequences just for the purpose of rewinding them.

Rewinding all sequences and not just the global ones

e79d2ca

houndci-bot reviewed Nov 6, 2017

View reviewed changes

Damian Le Nouaille added 2 commits November 6, 2017 09:53

patch style

91d982d

more spec and multiple factory scenario, plus a patch for proc

92bd3af

houndci-bot reviewed Nov 6, 2017

View reviewed changes

remove space

7d0fece

composerinteralia requested changes Jun 17, 2018

View reviewed changes

composerinteralia reviewed Jun 17, 2018

View reviewed changes

composerinteralia added the 5.0 label Jul 8, 2018

composerinteralia mentioned this pull request Aug 4, 2018

Register inline sequence to allow for rewinding #1164

Merged

composerinteralia added this to the 5.0 Release milestone Aug 4, 2018

composerinteralia removed the 5.0 label Aug 4, 2018

composerinteralia closed this Aug 5, 2018

composerinteralia mentioned this pull request Feb 5, 2019

Allow inline sequences in traits to have same name #1258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Rewinding all sequences and not just the global ones #1078

Rewinding all sequences and not just the global ones #1078

Uh oh!

damln commented Nov 6, 2017

Uh oh!

houndci-bot Nov 6, 2017

Uh oh!

damln Nov 6, 2017

Uh oh!

houndci-bot Nov 6, 2017

Uh oh!

damln Nov 6, 2017

Uh oh!

houndci-bot Nov 6, 2017

Uh oh!

damln Nov 6, 2017

Uh oh!

composerinteralia left a comment

Uh oh!

composerinteralia Jun 17, 2018

Uh oh!

composerinteralia Jun 17, 2018

Uh oh!

composerinteralia Jun 17, 2018

Uh oh!

composerinteralia Jun 17, 2018

Uh oh!

composerinteralia Jun 17, 2018 •

edited

Loading

Uh oh!

composerinteralia commented Aug 5, 2018

Uh oh!

Uh oh!

Uh oh!

Rewinding all sequences and not just the global ones #1078

Rewinding all sequences and not just the global ones #1078

Uh oh!

Conversation

damln commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

composerinteralia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

composerinteralia Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

composerinteralia commented Aug 5, 2018

Uh oh!

Uh oh!

composerinteralia Jun 17, 2018 •

edited

Loading