New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Project Controller Tests #438

Merged
merged 1 commit into from Nov 11, 2016

Conversation

Projects
None yet
6 participants
@amyschools
Contributor

amyschools commented Nov 4, 2016

What's in this PR?

Add api_case.ex helpers to Project controller tests.

References

Closes #437

Progress on: #413

@amyschools amyschools self-assigned this Nov 4, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 4, 2016

Coverage Status

Coverage decreased (-0.2%) to 93.689% when pulling 3972bb5 on 437-project-controller-test into 604f6df on develop.

@coveralls

This comment has been minimized.

coveralls commented Nov 4, 2016

Coverage Status

Coverage decreased (-0.2%) to 93.689% when pulling 486c596 on 437-project-controller-test into 604f6df on develop.

@sbatson5

Just 1 suggestion. This is looking great 👍

assert data["attributes"]["title"] == "Test project"
assert data["attributes"]["description"] == "Test project description"
assert data["attributes"]["long-description-markdown"] == "A markdown **description**"
assert data["relationships"]["organization"]["data"]["id"] == Integer.to_string(project.organization_id)

This comment has been minimized.

@sbatson5

sbatson5 Nov 5, 2016

Contributor

This looks so much better 👏

This comment has been minimized.

@begedin

begedin Nov 8, 2016

Contributor

image

This is what I love the most about these.

@@ -30,24 +21,12 @@ defmodule CodeCorps.ProjectControllerTest do
project_1 = insert(:project, title: "Test Project 1", organization: organization)
project_2 = insert(:project, title: "Test Project 2", organization: organization)

This comment has been minimized.

@sbatson5

sbatson5 Nov 5, 2016

Contributor

Are the titles here necessary? Thoughts on reducing this to insert_pair and letting the factory generate the titles? Since we aren't testing for them, I'm not sure we need them to be unique.

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

Agree here.

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

Agreed here.

@joshsmith

Some changes here. Remember to ask yourself where the dividing line is on what you're testing.


conn = get conn, "/test-organization/test-project"

data =
conn
|> json_response(200)
|> Map.get("data")

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

The spacing on this is off now that data was removed. Make sure to watch your alignment.

description: "Test project description",
long_description_markdown: "A markdown **description**",
slug: "test-project")
project = insert(:project, title: "Test project", slug: "test-project")

conn = get conn, "/test-organization/test-project"

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

So this is confusing and brittle because we're relying on test-organization from the factory, but specifying test-project.

I think I would rather do:

organization = insert(:organization)
project = insert(:project, organization: organization)

path = "#{organization.slug}/#{project.slug}"

What do you think?

This comment has been minimized.

@amyschools

amyschools Nov 7, 2016

Contributor

That's much better!!

@@ -106,142 +68,66 @@ defmodule CodeCorps.ProjectControllerTest do
end

describe "create" do
@tag :authenticated
@tag authenticated: :admin

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

This should just be @tag :authenticated if you're relying on insert(:organization_membership, role: "admin"), otherwise you're going to defer to the site admin authentication since it takes precedence.

path = conn |> project_path(:create)
assert conn |> post(path, payload) |> json_response(403)
attrs = @invalid_attrs |> Map.merge(%{organization: organization})
assert conn |> request_create(attrs) |> json_response(403)

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

Why are you doing this extra work here? Would request_create on its own not work?

This comment has been minimized.

@amyschools

amyschools Nov 7, 2016

Contributor

I was having trouble getting this test to pass and this was the only solution that worked. I couldn't figure out why the usual request_create wasn't working so I just included the attributes. I'll look at it again today.

This comment has been minimized.

@amyschools

amyschools Nov 7, 2016

Contributor

Here's the error I'm getting: ** (FunctionClauseError) no function clause matching in CodeCorps.Helpers.Policy.get_membership/2

This comment has been minimized.

@joshsmith

joshsmith Nov 7, 2016

Contributor

Okay we'll dig into it when we pair.

test "renders 403 when not authorized", %{conn: conn} do
organization = insert(:organization)
attrs = @invalid_attrs |> Map.merge(%{organization: organization})
assert conn |> request_update(attrs) |> json_response(403)

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

Same as above, why not just request_update?

@@ -30,24 +21,12 @@ defmodule CodeCorps.ProjectControllerTest do
project_1 = insert(:project, title: "Test Project 1", organization: organization)
project_2 = insert(:project, title: "Test Project 2", organization: organization)

This comment has been minimized.

@joshsmith

joshsmith Nov 5, 2016

Contributor

Agreed here.

@coveralls

This comment has been minimized.

coveralls commented Nov 8, 2016

Coverage Status

Coverage increased (+0.3%) to 94.175% when pulling 9e1587b on 437-project-controller-test into 604f6df on develop.

@sbatson5

This still LGTM 👍

@begedin

begedin approved these changes Nov 8, 2016

👍

Others approved

@amyschools amyschools force-pushed the 437-project-controller-test branch from 9e1587b to 1cf4a86 Nov 11, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 11, 2016

Coverage Status

Coverage decreased (-0.2%) to 93.7% when pulling 1cf4a86 on 437-project-controller-test into e492da7 on develop.

@amyschools amyschools merged commit 1928206 into develop Nov 11, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 93.7%
Details
ci/circleci Your tests passed on CircleCI!
Details

@amyschools amyschools deleted the 437-project-controller-test branch Nov 11, 2016

@amyschools amyschools removed the in progress label Nov 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment