Phoenix: Testing guides overhaul

[buhman]

After following the testing guides, it seems like they could be improved

• [ ] each guide uses its own (sometimes implicitly, due to conflicts with previously generated code) phx.new, but maybe this isn't necessary.
• [ ] testing.md
  
  
  
  • [ ] "Generating More Files" looks like it's supposed to be used in testing_schemas or something, but uses a different context name
  
  
• [ ] testing_schemas.md
  
  
  
  • [ ] "Generating an HTML Resource" mentions manually doing ecto tasks, which are handled automatically
  
  
• [ ] testing_controllers.md
  
  
  
  • [ ] "Show when the user is not found" doesn't mention or use action_fallback
  
  
  • [ ] is it actually useful to say "there's this generator that you could use that does this for you, but don't use that"? maybe this could be rewritten to build functionality on top of generated code, like previous guides?
  
  
  • [ ] it seems slightly forced to start by testing a list response two items, then deleting most of the code with more efficient code later; maybe this could be restructured to introduce test setup macros first, then followed by actual tests?
  
  
  • maybe the test macro stuff could be put in the introduction.
  
  

2493 makes it sound like there might have been some other plan that builds off of context

[oldpond]

I think this is a typo in the 1.3.0 guide:
It fails - which is exactly what it should do! We haven’t written the code to make it pass yet. To do that, we need to remove the :number_of_pets attribute from our validate_required/3 function in lib/hello_web/models/user.ex.
No models anymore.

[ebengt]

Is there a reason for the generated test cases ('mix phx.gen.json' in my case) hard codes the test asserts when testing Schema? I apologise if this is not the right place to mention this.

Ex:
defmodule Becomics.ComicsTest do
use Becomics.DataCase

alias Becomics.Comics

describe "comics" do
alias Becomics.Comics.Comic

@valid_attrs %{name: "some name", url: "some url"}

...
test "create_comic/1 with valid data creates a comic" do
assert {:ok, %Comic{} = comic} = Comics.create_comic(@valid_attrs)
assert comic.name == "some name"
assert comic.url == "some url"
end

When I change the valid attributes

@valid_attrs %{name: "some name", url: "http://some url"}

I then have to change the test case too.

That would not be needed if the test case is

test "create_comic/1 with valid data creates a comic" do
  assert {:ok, %Comic{} = comic} = Comics.create_comic(@valid_attrs)
  assert comic.name == @valid_attrs.name
  assert comic.url == @valid_attrs.url
end

[josevalim]

I prefer to have the values explicitly written, decoupling the input data from the output one.

[chrismccord]

@Gazler @jeregrine as we prep for the 1.4 release, are we planning on talking this issue in whole, or in part as part of 1.4? If you think we need split this out into issues with a 1.4 milestone, please let me know. Thanks!

[Gazler]

@chrismccord I'll have some time to check this out over the weekend. Will tackle what I can then.

[Gazler]

each guide uses its own (sometimes implicitly, due to conflicts with previously generated code) phx.new, but maybe this isn't necessary.

This is true - maybe we should just reference the relevant generators in other guides and always assume the user is using that guide to generate the project or resource under test?

testing.md
"Generating More Files" looks like it's supposed to be used in testing_schemas or something, but uses a different context name

I think it is fine here, the purpose of the section is to show that the generators also generate tests. If we go with the idea above then we could reference the generator in its own guide and simply mention that the tests are also created.

testing_schemas.md
"Generating an HTML Resource" mentions manually doing ecto tasks, which are handled automatically

Is this in reference to the migrate? I think it is good practice to mention mix ecto.migrate explicitly in the guides because although it is not required for the tests, it is for the development environment.

"Show when the user is not found" doesn't mention or use action_fallback

:+1: I've created a PR for this at https://github.com/phoenixframework/phoenix/pull/2769

is it actually useful to say "there's this generator that you could use that does this for you, but don't use that"? maybe this could be rewritten to build functionality on top of generated code, like previous guides?

Again, I think we should mention that there is a generator. Instead of writing the command out, reference the generators section, and then continue with the guide as is.

it seems slightly forced to start by testing a list response two items, then deleting most of the code with more efficient code later; maybe this could be restructured to introduce test setup macros first, then followed by actual tests?

Yes, I think if we put setup in the introduction then we can reference it here and use it straight away.

maybe the test macro stuff could be put in the introduction.

Agreed, the introduction is definitely the place to introduce setup and maybe describe too.

---

@buhman @chrismccord Thoughts on the above?

[buhman]

and then continue with the guide as is.

I think either way, the overall style/structure of the tests from the guide should closely match the generator output--they didn't when I wrote that.

reference the relevant generators in other guides

Sounds good to me.