From 648117cd48b7b3acd22cbae2b5a0e322a4115397 Mon Sep 17 00:00:00 2001 From: German Velasco Date: Sat, 12 Dec 2020 05:06:33 -0500 Subject: [PATCH 1/2] Use a real struct (not named struct) in ExMachinaTest We used to have a struct factory defined as `%{__struct__: FooBar}` but it was confusing since the factory name was `struct_factory`. So we change to be `foo_bar_factory` and create a module with a struct called `FooBar`. This has the benefit of allowing us to assert `%FooBar{}` in tests -- something we couldn't do with the previous format. --- test/ex_machina_test.exs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/ex_machina_test.exs b/test/ex_machina_test.exs index f9a682d..8bd87bb 100644 --- a/test/ex_machina_test.exs +++ b/test/ex_machina_test.exs @@ -1,6 +1,10 @@ defmodule ExMachinaTest do use ExUnit.Case + defmodule FooBar do + defstruct [:name] + end + defmodule Factory do use ExMachina @@ -24,10 +28,8 @@ defmodule ExMachinaTest do } end - def struct_factory do - %{ - __struct__: Foo.Bar - } + def foo_bar_factory do + %FooBar{} end def comment_factory(attrs) do @@ -94,7 +96,7 @@ defmodule ExMachinaTest do test "build/2 raises if passing invalid keys to a struct factory" do assert_raise KeyError, fn -> - Factory.build(:struct, doesnt_exist: true) + Factory.build(:foo_bar, doesnt_exist: true) end end From ffb86d512bed3ecfdaaf8cf25e39402ef5e8bc71 Mon Sep 17 00:00:00 2001 From: German Velasco Date: Tue, 8 Dec 2020 05:55:49 -0500 Subject: [PATCH 2/2] Introduce `build_lazy/2` The problem ------------ People want a different email per account (especially if the email factory has a sequence), but they do this: ```elixir build_pair(:account, email: build(:email)) ``` The problem is that `build/2` is just function calling. So the above is equivalent to this: ```elixir email = build(:email) build_pair(:account, email: email) ``` In other words, we get on email factory for all of the accounts. The problem is made worse when using it with Ecto. We can imagine the following scenario: ```elixir insert_pair(:account, user: build(:user)) ``` If the user factory has a uniqueness constraint, `insert_pair/2` will raise an error because we'll try to insert a user with the same value (even if using a sequence). Solution -------- The solution is to delay evaluation of the factory. We do this by introducing a private struct `%ExMachina.InstanceTemplate{}` to hold the data necessary to build the instance of that factory. Note that this struct is private and liable to change, so users of the library shouldn't depend on it. The trick then lies in the `build/2` function. We make it a terminal function in that it will evaluate any lazy factories recursively. To do that, we update the `build/2` function to evaluate `ExMachina.InstanceTemplate` structs after building the `build/2` factory. In an early implementation of this feature, I evaluated the lazy factories when we first got the attributes in `build/2` (i.e. `attrs |> evaluate_lazy_factories() |> Enum.into(%{})`). But then I opted for evaluating the lazy factories after the factory is created because that allows us to use `build_lazy/2` from within factory definitions, and not just when combined with `build/2` or `build_*` functions. How it interacts with "full-control" factories --------------------------------------------- We evaluate lazy factories before passing the attrs to the factories that have full control. That means that we can still use `build_lazy` within `build/2` and `build_*` functions that have a factory with full control, but we cannot use `build_lazy/2` as part of the definition of a factory that has full control. In other words, we can do this: ```elixir def account_factory(attrs) do %{ user: build(:user), ... } end build(:account, build_lazy(:user)) ``` But we cannot do this: ```elixir def account_factory(attrs) do %{ user: build_lazy(:user), ... } end ``` This seems like the best compromise we can make so that people can continue using factories with full control, but without the ability to define lazy factories in the definition -- since ExMachina does nothing after the factory is defined. Notes on the name `InstanceTemplate` ----------------------------------- I call it `InstanceTemplate` because it's not exactly an instance of a factory. I only has the recipe to build a factory in that it has the factory `name` and the extra attributes. But, evaluating the instance template twice when the underlying factory has a sequence will not result in identical factories. In other words, given the factory: ```elixir def user_factory do %{email: sequence("email")} end ``` Then evaluating the same build lazy template will not give use the same sequence: ```elixir template = build_lazy(:user) InstanceTemplate.evaluate(template) != InstanceTemplate.evaluate(template) ``` Potential future work --------------------- This work opens the possibility of passing the built factory as an argument to `build_lazy` in a factory definition, but we'd have to modify how it works. I leave that for future work, if indeed that's something people want. Here's an example of how it could work: ```elixir def account_factory do %{ private: true, profile: build_lazy(fn account -> build(:profile, private: account.private end) } end ``` --- lib/ex_machina.ex | 86 +++++++++++++++++++- lib/ex_machina/instance_template.ex | 10 +++ priv/test_repo/migrations/1_migrate_all.exs | 28 ++++--- test/ex_machina/ecto_test.exs | 19 +++++ test/ex_machina_test.exs | 87 +++++++++++++++++++-- test/support/models/publisher.ex | 3 +- test/support/test_factory.ex | 4 +- 7 files changed, 216 insertions(+), 21 deletions(-) create mode 100644 lib/ex_machina/instance_template.ex diff --git a/lib/ex_machina.ex b/lib/ex_machina.ex index 34717dc..306b3dd 100644 --- a/lib/ex_machina.ex +++ b/lib/ex_machina.ex @@ -42,6 +42,10 @@ defmodule ExMachina do ExMachina.build(__MODULE__, factory_name, attrs) end + def build_lazy(factory_name, attrs \\ %{}) do + ExMachina.build_lazy(__MODULE__, factory_name, attrs) + end + def build_pair(factory_name, attrs \\ %{}) do ExMachina.build_pair(__MODULE__, factory_name, attrs) end @@ -165,8 +169,16 @@ defmodule ExMachina do If you want full control over the factory attributes, you can define the - factory with `[factory_name]_factory/1`. Note that you will need to merge the - attributes passed if you want to emulate ExMachina's default behavior. + factory with `[factory_name]_factory/1`. + + Caveats: + + - ExMachina will no longer merge the attributes for your factory. If you want + to do that, you can merge the attributes with the `merge_attributes/2` helper. + + - You cannot use `c:build_lazy/2` within the factory definition, since ExMachina + will no longer perform it's usual evaluation of lazy factories after the + factory creation. ## Example @@ -192,20 +204,53 @@ defmodule ExMachina do @doc false def build(module, factory_name, attrs \\ %{}) do attrs = Enum.into(attrs, %{}) + function_name = build_function_name(factory_name) cond do factory_accepting_attributes_defined?(module, function_name) -> + attrs = evaluate_lazy_factories(attrs) apply(module, function_name, [attrs]) factory_without_attributes_defined?(module, function_name) -> - apply(module, function_name, []) |> merge_attributes(attrs) + apply(module, function_name, []) + |> merge_attributes(attrs) + |> evaluate_lazy_factories() true -> raise UndefinedFactoryError, factory_name end end + defp evaluate_lazy_factories(%{__struct__: record} = factory) do + struct!( + record, + factory |> Map.from_struct() |> evaluate_lazy_factories() + ) + end + + defp evaluate_lazy_factories(attrs) when is_map(attrs) do + attrs + |> Enum.map(fn + {k, %ExMachina.InstanceTemplate{} = v} -> + {k, ExMachina.InstanceTemplate.evaluate(v)} + + {k, list} when is_list(list) -> + {k, evaluate_lazy_factories_in_list(list)} + + {_, _} = tuple -> + tuple + end) + |> Enum.into(%{}) + end + + defp evaluate_lazy_factories_in_list(list) do + Enum.map(list, fn + %ExMachina.InstanceTemplate{} = instance -> ExMachina.InstanceTemplate.evaluate(instance) + item -> item + end) + end + defp build_function_name(factory_name) do factory_name |> Atom.to_string() @@ -221,6 +266,41 @@ defmodule ExMachina do Code.ensure_loaded?(module) && function_exported?(module, function_name, 0) end + @doc """ + Builds a factory instance that won't be evaluated immediately. As such, this + function should not be used on its own, but should be combined with + `c:build/2`, `c:build_pair/2`, `c:build_list/3`, or it should be used in a + factory definition. + + `build_lazy/2` is evaluated as part of one of the other functions, and it is + particularly useful when using it with `c:build_pair/2` or `c:build_list/3`. + + For example, people might want to build a separate user per account. + + ## Example + + def user_factory do + %{name: "John Doe", username: sequence("johndoe")} + end + + # build/2 is eager + build_pair(:account, user: build(:user)) + + # so it's equivalent to this + user = build(:user) + build_pair(:account, user: user) # same user for both accounts + + # to get a separate user struct per account, use build_lazy/2 + build_pair(:account, user: build_lazy(:user)) + """ + @callback build_lazy(factory_name :: atom) :: any + @callback build_lazy(factory_name :: atom, attrs :: keyword | map) :: any + + @doc false + def build_lazy(module, factory_name, attrs \\ %{}) do + %ExMachina.InstanceTemplate{module: module, name: factory_name, attrs: attrs} + end + @doc """ Helper function to merge attributes into a factory that could be either a map or a struct. diff --git a/lib/ex_machina/instance_template.ex b/lib/ex_machina/instance_template.ex new file mode 100644 index 0000000..9ab2f66 --- /dev/null +++ b/lib/ex_machina/instance_template.ex @@ -0,0 +1,10 @@ +defmodule ExMachina.InstanceTemplate do + @moduledoc false + + defstruct [:module, :name, :attrs] + + @doc false + def evaluate(%{module: module, name: name, attrs: attrs}) do + apply(module, :build, [name, attrs]) + end +end diff --git a/priv/test_repo/migrations/1_migrate_all.exs b/priv/test_repo/migrations/1_migrate_all.exs index a9c6f88..c6455f6 100644 --- a/priv/test_repo/migrations/1_migrate_all.exs +++ b/priv/test_repo/migrations/1_migrate_all.exs @@ -3,23 +3,29 @@ defmodule ExMachina.TestRepo.Migrations.MigrateAll do def change do create table(:users) do - add :name, :string - add :admin, :boolean - add :net_worth, :decimal + add(:name, :string) + add(:admin, :boolean) + add(:net_worth, :decimal) end + create table(:publishers) do + add(:pub_number, :string) + end + + create(unique_index(:publishers, [:pub_number])) + create table(:articles) do - add :title, :string - add :author_id, :integer - add :editor_id, :integer - add :publisher_id, :integer - add :visits, :decimal + add(:title, :string) + add(:author_id, :integer) + add(:editor_id, :integer) + add(:publisher_id, :integer) + add(:visits, :decimal) end create table(:comments) do - add :article_id, :integer - add :author, :map - add :links, {:array, :map}, default: [] + add(:article_id, :integer) + add(:author, :map) + add(:links, {:array, :map}, default: []) end end end diff --git a/test/ex_machina/ecto_test.exs b/test/ex_machina/ecto_test.exs index 354fb6a..d56d114 100644 --- a/test/ex_machina/ecto_test.exs +++ b/test/ex_machina/ecto_test.exs @@ -1,6 +1,8 @@ defmodule ExMachina.EctoTest do use ExMachina.EctoCase + alias ExMachina.Article + alias ExMachina.Publisher alias ExMachina.TestFactory alias ExMachina.User @@ -65,6 +67,23 @@ defmodule ExMachina.EctoTest do test "insert_list/3 handles the number 0" do assert [] = TestFactory.insert_list(0, :user) end + + test "build_lazy records get evaluated with insert/2 and insert_* functions" do + assert %Article{publisher: %Publisher{}} = + TestFactory.insert(:article, publisher: TestFactory.build_lazy(:publisher)) + + [%Article{publisher: publisher1}, %Article{publisher: publisher2}] = + TestFactory.insert_pair(:article, publisher: TestFactory.build_lazy(:publisher)) + + assert publisher1 != publisher2 + + [publisher1, publisher2, publisher3] = + TestFactory.insert_list(3, :article, publisher: TestFactory.build_lazy(:publisher)) + + assert publisher1.author != publisher2.author + assert publisher2.author != publisher3.author + assert publisher3.author != publisher1.author + end end describe "params_for/2" do diff --git a/test/ex_machina_test.exs b/test/ex_machina_test.exs index 8bd87bb..a695f7b 100644 --- a/test/ex_machina_test.exs +++ b/test/ex_machina_test.exs @@ -16,6 +16,20 @@ defmodule ExMachinaTest do } end + def profile_factory do + %{ + username: sequence("username"), + user: build(:user) + } + end + + def account_factory do + %{ + private: true, + profile: build_lazy(:profile) + } + end + def email_factory do %{ email: sequence(:email, &"me-#{&1}@foo.com") @@ -101,11 +115,10 @@ defmodule ExMachinaTest do end test "build/2 allows factories to have full control of provided arguments" do - assert Factory.build(:comment, name: "James") == %{ - author: "James Doe", - username: "James-0", - name: "James" - } + comment = Factory.build(:comment, name: "James") + + assert %{author: "James Doe", name: "James"} = comment + assert String.starts_with?(comment[:username], "James-") end test "build/2 allows custom (non-map) factories to be built" do @@ -114,6 +127,70 @@ defmodule ExMachinaTest do end end + describe "build_lazy/2" do + test "build_lazy/2 returns a struct presentation of the factory to build" do + %ExMachina.InstanceTemplate{} = factory = Factory.build_lazy(:user) + + assert ExMachina.InstanceTemplate.evaluate(factory) == %{ + id: 3, + name: "John Doe", + admin: false + } + end + + test "build_lazy/3 accepts arguments" do + %ExMachina.InstanceTemplate{} = factory = Factory.build_lazy(:user, name: "Jane Doe") + + assert ExMachina.InstanceTemplate.evaluate(factory) == %{ + id: 3, + name: "Jane Doe", + admin: false + } + end + + test "build_lazy/2 can be used in a factory definition" do + account = Factory.build(:account) + + assert %{username: _} = account.profile + end + + test "build_lazy/2 can be used with struct factories" do + user = Factory.build(:user, foo_bar: Factory.build_lazy(:foo_bar)) + + assert %FooBar{} = user.foo_bar + end + + test "build_lazy/2 is evaluated before being passed to factories with full control" do + comment = Factory.build(:comment, name: "James", user: Factory.build_lazy(:user)) + + assert %{id: 3, name: "John Doe", admin: false} = comment.user + end + + test "build/2 recursively builds nested build_lazy/2 factories" do + lazy_profile = Factory.build_lazy(:profile, user: Factory.build_lazy(:user)) + account = Factory.build(:account, profile: lazy_profile) + + assert %{username: _} = account.profile + assert %{name: "John Doe", admin: false} = account.profile.user + end + + test "build_list/2 recursively builds many nested build_lazy/2 factories" do + lazy_profile = Factory.build_lazy(:profile, user: Factory.build_lazy(:user)) + [account1, account2] = Factory.build_pair(:account, profile: lazy_profile) + + assert account1.profile.username != account2.profile.username + end + + test "build_lazy/2 gets evaluated when is part of a list" do + user = Factory.build(:user, profiles: [Factory.build_lazy(:profile)]) + + profile = hd(user.profiles) + + assert Map.has_key?(profile, :username) + assert Map.has_key?(profile, :user) + end + end + describe "build_pair/2" do test "build_pair/2 builds 2 factories" do records = Factory.build_pair(:user, admin: true) diff --git a/test/support/models/publisher.ex b/test/support/models/publisher.ex index 4cbdc4e..689de7b 100644 --- a/test/support/models/publisher.ex +++ b/test/support/models/publisher.ex @@ -1,6 +1,7 @@ defmodule ExMachina.Publisher do use Ecto.Schema - schema "users" do + schema "publishers" do + field(:pub_number, :string) end end diff --git a/test/support/test_factory.ex b/test/support/test_factory.ex index a967c37..303ecbc 100644 --- a/test/support/test_factory.ex +++ b/test/support/test_factory.ex @@ -18,7 +18,9 @@ defmodule ExMachina.TestFactory do end def publisher_factory do - %ExMachina.Publisher{} + %ExMachina.Publisher{ + pub_number: sequence("PUB_23") + } end def article_factory do