How to test all functionality without exposing private methods?

Phoenix Forum Questions / Help
,
1

I’m trying to find a way to fix a design issue where I have some module methods public for the sole purpose of running tests on them.

I have a module that creates a user along with a confirmation token (which is stored in users table field with uniqueness constraint on it) using these steps:

  • generate a token with the help of EmailConfirmationHandler
  • merge it with passed user attributes
  • persist the user

Because the token uniqueness validation happens on a user persisting step, there can be a case when the saving fails and the whole process needs to be repeated with a regenerated token.

My main module has only 1 method call that I want to expose, but in order to test all scenarios properly I need to also test private methods(as I need to check how module performs with different results from EmailConfirmationHandler), hence I change their accessibility level to public.

So the question is, is there a better way to design this functionality so I don’t have any private methods exposed for the sole purpose of testing?

Any feedback is highly appreciated. Thanks!

Main module

defmodule App.UserCreator do
  @moduledoc """
  Used for creating user with specified params
  """

  alias App.{Accounts.EmailConfirmationHandler, Accounts.User, MapUtils, Repo}

  def call(attrs) do
    attrs
    |> merge_confirmation_attrs
    |> create_user_with
    |> process_user_creation_result(attrs)
  end

  def merge_confirmation_attrs(attrs) do
    Map.merge(MapUtils.atomize_keys(attrs),
              EmailConfirmationHandler.generate_confirmation_params())
  end

  def create_user_with(attrs) do
    %User{}
    |> User.create_changeset(attrs)
    |> Repo.insert()
  end

  def process_user_creation_result(result, attrs) do
    case result do
     {:ok, user} -> {:ok, user}
     {:error, changeset} -> if confirmation_token_error_present?(changeset),
                               do: call(attrs),
                               else: {:error, changeset}
    end
  end

  defp confirmation_token_error_present?(%Ecto.Changeset{errors: errors}) do
    List.keymember?(errors, :confirmation_token, 0)
  end
end

Tests

defmodule UserCreatorTest do
  use App.DataCase

  alias App.Accounts
  alias App.UserCreator

  @user_attrs %{email: "fred@example.com",
                password: "reallyHard2gue$$"}

  def fixture(:user, attrs \\ @user_attrs) do
    {:ok, user} = Accounts.create_user(attrs)
    user
  end

  test "create_user/1 with valid data creates a user" do
    {:ok, user} = UserCreator.call(@user_attrs)

    assert user.email == "fred@example.com"
    assert user.confirmation_token
    assert user.confirmation_sent_at
  end

  test "merge_confirmation_attrs/1 merges user attrs with generated confirmation attrs" do
    result = UserCreator.merge_confirmation_attrs(%{email: "test@volocare.com"})

    assert result.email == "test@volocare.com"
    assert result.confirmation_sent_at
    assert String.length(result.confirmation_token) == 8
  end

  test "create_user_with/1 creates user with passed attrs" do
    attrs = %{email: "test@volocare.com",
              password: "test1234",
              confirmation_token: "just a random string"}

    {:ok, user} = UserCreator.create_user_with(attrs)

    assert user.email == "test@volocare.com"
    assert user.confirmation_token == "just a random string"
  end

  test "process_user_creation_result/1 returns passed result whe it is successful" do
    user = fixture(:user)

    assert UserCreator.process_user_creation_result({:ok, user}, %{}) == {:ok, user}
  end

  test "process_user_creation_result/1 recreates user with new confirmation attrs when confirmation_token in invalid" do
    user1 = fixture(:user)
    user2_attrs = %{email: "test@volocare.com", password: "test1234"}
    failed_creation = Map.merge(user2_attrs, %{confirmation_token: user1.confirmation_token})
                      |> UserCreator.create_user_with()

    {:ok, result} = UserCreator.process_user_creation_result(failed_creation, user2_attrs)

    assert result.email == "test@volocare.com"
    refute result.confirmation_token == user1.confirmation_token
  end

  test "process_user_creation_result/1 returns passed result whe it is unsuccessful not because of confirmation_token" do
    fixture(:user)
    failed_creation = UserCreator.create_user_with(@user_attrs)

    {:error, result} = UserCreator.process_user_creation_result(failed_creation, %{})

    assert List.keymember?(result.errors, :email, 0)
  end
end
2

:wave:

I don’t see you using App.UserCreator functions in your test at all. Based on what you do use in your tests, you can change create_user_with and process_user_creation_result accessibility levels to private.

Off-topic …

I generally try not to mess with attrs and treat it as an opaque data structure, since in my apps they usually come from the user and I can’t trust it, so I do something like this:

  def call(attrs) do
    %User{confirmation_token: EmailConfirmationHandler.generate_confirmation_token()}
    |> create_user_with(attrs)
    |> process_user_creation_result(attrs)
  end

  defp create_user_with(user, attrs) do
    user
    |> User.create_changeset(attrs)
    |> Repo.insert()
  end

It removes the need for merge_confirmation_attrs and makes the call function more resilient to a (sadly) quite “popular” phoenix security bag – casting foreign keys or any other private data.

    user = fixture(:user)
    unix_confirmation_sent_at = DateTime.to_unix(DateTime.utc_now) - 60 * 60 * 24 - 1
    {:ok, date} = DateTime.from_unix(unix_confirmation_sent_at)
    Accounts.update_user(user, %{"confirmation_sent_at" => date})

can probably be replaced with a bit more flexible fixture and a helper function

  # I usually have a separate file for all fixtures like this
  def fixture(:user, attrs \\ []) do
    Repo.insert!(%Accounts.User{
      email: attrs[:email] || "fred@example.com",
      password: attrs[:password] || "reallyHard2gue$$",
      confirmation_sent_at: attrs[:confirmation_sent_at] || DateTime.utc_now()
    })
  end

  # can add in App.DataCase
  defp into_past(dt, period) do
    DateTime.from_unix!(DateTime.to_unix(dt) - period)
  end

  test "verify_confirmation_params/1 when confirmation token is outdated, returns an error" do
    user = fixture(:user, confirmation_sent_at: into_past(DateTime.utc_now(), 60 * 60 * 24 + 1))
    result = EmailConfirmationHandler.verify_confirmation_params(%{"key" => user.confirmation_token})
    assert result == {:error, "Email confirmation token is not valid or expired"}
  end
3

Thanks for an answer. My fault with a test file, as I pasted a wrong one by mistake.
Now valid one is here