Do we need to test changeset in Phoenix 1.3?

Phoenix Forum Questions / Help
1

I wrote tests a lot for changeset in Phoenix 1.2, but I’m confused with 1.3 now.

The generated context file defined the changeset as:

  defp user_changeset(%User{} = user, attrs) do
    user
    |> cast(attrs, [:username, :password, :nickname])
    |> validate_required([:username, :password])
    |> unique_constraint(:username)
  end

It’s private, and I can remember Jose’s answer here Is there a way to test private functions in modules in ExUnit of Elixir? - Stack Overflow

No, there is no way to test them via ExUnit.

Also there’s no more any tests for changeset generated automatically in test files.

So do we need to test them now in Phoenix 1.3?

2 Likes
2

hi @chensan,

i think for the 1.3 i believe that writing context tests is more preferred than tests direct on changesets test, so not to duplicates our tests. cause contexts are like wrappers to all the manipulating the changeset functions. but there is nothing wrong testing changeset related properties also.

since 1.3 came out not so while back. i think people still have different views on this. it’s depend on how much smaller the unit people are looking at.

dev

1 Like
3

I wonder how you gonna use private function from schema in the context. For me it has to be public to reveal itself to context or it has be wrapped out in the schema - again - to be revealed eventually to the context. Either way, you have to test changeset.
Fact that schemas are only for contexts to manage is sort of contract. In Elixir you can write very dirty code if you would do, so this is the assumption - you will follow guidelines from guys, who deliver these great products.
You could still interact with schemas in the controllers, but you just don’t want to do that :slight_smile:

1 Like
4

The changeset generated are defined in context file now, no longer in schema.

5

So even better- test only public functions. Always :wink:

6

Say I add a new validate_format to the changeset:

  defp user_changeset(%User{} = user, attrs) do
    user
    |> cast(attrs, [:username, :password, :nickname])
    |> validate_required([:username, :password])
    |> validate_format(:username, ~r/[a-zA-Z0-9]+/, message: gettext("Username should only contain character or number"))
    |> unique_constraint(:username)
  end

If I do not test the changeset, then I’m not confident in my code. Also that changeset might grow very large, it would be error prone.

7

Yes, but this function will be used in any public function, so when testing behaviour of public function, this one should be covered.

8

@blisscs Thank both of you for sharing your idea.

This is what I got:

  test "username should not contain ." do
    assert {:error, changeset} = Accounts.create_user(%{username: "+??", password_hash: "fdslfjdsla"})
    assert "Username should only contain character or number" in errors_on(changeset).username
  end

It would avoid testing changeset directly.

1 Like
9

Hi @chensan,

In your test, I see you are returning the changeset along with the error here.
I think it’s better to hide the changeset and it properties from outside world. I think better returning a error map to the caller of this function, something like this: {:error, %{username: “is invalid”}}.

I think the main reason of having a context is to hide about internal schema and changeset from the rest of application.

Dev.

10

:grin: Yes, I agree with you, but I don’t know how to make it in current situation. Would you love to give it a try here https://github.com/phoenixframework/phoenix/blob/master/installer/templates/phx_ecto/data_case.ex#L41? Maybe you could improve it.

11

Hi @chensan,

After looking at the code there. I think my understanding was a little wrong. We still need to return a changeset there, since in general case (Html case, not api case) we still need changeset to work with the html generated form in the template and in the controller too. But if for the api app one I would return it without the changeset to hide the changeset part in the context.

I just started playing with 1.3 also. Let’s see if any idea come out.

Dev.

12

I’d always use changesets in controllers regardless if it’s for HTML or API. Part of it is uniformity, part of it is that changeset errors contain additional metadata that might be useful when displaying them.

For example, at hex.pm we have a field :links, {:map, :string} and if something incorrect will be passed in (e.g. map with integer values), we’ll have following entry in the changeset.errors: links: {"is invalid", [type: {:map, :string}, validation: :cast]}. Then, the controller/view can display it in a slightly more user friendly way [1]: expected type map(string)

[1] hexpm/test/hexpm/web/controllers/api/release_controller_test.exs at 0ce8169739d9baf061d6be5ba9e4940a7d0515b6 · hexpm/hexpm · GitHub

13

hello @wojtekmach,

Thank for recommending for uniformity we should return changeset here.
I saw the example from hex.pm and see their controllers. Now I think we should use the changeset instead of what converting to the plane map.

Dev.

1 Like