Create tests that are adaptable

[chargio]

When you create a new user model and use the standard tests, you get 97 errors, as many of the test only set up email and password, and then fail in any other validation of the model. Modifying the fixture does not help, because tests are hardcoded to only use email and password.

I have been able to fix many of the uses modifying the tests, to create a full user using user_fixture(), and then modifying those things that need to fail for the test, instead of inserting email and passwor, getting rid of almost all of them, with the exception of get_connection(conn, :user_token), that fails if you use the Struct from user_fixture() and convert it to a map instead of generating the map directly.

It would be nice if you can follow that strategy in the generator, so you:

• Generate the application
• Use phx.gen.auth to add the User model
• Modify it with the new fields (i.e. name, surname, phone)
• Change changesets to require the new fields and validate them
• All tests works... you write tests only for the new fields

[josevalim]

@chargio can you please send a diff showing how you changed one or two tests, so your proposal is clearer to us? Thank you.

[chargio]

when applicable:

>     setup do
>       %{user: user_fixture()}
>     end
>

And then

<       {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password()})
---
>       password = valid_user_password()
>       mappings = Map.from_struct(%{ user_fixture() |email: email, password: password})

The result is the same if only email and password are part of the struct, but it will be valid if the struct is more complex

[chargio]

I would make a PR but I am learning Elixir and Phoenix and it would take me long.

Of course, you need to manually update some tests, but that is ok because they are testing your User struct

<       assert changeset.required == [:password, :email]
---
>       assert changeset.required == [:password, :email, :name, :surname, :telephone]
105c123
<

[chargio]

And lastly

 def user_fixture(attrs \\ %{}) do
    {:ok, user} =
      attrs
      |> Enum.into(%{
        email: unique_user_email(),
        password: valid_user_password(),
        name: valid_user_name(),
        surname: valid_user_surname(),
        telephone: valid_user_phone(),
      })
      |> ReservationBook.Accounts.register_user()

    user
  end

Where the different calls are functions that will generate a valid field (so I can use Fake or equivalent to make better tests)

[josevalim]

I see. Maybe a better option here is to generate a valid_user_attributes function as part of the fixtures and use it when testing the register_user functions. WDYT @aaronrenner ?

[chargio]

I agree, I've viewed the register_user and understood why the register was complaining about duplicated email

[chargio]

Created a working version of that like this:

  def valid_user_attributes(attrs \\ %{}) do
    attrs
    |> Enum.into(%{
      email: unique_user_email(),
      password: valid_user_password(),
      name: valid_user_name(),
      surname: valid_user_surname(),
      telephone: valid_user_phone(),
      comments: nil,
    })
  end

  def user_fixture(attrs \\ %{}) do
    {:ok, user} =
      valid_user_attributes(attrs)
      |> ReservationBook.Accounts.register_user()
    user
  end

And the only test that was failing now becomes:

describe "POST /users/register" do
    @tag :capture_log
    test "creates account and logs the user in", %{conn: conn} do
      %{email: email} = user_attrs = valid_user_attributes()

      conn =
        post(conn, Routes.user_registration_path(conn, :create), %{
          "user" => user_attrs}
        )

      assert get_session(conn, :user_token)
      assert redirected_to(conn) =~ "/"

      # Now do a logged in request and assert on the menu
      conn = get(conn, "/")
      response = html_response(conn, 200)
      assert response =~ email
      assert response =~ "Settings</a>"
      assert response =~ "Log out</a>"
    end

[aaronrenner]

@chargio Thanks for reporting this. Sorry it's taken a while for me to get to this, but I wanted to let you know I'm starting to look into it.

[aaronrenner]

@chargio I just added a new field in a test app and here are the changes I needed to make.

diff --git a/lib/my_app/accounts/user.ex b/lib/my_app/accounts/user.ex
index 4c78845..5ee2e5f 100644
--- a/lib/my_app/accounts/user.ex
+++ b/lib/my_app/accounts/user.ex
@@ -3,6 +3,7 @@ defmodule MyApp.Accounts.User do
   import Ecto.Changeset

   schema "users" do
+    field :name, :string
     field :email, :string
     field :password, :string, virtual: true, redact: true
     field :hashed_password, :string, redact: true
@@ -30,9 +31,10 @@ defmodule MyApp.Accounts.User do
   """
   def registration_changeset(user, attrs, opts \\ []) do
     user
-    |> cast(attrs, [:email, :password])
+    |> cast(attrs, [:email, :password, :name])
     |> validate_email()
     |> validate_password(opts)
+    |> validate_name()
   end

   defp validate_email(changeset) do
@@ -54,6 +56,10 @@ defmodule MyApp.Accounts.User do
     |> maybe_hash_password(opts)
   end

+  defp validate_name(changeset) do
+    validate_required(changeset, [:name])
+  end
+
   defp maybe_hash_password(changeset, opts) do
     hash_password? = Keyword.get(opts, :hash_password, true)
     password = get_change(changeset, :password)
diff --git a/lib/my_app_web/templates/user_registration/new.html.eex b/lib/my_app_web/templates/user_registration/new.html.eex
index 18f36d7..4eafd37 100644
--- a/lib/my_app_web/templates/user_registration/new.html.eex
+++ b/lib/my_app_web/templates/user_registration/new.html.eex
@@ -7,6 +7,10 @@
     </div>
   <% end %>

+  <%= label f, :name %>
+  <%= text_input f, :name, required: true %>
+  <%= error_tag f, :name %>
+
   <%= label f, :email %>
   <%= email_input f, :email, required: true %>
   <%= error_tag f, :email %>
diff --git a/priv/repo/migrations/20210119051126_create_users_auth_tables.exs b/priv/repo/migrations/20210119051126_create_users_auth_tables.exs
index f633b07..d50619d 100644
--- a/priv/repo/migrations/20210119051126_create_users_auth_tables.exs
+++ b/priv/repo/migrations/20210119051126_create_users_auth_tables.exs
@@ -5,6 +5,7 @@ defmodule MyApp.Repo.Migrations.CreateUsersAuthTables do
     execute "CREATE EXTENSION IF NOT EXISTS citext", ""

     create table(:users) do
+      add :name, :string, null: false
       add :email, :citext, null: false
       add :hashed_password, :string, null: false
       add :confirmed_at, :naive_datetime
diff --git a/test/my_app/accounts_test.exs b/test/my_app/accounts_test.exs
index 2ba183c..2b6ce95 100644
--- a/test/my_app/accounts_test.exs
+++ b/test/my_app/accounts_test.exs
@@ -86,8 +86,9 @@ defmodule MyApp.AccountsTest do

     test "registers users with a hashed password" do
       email = unique_user_email()
-      {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password()})
+      {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password(), name: "Some name"})
       assert user.email == email
+      assert user.name == "Some name"
       assert is_binary(user.hashed_password)
       assert is_nil(user.confirmed_at)
       assert is_nil(user.password)
@@ -97,7 +98,7 @@ defmodule MyApp.AccountsTest do
   describe "change_user_registration/2" do
     test "returns a changeset" do
       assert %Ecto.Changeset{} = changeset = Accounts.change_user_registration(%User{})
-      assert changeset.required == [:password, :email]
+      assert changeset.required == [:name, :password, :email]
     end

     test "allows fields to be set" do
@@ -105,12 +106,13 @@ defmodule MyApp.AccountsTest do
       password = valid_user_password()

       changeset =
-        Accounts.change_user_registration(%User{}, %{"email" => email, "password" => password})
+        Accounts.change_user_registration(%User{}, %{"email" => email, "password" => password, "name" => "Some name"})

       assert changeset.valid?
       assert get_change(changeset, :email) == email
       assert get_change(changeset, :password) == password
       assert is_nil(get_change(changeset, :hashed_password))
+      assert get_change(changeset, :name) == "Some name"
     end
   end

diff --git a/test/my_app_web/controllers/user_registration_controller_test.exs b/test/my_app_web/controllers/user_registration_controller_test.exs
index b5a9dff..5501e2d 100644
--- a/test/my_app_web/controllers/user_registration_controller_test.exs
+++ b/test/my_app_web/controllers/user_registration_controller_test.exs
@@ -25,7 +25,7 @@ defmodule MyAppWeb.UserRegistrationControllerTest do

       conn =
         post(conn, Routes.user_registration_path(conn, :create), %{
-          "user" => %{"email" => email, "password" => valid_user_password()}
+          "user" => %{"email" => email, "password" => valid_user_password(), "name" => "Some name"}
         })

       assert get_session(conn, :user_token)
diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex
index c2f4d34..7477fde 100644
--- a/test/support/fixtures/accounts_fixtures.ex
+++ b/test/support/fixtures/accounts_fixtures.ex
@@ -11,6 +11,7 @@ defmodule MyApp.AccountsFixtures do
     {:ok, user} =
       attrs
       |> Enum.into(%{
+        name: "some name",
         email: unique_user_email(),
         password: valid_user_password()
       })

Since every test except for the UserRegistrationControllerTest asserts all values are set as expected, I'm not really sure what we'd be gaining by adding a valid_user_attributes/1 function.

I'll continue thinking about what approach we should take when adding fields to the user model. Please let me know if you have any suggestions. :)

[chargio]

I agree with you that your way of doing things work but:

• As a new user, I get 90 errors if I add a name to a user because every test that supposes that the only valid fields are user and password will fail. It is not only to create the fixture, but on user registration you are hardcoding the fields needed, user and password, while my model has 7.
• I needed to perform a code review to understand why and modify a good portion of the list of tests. It took make time to do so, and then I needed to add my own tests for name, surname, telephone, etc. It is not a daunting task, but still I believe is easier to extend the tests when you start from a fully valid user attibutes list.

With the valid_user_attributes, I have a way to control what is going on. You can create the user directly, but it makes it easier to understand what is going on: I have valid attributes, I modify one of them for the test (nil, valid, or invalid), I try to create or update the user, see the results and test what happens with the original values. Actually, it allows me to use fixture generators (my phone numbers or names are different every time I create a user, so there is a chance I catch more errors). And all of that without needing to change my tests when I add a new field (all my tests will still be valid, even if I don't assert the new fields).

[aaronrenner]

@josevalim Here's the proposed changes to the generated code for this one. If this looks good, I'll update the generators.

Let me know what you think!

diff --git a/test/default_app/accounts_test.exs b/test/default_app/accounts_test.exs
index a572b3f..599f6df 100644
--- a/test/default_app/accounts_test.exs
+++ b/test/default_app/accounts_test.exs
@@ -86,7 +86,7 @@ defmodule DefaultApp.AccountsTest do

     test "registers users with a hashed password" do
       email = unique_user_email()
-      {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password()})
+      {:ok, user} = Accounts.register_user(valid_user_attributes(email: email))
       assert user.email == email
       assert is_binary(user.hashed_password)
       assert is_nil(user.confirmed_at)
@@ -105,7 +105,10 @@ defmodule DefaultApp.AccountsTest do
       password = valid_user_password()

       changeset =
-        Accounts.change_user_registration(%User{}, %{"email" => email, "password" => password})
+        Accounts.change_user_registration(
+          %User{},
+          valid_user_attributes(email: email, password: password)
+        )

       assert changeset.valid?
       assert get_change(changeset, :email) == email
diff --git a/test/default_app_web/controllers/user_registration_controller_test.exs b/test/default_app_web/controllers/user_registration_controller_test.exs
index 6764d9d..d8a850d 100644
--- a/test/default_app_web/controllers/user_registration_controller_test.exs
+++ b/test/default_app_web/controllers/user_registration_controller_test.exs
@@ -25,7 +25,7 @@ defmodule DefaultAppWeb.UserRegistrationControllerTest do

       conn =
         post(conn, Routes.user_registration_path(conn, :create), %{
-          "user" => %{"email" => email, "password" => valid_user_password()}
+          "user" => valid_user_attributes(email: email)
         })

       assert get_session(conn, :user_token)
diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex
index b909a10..282de93 100644
--- a/test/support/fixtures/accounts_fixtures.ex
+++ b/test/support/fixtures/accounts_fixtures.ex
@@ -7,13 +7,18 @@ defmodule DefaultApp.AccountsFixtures do
   def unique_user_email, do: "user#{System.unique_integer()}@example.com"
   def valid_user_password, do: "hello world!"

+  def valid_user_attributes(attrs \\ %{}) do
+    Enum.into(attrs, %{
+      email: unique_user_email(),
+      password: valid_user_password(),
+      name: "Default name"
+    })
+  end
+
   def user_fixture(attrs \\ %{}) do
     {:ok, user} =
       attrs
-      |> Enum.into(%{
-        email: unique_user_email(),
-        password: valid_user_password()
-      })
+      |> valid_user_attributes()
       |> DefaultApp.Accounts.register_user()

     user

[josevalim]

Looks great honestly!