BUGFIX: Parent/Child implicit trait results depend on build order.

[CodeMeister]

Fixes: #1716

Summary

When a parent with an implicit trait is built at the same time as a child
that inherits, and calls, that trait, the result changes based on which is built first.

In the scenario below a :user factory and its child factory :admin are
both created at the same time, but in a different order within each test.

Keeping everything the same, except the order in which the factories are run, raises a different error in each test.

Scenario

factory :user do  
  trait :new_name do
    name { 'User' }
  end

  trait :change_name do
    new_name
  end

  factory :admin do
    trait :new_name do
      name { 'Admin' }
    end
  end
end

Test User First

it "should assign the correct names when user created first" do
  user  = create :user, :change_name
  admin = create :admin, :change_name

  user.name  #=> 'User'
  admin.name #=> 'User'

  expect(user.name).to  eq 'User'
  expect(admin.name).to eq 'Admin')
end

# Failure/Error: expect(admin.name).to eq "Admin"
#
#       expected: "Admin"
#            got: "User"

Test Admin First

it "should assign the correct names when admin created first" do
  admin = create :admin, :change_name
  user  = create :user, :change_name

  user.name  #=> 'Admin'
  admin.name #=> 'Admin'

  expect(user.name).to  eq 'User'
  expect(admin.name).to eq 'Admin')
end

# Failure/Error: expect(user.name).to eq "User"
#
#       expected: "User"
#            got: "Admin"

Note:
There is no issue when either factory is built and tested on their own,
only when built together and an implied trait is used by both.

The Cause

The cause, was that an implicit trait is evaluated in the context of its
first run, storing the reference to the implied trait.

In the scenario above where :admin is built first:

1. Admin factory starts to run
2. the `:change_name' trait is called for the first time and evaluated
3. the evaluation maps the implicit new_name call to the admin's :new_name trait
4. the admin's ':new_name' trait is run and sets the name to 'Admin'
5. User factory starts to run
6. the :change_name trait has already been evaluated (still pointing to admin's :new_name)
7. the admin's ':new_name' trait is run and sets the name to 'Admin'

The Solution

When running a child factory, it inherits the parent's traits. The solution was two-fold:

1. don't inherit a trait if a trait with the same name already exists.
2. clone all inherited traits so they are re-evaluated in the child's context.

Note: Cloning ALL inherited traits is a bit over-kill, but it's actually faster
than detecting and cloning only the implicit traits; and should help future-proof any other
context issues.

Changes

In Definition I added a new helper method to return an array of the names for all traits already defined:

# lib/factory_bot/definition.rb:98
def defined_traits_names
  @defined_traits.map(&:name)
end

In Trait I added a method to create a new 'clean' clone of the trait:

# lib/factory_bot/trait.rb:17
def clone
  Trait.new(name, &block)
end

In Factory I added :defined_traits_names to the @definition delegation list:

delegate :add_callback, :declare_attribute, :to_create, :define_trait, :constructor,
  :defined_traits, :defined_traits_names, :inherit_traits, :append_traits,
  to: :@definition

I added a new private method :inherit_parent_traits to manage the inheritance:

def inherit_parent_traits
  parent.defined_traits.each do |trait|
    next if defined_traits_names.include?(trait.name)
    define_trait(trait.clone)
  end
end

And finally I altered :compile to call the new :inherit_parent_traits method:

def compile
  unless @compiled
    parent.compile
    inherit_parent_traits
    @definition.compile(build_class)
    build_hierarchy
    @compiled = true
  end
end

Testing

• added tests to spec/acceptance/traits_spec.
• all test passing.

😃

[kucho]

@neilvcarvalho could you take a look at this PR, please? 🙏 I would love to get this merged, this is failing consistently in our CI.

[neilvcarvalho]

@kucho I should have time to look at this PR this Friday. I tagged myself so I remember.

[neilvcarvalho]

Looks great! As the suggestions I made were simple exclusions, I'm going to merge them so I can merge the PR right away and include in the release I'll create today.

[ngan]

I think the change caused a regression: #1738