Phoenix: Proposal: Updates to `phx.gen.json` generated tests

[jeregrine]

I'd like to propose some small quality of life changes to the phx.gen.json generated tests.

When running the following command mix phx.gen.json Pets Dog dogs name:string type:string size:string goodness:boolean we get the following assertion for creating and a similar one for updating.

elixir assert json_response(conn, 200)["data"] == %{ "id" => id, "goodness" => true, "name" => "some name", "size" => "some size", "type" => "some type" }

I'd like to propose changing this to
elixir assert data = json_response(conn, 200)["data"] assert data["id"] == id assert data["goodness"] == true assert data["name"] == "some name" assert data["size"] == "some size" assert data["type"] == "some type"

Pros:

• When you make an addition to the view the test suite doesn't fail immediately.
• You only need to add a single line instead of manipulating a map.

Cons:

• The test doesn't fail whenever a change happens to the views.

I know this a pretty edgy request so I hope it doesn't offend @Gazler.

[chrismccord]

what about:

assert %{
  "id" => id,
  "goodness" => true,
  "name" => "some name",
  "size" => "some size",
  "type" => "some type",
} = json_response(conn, 200)["data"]

This is less noisy and more natural to match on the structure of the response. I would prefer this approach to the asserts per key. Thoughts?

[josevalim]

Great discussion!

I think we should ask ourselves what is the real purpose of this test and if the gap we introduce here should be filled or will be filled elsewhere. For example:

1. Should we assert at some point on all of the fields returned when rendering JSON?
2. If so, should we assert this in the view test (unit test) or the controller (integration test)?

Strictly speaking, the advantage of the integration test is that we see the value as it is rendered by JSON, so it can help us avoid surprises.

@chrismccord unfortunately the issue with pattern matching is that the error message is way less inferior to the == operator.

[chrismccord]

The error messages are generally good enough – the assert %{..} = call is the style I take in my own code the vast majority of the time vs ==

[jeregrine]

I don't mind @chrismccord's suggestion.

@josevalim to your points I agree, it feels like a good thing the test fail when you introduce changes to a view. ATM we don't generate view tests at all.

[ggcampinho]

One advantage of pattern matching is that you can assign a variable and
test that it isn't nil afterwards, this is useful for generated things like
an id and timestamps. I believe this should be broken into two assertions
at least. One for the data and other for the attributes.

assert %{
"data" => data
} = json_response(conn, 200)

assert %{
"id" => id,
"goodness" => true,
"name" => "some name",
"size" => "some size",
"type" => "some type",
"updated_at" => %DateTime{}
} = data

Or it should assert a nested map. Otherwise, if the key data changes, the
error is not clear. In this case, we don't care if new keys are added, but
I think is fine because a new field in a JSON is not a breaking change.

On Thu, Jun 14, 2018, 05:35 José Valim [e-mail] wrote:

Great discussion!

I think we should ask ourselves what is the real purpose of this test and
if the gap we introduce here should be filled or will be filled elsewhere.
For example:

1. Should we assert at some point on all of the fields returned when
   rendering JSON?
2. If so, should we assert this in the view test (unit test) or the
   controller (integration test)?

Strictly speaking, the advantage of the integration test is that we see
the value as it is rendered by JSON, so it can help us avoid surprises.

@chrismccord https://github.com/chrismccord unfortunately the issue
with pattern matching is that the error message is way less inferior to the
== operator.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/phoenixframework/phoenix/issues/2911#issuecomment-397216662,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACt1pmQ0iLq9p6W2hqVvYrvCAOkk7nJ8ks5t8iBjgaJpZM4UnFx9
.

[soteras]

I usually use pattern matching. I liked the @chrismccord idea.