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

Conversation

@composerinteralia
Copy link
Member

composerinteralia commented Apr 10, 2020

Enum traits

This PR adds additional test coverage and finalizes the work started in #1301.

Given a Rails model with an enum attribute:

class Task < ActiveRecord::Base
  enum status: {queued: 0, started: 1, finished: 2}
end

It is common for people to define traits for each possible value of the enum:

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

    trait :started do
      status { :started }
    end

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

With this commit, those trait definitions are no longer necessary—they are defined automatically by factory_bot.

If automatically defining traits for enum attributes on every factory is not desired, it is possible to disable the feature by setting FactoryBot.automatically_define_enum_traits = false (see commit: Allow opting out of automatically defining traits).

In that case, it is still possible to explicitly define traits for an enum attribute in a particular factory:

FactoryBot.automatically_define_enum_traits = false

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

It is also possible to use this feature for other enumerable values, not specifically tied to ActiveRecord enum attributes:

class Task
  attr_accessor :status
end

FactoryBot.define do
  factory :task do
    traits_for_enum(:status, ["queued", "started", "finished"])
  end
end

The second argument here can be an enumerable object, including a Hash or Array.

Closes #1049

composerinteralia and others added 7 commits Feb 3, 2019
Co-authored-by: Lance Johnson <lancejjohnson@gmail.com>
* Case for building traits from any type of enum
  Refactored Enum#enum_values to get this test passing

* Case for preferring user defined traits over automatically built traits
While I think it makes sense to default to enabling this feature, there
may be cases where this is a breaking change and somebody wants to opt
out of the feature.

If somebody disables the automatic feature, they can still manually call
`traits_for_enum`.
Before this commit the `Factory` class needed to know about both
automatically registering enums and then expanding them. With this
commit automatic registration is handled within a private method in the
`Definition` so the `Factory` can remain ignorant of this step.
@@ -84,6 +84,7 @@ def compile
unless @compiled
parent.compile
parent.defined_traits.each { |trait| define_trait(trait) }
@definition.expand_enum_traits(build_class)
@definition.compile

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 10, 2020

Author Member

In an ideal world we could pass the build_class to @definition.compile and let the definition itself handle expanding the traits, but we call @definition.compile in other places where a build_class is not available.

This comment has been minimized.

Copy link
@eLod

eLod Apr 24, 2020

Contributor

i agree with placing the triggering logic in @definition.compile would be better, but i also agree that it may not worth it to refactor other parts largely only for this.

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 24, 2020

Author Member

I pushed up 9c4d7c5, which solves this but adds an extra conditional to Definition#compile. Do you think that is worth it?

@composerinteralia composerinteralia mentioned this pull request Apr 10, 2020
@fridaland fridaland changed the title Increase test coverage enum traits Add functionality for enum traits Apr 10, 2020
Add documentation to DefinitionProxy#traits_for_enum, and include
@api private comment to new Enum class.
- automatically_define_enum_traits = false
- automatically_define_enum_traits = true

When setting `FactoryBot.automatically_define_enum_traits = false`, we
allow for 'turning off' automatically built traits from enum. Add test
to confirm that attempting to build automatically while this is false
raises an error.
# end
#
# Arguments:
# * name_of_enum_type_attribute: +Symbol+

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 17, 2020

Author Member

I don't think we use this name anywhere else. I think we should stick to the argument names we use in the method signature itself.

# traits_for_enum :status, statuses
# end
#
# Arguments:

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 17, 2020

Author Member

I am finding it a little confusing to have multiple argument lists. Elsewhere I think we have consolidated them into one.

I am thinking:

# 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 from the class of the object being built by calling the
#     pluralized `attribute_name` class method.

This comment has been minimized.

Copy link
@fridaland

fridaland Apr 21, 2020

Contributor

It makes sense to consolidate, I was questioning whether or not having multiple argument lists would be valuable.

# * Where the Array has enum values
# statuses = %w[queued started finished]
#
# * Where the CustomEnumClass has enum values

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 17, 2020

Author Member

I don't think we need this. This is teaching people how to build custom enumerables, which is beyond the scope of this documentation.

This comment has been minimized.

Copy link
@fridaland

fridaland Apr 21, 2020

Contributor

Fair point!

@composerinteralia
Copy link
Member Author

composerinteralia commented Apr 17, 2020

@fridaland I pushed up a commit to move some things around in the documentation based on my comments above. Let me know what you think!

@composerinteralia
Copy link
Member Author

composerinteralia commented Apr 17, 2020

I still need to work on adding some documentation to the GETTING_STARTED guided, but I was waiting until #1381 gets reviewed.

@fridaland
Copy link
Contributor

fridaland commented Apr 21, 2020

@composerinteralia updates look good. Thanks for clarifying the documentation!

@eLod
eLod approved these changes Apr 24, 2020
Copy link
Contributor

eLod left a comment

other than the small things for tests (and again those are not very important and some even debatable) it looks good

@@ -84,6 +84,7 @@ def compile
unless @compiled
parent.compile
parent.defined_traits.each { |trait| define_trait(trait) }
@definition.expand_enum_traits(build_class)
@definition.compile

This comment has been minimized.

Copy link
@eLod

eLod Apr 24, 2020

Contributor

i agree with placing the triggering logic in @definition.compile would be better, but i also agree that it may not worth it to refactor other parts largely only for this.

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

Task.reset_column_information

This comment has been minimized.

Copy link
@eLod

eLod Apr 24, 2020

Contributor

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

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 24, 2020

Author Member

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.

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 }

This comment has been minimized.

Copy link
@eLod

eLod Apr 24, 2020

Contributor

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)

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 24, 2020

Author Member

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

end

Task.reset_column_information
end

This comment has been minimized.

Copy link
@eLod

eLod Apr 24, 2020

Contributor

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

This comment has been minimized.

Copy link
@composerinteralia

composerinteralia Apr 24, 2020

Author Member

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.

This keeps Factory completely ignorant of enum traits. The downside is
that it adds an extra conditional the Definition#compile, since trait
definitions do not have access to the build class.
@composerinteralia
Copy link
Member Author

composerinteralia commented Apr 24, 2020

@eLod thanks so much for taking a look. I left some comments on your comments and pushed up two additional commits.

@composerinteralia composerinteralia merged commit 975fc4f into master May 1, 2020
3 checks passed
3 checks passed
Hound Smells good to me. Woof!
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@composerinteralia composerinteralia deleted the increase-test-coverage-enum-traits branch May 1, 2020
composerinteralia added a commit that referenced this pull request Jun 19, 2020
This commit adds top-level documentation for the enum traits feature
adding in #1380
composerinteralia added a commit that referenced this pull request Jun 19, 2020
This commit adds top-level documentation for the enum traits feature
adding in #1380
composerinteralia added a commit that referenced this pull request Jun 19, 2020
This commit adds top-level documentation for the enum traits feature
adding in #1380
composerinteralia added a commit that referenced this pull request Jun 19, 2020
This commit adds top-level documentation for the enum traits feature
adding in #1380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.