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
Collaborator

@composerinteralia 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 February 3, 2019 17:23
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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+
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 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:
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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
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 don't think we need this. This is teaching people how to build custom enumerables, which is beyond the scope of this documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point!

@composerinteralia
Copy link
Collaborator Author

@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
Collaborator Author

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

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

Copy link
Contributor

@eLod eLod left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
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.

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

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.

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
Collaborator Author

@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
@composerinteralia composerinteralia deleted the increase-test-coverage-enum-traits branch May 1, 2020 21:50
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature question: Boilerplate for enum traits
3 participants