Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality for enum traits #1380

Merged
merged 12 commits into from May 1, 2020
Merged
4 changes: 4 additions & 0 deletions lib/factory_bot.rb
Expand Up @@ -31,6 +31,7 @@
require "factory_bot/sequence"
require "factory_bot/attribute_list"
require "factory_bot/trait"
require "factory_bot/enum"
require "factory_bot/aliases"
require "factory_bot/definition"
require "factory_bot/definition_proxy"
Expand Down Expand Up @@ -61,6 +62,9 @@ def self.reset_configuration
mattr_accessor :use_parent_strategy, instance_accessor: false
self.use_parent_strategy = true

mattr_accessor :automatically_define_enum_traits, instance_accessor: false
self.automatically_define_enum_traits = true

# Look for errors in factories and (optionally) their traits.
# Parameters:
# factories - which factories to lint; omit for all factories
Expand Down
31 changes: 29 additions & 2 deletions lib/factory_bot/definition.rb
@@ -1,13 +1,14 @@
module FactoryBot
# @api private
class Definition
attr_reader :defined_traits, :declarations, :name
attr_reader :defined_traits, :declarations, :name, :registered_enums

def initialize(name, base_traits = [])
@name = name
@declarations = DeclarationList.new(name)
@callbacks = []
@defined_traits = Set.new
@registered_enums = []
@to_create = nil
@base_traits = base_traits
@additional_traits = []
Expand Down Expand Up @@ -43,8 +44,10 @@ def callbacks
aggregate_from_traits_and_self(:callbacks) { @callbacks }
end

def compile
def compile(klass = nil)
unless @compiled
expand_enum_traits(klass) unless klass.nil?

declarations.attributes

defined_traits.each do |defined_trait|
Expand Down Expand Up @@ -81,6 +84,10 @@ def define_trait(trait)
@defined_traits.add(trait)
end

def register_enum(enum)
@registered_enums << enum
end

def define_constructor(&block)
@constructor = block
end
Expand Down Expand Up @@ -135,5 +142,25 @@ def aggregate_from_traits_and_self(method_name, &block)
additional_traits.map(&method_name),
].flatten.compact
end

def expand_enum_traits(klass)
if automatically_register_defined_enums?(klass)
automatically_register_defined_enums(klass)
end

registered_enums.each do |enum|
traits = enum.build_traits(klass)
traits.each { |trait| define_trait(trait) }
end
end

def automatically_register_defined_enums(klass)
klass.defined_enums.each_key { |name| register_enum(Enum.new(name)) }
end

def automatically_register_defined_enums?(klass)
FactoryBot.automatically_define_enum_traits &&
klass.respond_to?(:defined_enums)
end
end
end
58 changes: 58 additions & 0 deletions lib/factory_bot/definition_proxy.rb
Expand Up @@ -176,6 +176,64 @@ def trait(name, &block)
@definition.define_trait(Trait.new(name, &block))
end

# Creates traits for enumerable values.
#
# Example:
# factory :task do
# traits_for_enum :status, [:started, :finished]
# end
#
# Equivalent to:
# factory :task do
# trait :started do
# status { :started }
# end
#
# trait :finished do
# status { :finished }
# end
# end
#
# Example:
# factory :task do
# traits_for_enum :status, {started: 1, finished: 2}
# end
#
# Example:
# class Task
# def statuses
# {started: 1, finished: 2}
# end
# end
#
# factory :task do
# traits_for_enum :status
# end
#
# Both equivalent to:
# factory :task do
# trait :started do
# status { 1 }
# end
#
# trait :finished do
# status { 2 }
# end
# end
#
#
# Arguments:
# attribute_name: +Symbol+ or +String+
# the name of the attribute these traits will set the value of
# values: +Array+, +Hash+, or other +Enumerable+
# An array of trait names, or a mapping of trait names to values for
# those traits. When this argument is not provided, factory_bot will
# attempt to get the values by calling the pluralized `attribute_name`
# class method.
def traits_for_enum(attribute_name, values = nil)
fridaland marked this conversation as resolved.
Show resolved Hide resolved
@definition.register_enum(Enum.new(attribute_name, values))
end

def initialize_with(&block)
@definition.define_constructor(&block)
end
Expand Down
27 changes: 27 additions & 0 deletions lib/factory_bot/enum.rb
@@ -0,0 +1,27 @@
module FactoryBot
# @api private
class Enum
def initialize(attribute_name, values = nil)
@attribute_name = attribute_name
@values = values
end

def build_traits(klass)
enum_values(klass).map do |trait_name, value|
build_trait(trait_name, @attribute_name, value || trait_name)
end
end

private

def enum_values(klass)
@values || klass.send(@attribute_name.to_s.pluralize)
end

def build_trait(trait_name, attribute_name, value)
Trait.new(trait_name) do
add_attribute(attribute_name) { value }
end
end
end
end
2 changes: 1 addition & 1 deletion lib/factory_bot/factory.rb
Expand Up @@ -84,7 +84,7 @@ def compile
unless @compiled
parent.compile
parent.defined_traits.each { |trait| define_trait(trait) }
@definition.compile
@definition.compile(build_class)
build_hierarchy
@compiled = true
end
Expand Down
161 changes: 161 additions & 0 deletions spec/acceptance/enum_traits_spec.rb
@@ -0,0 +1,161 @@
describe "enum traits" do
context "when automatically_define_enum_traits is true" do
it "builds traits automatically for model enum field" do
define_model("Task", status: :integer) do
enum status: { queued: 0, started: 1, finished: 2 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are somewhat also redundant, so they are used in every it the same or just the keys as a string array, it may make it more readable to do a single let for them for the whole describe block (again obviously not high prio)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally not a fan of let for the reasons outlined in this post: https://thoughtbot.com/blog/lets-not.

end

FactoryBot.define do
factory :task
end

Task.statuses.each_key do |trait_name|
task = FactoryBot.build(:task, trait_name)

expect(task.status).to eq(trait_name)
end

Task.reset_column_information
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these may be better in an after hook so it's more readable (obviously not high prio)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me realize that not all of these tests need models, so I changed some in 1f70160 to use plain old Ruby objects instead. It no longer makes sense to call Task.reset_column_information in an after hook since Task won't have that method in every test.

end

it "prefers user defined traits over automatically built traits" do
define_model("Task", status: :integer) do
enum status: { queued: 0, started: 1, finished: 2 }
end

FactoryBot.define do
factory :task do
trait :queued do
status { :finished }
end

trait :started do
status { :finished }
end

trait :finished do
status { :finished }
end
end
end

Task.statuses.each_key do |trait_name|
task = FactoryBot.build(:task, trait_name)

expect(task.status).to eq("finished")
end

Task.reset_column_information
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to some extent the examples could be refactored to use a shared example, but i am not sure at what point it is just "clever" overengineering vs making it more readable. generally all the tests are doing

  • define the "same" model with integer or string column, optionally adding an (AR) enum
  • define the factory with or without explicit traits, obviously this is the main part, not strictly the subject of the tests but close (so the less code share available)
  • assert the factory's traits building with the right attribute value

i would argue the first and last steps are very easy to share between the examples (but again, at some point it will be counter-productive), obviously low prio & more debatable

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be some room to pull out some shared methods here, but lately I have been preferring repetition in my tests over clever shared behavior. I think https://thoughtbot.com/blog/the-case-for-wet-tests covers a lot of what I am thinking. I also talk a little bit about my preference for duplication in tests in https://www.bikeshed.fm/186.


it "builds traits for each enumerated value using a provided list of values as a Hash" do
statuses = { queued: 0, started: 1, finished: 2 }

define_class "Task" do
attr_accessor :status
end

FactoryBot.define do
factory :task do
traits_for_enum :status, statuses
end
end

statuses.each do |trait_name, trait_value|
task = FactoryBot.build(:task, trait_name)

expect(task.status).to eq(trait_value)
end
end

it "builds traits for each enumerated value using a provided list of values as an Array" do
statuses = %w[queued started finished]

define_class "Task" do
attr_accessor :status
end

FactoryBot.define do
factory :task do
traits_for_enum :status, statuses
end
end

statuses.each do |trait_name|
task = FactoryBot.build(:task, trait_name)

expect(task.status).to eq(trait_name)
end
end

it "builds traits for each enumerated value using a custom enumerable" do
statuses = define_class("Statuses") do
include Enumerable

def each(&block)
["queued", "started", "finished"].each(&block)
end
end.new

define_class "Task" do
attr_accessor :status
end

FactoryBot.define do
factory :task do
traits_for_enum :status, statuses
end
end

statuses.each do |trait_name|
task = FactoryBot.build(:task, trait_name)

expect(task.status).to eq(trait_name)
end
end
end

context "when automatically_define_enum_traits is false" do
it "raises an error for undefined traits" do
with_temporary_assignment(FactoryBot, :automatically_define_enum_traits, false) do
define_model("Task", status: :integer) do
enum status: { queued: 0, started: 1, finished: 2 }
end

FactoryBot.define do
factory :task
end

Task.statuses.each_key do |trait_name|
expect { FactoryBot.build(:task, trait_name) }.to raise_error(
KeyError, "Trait not registered: \"#{trait_name}\""
)
end

Task.reset_column_information
end
end

it "builds traits for each enumerated value when traits_for_enum are specified" do
with_temporary_assignment(FactoryBot, :automatically_define_enum_traits, false) do
define_model("Task", status: :integer) do
enum status: { queued: 0, started: 1, finished: 2 }
end

FactoryBot.define do
factory :task do
traits_for_enum(:status)
end
end

Task.statuses.each_key do |trait_name|
task = FactoryBot.build(:task, trait_name)

expect(task.status).to eq(trait_name)
end

Task.reset_column_information
end
end
end
end
10 changes: 10 additions & 0 deletions spec/factory_bot/definition_spec.rb
Expand Up @@ -72,4 +72,14 @@

expect(definition.to_create).to eq block
end

it "maintains a list of enum fields" do
definition = described_class.new(:name)

enum_field = double("enum_field")

definition.register_enum(enum_field)

expect(definition.registered_enums).to include(enum_field)
end
end
3 changes: 3 additions & 0 deletions spec/factory_bot/factory_spec.rb
Expand Up @@ -17,6 +17,7 @@
end

it "returns associations" do
define_class("Post")
factory = FactoryBot::Factory.new(:post)
FactoryBot::Internal.register_factory(FactoryBot::Factory.new(:admin))
factory.declare_attribute(FactoryBot::Declaration::Association.new(:author, {}))
Expand All @@ -32,6 +33,7 @@
association_on_parent = FactoryBot::Declaration::Association.new(:association_on_parent, {})
association_on_child = FactoryBot::Declaration::Association.new(:association_on_child, {})

define_class("Post")
factory = FactoryBot::Factory.new(:post)
factory.declare_attribute(association_on_parent)
FactoryBot::Internal.register_factory(factory)
Expand Down Expand Up @@ -134,6 +136,7 @@

it "creates a new factory while overriding the parent class" do
name = :user
define_class("User")
factory = FactoryBot::Factory.new(name)
FactoryBot::Internal.register_factory(factory)

Expand Down