Proposal: Introducing traits

[germsvel]

I find myself creating many helper functions in my factory file that change one or a few attributes of a factory. For example,

# factory file
def user_factory do
  %User{
    name: "user-name",
    email: "[email protected]",
    admin: false,
    status: :inactive
  }
end

def make_admin(user) do
  %{user | admin: true}
end

def set_active(user) do
  %{user | status: :active}
end

# use in test
user = build(:user) |> make_admin() |> set_active()

Alternatively, I see people not defining those small functions and always having to pass attributes into the factories like so,

user = build(:user, admin: true, status: :active)

I've been thinking that it would be nice to have a helper that merges all those attributes if they are defined as traits.

# factory file
def user_factory do
  %User{
    name: "user-name",
    email: "[email protected]",
    admin: false,
    status: :inactive
  }
end

def admin_trait do
  %{admin: true}
end

def active_trait do
  %{status: :active}
end

# use in test
user = build(:user) |> traits(:admin, :active)

Upsides

• I think this could clean up a lot of small function definitions in the factory file if that's how you're writing it.

# from
def make_admin(user) do
  %{user | admin: true}
end

# to
def admin_trait do
  %{admin: true}
end

• It would reduce the amount of piping needed when using the factory.

# from
build(:user) |> make_admin() |> set_active()

# to
build(:user) |> traits(:admin, :active)

• Traits are just maps, so they can be reused across multiple factories.

build(:user) |> traits(:admin)
build(:customer) |> traits(:admin)

• It makes the intent more clear: instead of needing to worry about the details of what it means for a user to be active, the test can just state that the user should be active. The factory knows that in order for that to happen, the status must be set to :active, but that knowledge is not spread out throughout all your tests, which is the case when we do build(:user, status: :active) everywhere.

Possible downsides

• It adds another layer of "magic", even though we'd simply be calling struct! or Map.merge underneath for each trait.

• It does not handle the case when we insert very well,

# cannot do this
insert(:user) |> traits(:admin, :active)

# have to do this
build(:user) |> traits(:admin, :active) |> insert()

Of course since the aim is to replace this,

build(:user) |> make_admin() |> set_active() |> insert()

the alternative of

build(:user) |> traits(:admin, :active) |> insert()

might be totally fine.

Still, people might think that since it's part of ExMachina, it should "just work" with insert.

What do you think?

I'm interested in hearing what people think of this idea. Would it be helpful? Any downsides (or upsides) I am missing? Also not sure if trait is the best word for them, so if people have suggestions, I'm happy to hear them!

[maymillerricci]

@germsvel Interesting! I've been thinking about this some and going a little back and forth but ultimately I think it could definitely be worthwhile as another option.

In general I think it makes sense to use derived factories if there's a combination of attributes that you're using a few times, but I can see the appeal of having each item be its own trait that can be combined any number of ways, vs. needing to specify a full factory definition for each possible combination of attributes that you'd want.

The idea that they're not tied to a specific struct type is an interesting one - so anything with an active field could make use of the active trait. There's something that feels a little bit funny about that to me, but then again I can see the argument that that actually makes them very flexible.

The fact that you need to do the full build(:user) |> traits(:admin, :active) |> insert() to do an insert maybe isn't the most ideal, but I think as long it's well documented it's a reasonable thing to have.

And I think trait seems like a clear word to me for what this is, especially coming from Factory Bot.

[frm]

I'm actually on the fence on this. I tend to prefer less "magic" happening around. I prefer the approach of derived factories, like @maymillerricci mentioned.

Given the nature of Elixir, I do think I would personally prefer a nice pipeline with small steps (build(:user) |> make_admin() |> set_active()) rather than something like build(:user) |> traits(:admin, :active). This might be just a personal taste, however. It feels more functional, at least to me, to compose and apply small data transformations, rather than having a set of traits, which seems a bit more imperative.

The idea that traits can be flexible and applied to any struct that has an active field confuses me a bit, as well. Structs are (almost) well-defined data types: you can't dynamically add more fields to it and, even though Elixir is dynamically typed, you can use typespecs to try to enforce the data types of certain fields. Having a trait to act upon different structs makes room for error. Imagine the following example.

defmodule Manager do
   defstruct [:name, :admin]
end

defmodule Employee do
  defstruct [:name, :role]
end

defmodule User do
  defstruct [:admin]
end

For a manager, the admin field could very well be an administrator id. For the user, it could be the boolean indicating if it has admin access or not. For the employee, even though it has no admin field, we can have an admin role. So we could have the following scenarios, all valid:

# for Users
def admin_trait do
  %{admin: true}
end

# for Employees
def admin_trait do
  %{role: "admin"}
end

# for Managers
def admin_trait do
  %{admin: 1}
end

It would make sense that we would have to define a trait for each data type or name a different trait accordingly. Both cases can be dealt with easily by using derived factories or function composition.

Using traits correctly and carefully comes down to programmer discipline. The example I presented is farfetched but having a transversal trait dependent on the field and not on the data type could be error prone. We could be assuming all admin fields have the same data type and it could well induce programmers in error.

One a side note, derived factories don't really need the struct!/2 call, do they? Wouldn't this do?

  def featured_article_factory do
    %{article_factory() | featured: true}
  end

I would probably find myself using this approach, especially in Ecto apps, since the insert case with traits comes off as rather verbose.

[prem-prakash]

the problem with pipeline with small steps (build(:user) |> make_admin() |> set_active()) is that it gets mixed with other factories, so if you have for example another kind of schema that need the same make_admin you will have to prepend or append factory name, and it becomes larger build(:user) |> make_user_admin() |> set_user_active() build(:student) |> make_student_admin() |> set_student_active()

[germsvel]

After thinking about this some more, I do agree that the trait approach of just defining a map is a little too "magical" and less clear than it could be. But I still think we could benefit from a helper function that basically reduces over a set of modifier functions.

My current thoughts on this would be to make a pipe_through function that takes a list of functions to apply on the result, each of which should take the previous map/struct as it's first and only argument:

# factory file
def user_factory do
  %User{
    name: "user-name",
    email: "[email protected]",
    admin: false,
    status: :inactive
  }
end

def admin(user) do
  %{user | admin: true}
end

def active(user) do
  %{user | status: :active}
end

# use in test
user = build(:user) |> pipe_through([:admin, :active]) |> insert()

In my mind, this behaves similar to Phoenix's pipe_through/1 function for pipelines and ExUnit's setup/2 when used with multiple functions (e.g. setup [:setup1, :setup2]), so it might be less magical than the previous proposal and more familiar to people who already use Phoenix and ExUnit.

[Hermanverschooten]

I am simply doing:

def admin do
  build(:user, admin: true)
end

and set the options in user to the most commonly used, so that I only need to override, eg active in a specific test.